[haiku-commits] Re: r35069 - haiku/trunk/src/add-ons/kernel/file_systems/ext2
- From: Ingo Weinhold <ingo_weinhold@xxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Thu, 14 Jan 2010 20:23:44 +0100
On 2010-01-14 at 20:01:59 [+0100], Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
wrote:
> Ingo Weinhold <ingo_weinhold@xxxxxx> wrote:
> > On 2010-01-14 at 13:36:14 [+0100], axeld@xxxxxxxxxxxxxxxx wrote:
> > > Author: axeld
> > > Date: 2010-01-14 13:36:14 +0100 (Thu, 14 Jan 2010)
> > > New Revision: 35069
> > > Changeset: http://dev.haiku-os.org/changeset/35069/haiku
> > >
> > > Modified:
> > > haiku/trunk/src/add-ons/kernel/file_systems/ext2/Volume.cpp
> > > haiku/trunk/src/add-ons/kernel/file_systems/ext2/Volume.h
> > > Log:
> > > * Volume::Unmount() never put the root node,
> > Nor should it. At least that's what the documentation states and
> > fs_unmount()
> > seems to agree with that.
>
> Could it be that you changed that without me noticing it? :-)
Nope. Even back in r10 the root node reference was released explicitly.
Interestingly, fat's dosfs_unmount() says:
// Unlike in BeOS, we need to put the reference to our root node
ourselves
Introduced by korli in r18667, though also at that revision fs_unmount()
freed all root node references itself.
> At least BFS does that, too -- it should be harmless, though.
Yep, put_vnode() is a no-op in that case. Though I think we should consider
making it panic() instead. I don't see a valid reason for calling
put_vnode(), when one doesn't have a reference to the node.
> However, the fs_unmount() code looks a bit fishy, but maybe I'm missing
> something:
> (line 7282ff.)
>
> VnodeList::Iterator iterator = mount->vnodes.GetIterator();
> while (struct vnode* vnode = iterator.Next()) {
> vnode->SetBusy(true);
> vnode_to_be_freed(vnode);
> }
>
> // The ref_count of the root node is 1 at this point, see above why
> this is
> mount->root_vnode->ref_count--;
> vnode_to_be_freed(mount->root_vnode);
>
> Isn't vnode_to_be_freed() twice here for the root node?
> It doesn't really matter, though, since nothing bad happens in
> vnode_to_be_freed() - it just doesn't clear kFlagsHot, so the hot path
> might be walked twice.
> I guess we should just reduce the ref before the loop, and that's it?
Mmh, I find it a bit worrisome though, that the root node is freed before
the FS's unmount() hook is called. The root node is supposed to be still
valid at that point.
CU, Ingo
Other related posts:
