On Tue, 20 Nov 2012, Attilio Rao wrote:
On 11/20/12, Konstantin Belousov <kostik...@gmail.com> wrote:
On Tue, Nov 20, 2012 at 01:27:07PM +0000, Attilio Rao wrote:
On 11/20/12, Bruce Evans <b...@optusnet.com.au> wrote:
...
I think I am right that the code makes no sense. It is ordered like
it is because placing the allocation of ip before the allocation of
vp used to be enough to prevent v_data being dereferenced. This makes
no sense when it isn't enough.
In the past, before VFS got locking and kernel was single-threaded,
the comment and code arranged in this way were sensitive and
effective.
As now this is not true anymore, there is no strict relationship
between the getnewvnode() and sleeping points.
It is important to remove stale comments because they confuse people,
the porters and as you can see the code/comment has been cut&paste
quite a bit around.
Putting the issue of the comment making no sense at all aside, which is
true but tangential to what I want to note.
Although malloc(9) is not ordered with the vnode locks in the lock
order, it is still good practice to not perform allocations under the
vnode lock. The reason is that pagedaemon might need to clean and write
pages, in particular, belonging to the vnodes. Generally, if you own
vnode lock and do something which might kick pagedaemon, you are creating
troubles for pagedaemon, which would attempt to lock the vnode with
timeout, spending the precious time waiting for lock it cannot obtain.
This is less an issue for the new vnode instantiation locations, because
vnode cannot have resident pages yet. But it is good to follow the same
pattern anywhere.
The comment could have been changed to one about this. Except it doesn't
really apply here. Neither of the allocations is onder the vnode lock.
Yes, I completely agree. Infact the code is not changed also to avoid
pagedaemon hosing.
However this is a different situation respect a "if you allocate while
you hold the lock you will get a deadlock/race/corruption".
The code is still nonsense since its order is unrelated to this too :-).
The natural code and order is:
o // no lock held here
o allocate vp
o allocate ip and any other stuff needed to complete vp
o aquire exclusive lock
o check that vp, ip and other stuff didn't go away
o wire ip and other stuff into vp // shouldn't do more allocations here
o release lock on return (?)
but the current order for at least ffs is:
o // no lock held here
o // oops, actually we always (?) hold at least a shared lock here. Often
// we need to promote to an exclusive lock.
o allocate ip
o allocate vp
o aquire exclusive lock
// no check that nothing went away?? A comment earlier still says that
// we intentionally allow races earlier. Is that correct when we hold
// at leasr a shared lock throughout?
o wire ip into vp. Includes futher initializations of both. Hopefully
these can't block.
bread() the inode. Can block now. No worse than blocking for write(2) (?).
o further zalloc()s for dinodes. Breaks kib's rule. It would be painful
to have to release all the zalloc()ated data on earlier failures. At
this point, we can still fail, but have initialized enough of the vnode
to just fail without cleaning it up -- ufs_reclaim() will clean it.
o further initializations, mostly not involving operations that can block.
release lock on return (?)
The allocation of ip is clearly special -- it contains state info that
hopefully records the state of the initialization, and it must be wired into
vp first so that desrtructors can see this info.
Bruce
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
