På ty , 10/02/2004 klokka 10:15, skreiv Olaf Kirch:
> It wasn't in 2.4.19 or whenever I ran into this. And a per-transport
lock isn't enough to protect a global variable shared by all transports.
Trust me: I spent several weeks debugging this particular problem :)
(See the PS for a somewhat lengthy description of what I remember)
Duh... I forgot that the XID was still global...
Does anybody see any problems with moving the XID counter into the struct
xprt? Please bear in mind that the replay caches on modern NFS servers
look at XID+client IP address+port number, so 2 sockets that play the same
XID to the server should not normally conflict.
> I think that BKL removal at this point is probably not a good idea. Suse
had a BKL removal patch in their SLES8 kernel, and I spent a total of
4-5
> weeks hunting down all sorts of bugs related to SMP races. And I don't
think we've seen all of these bugs yet. It might be better to postpone
this for the 2.7 branch.
I totally agree with this...
No extra work has been done on BKL removal in the 2.6 branch, and I know
that the old patch that was up for testing in the 2.4 tree was buggy in
several ways...
> > The code in unlink.c is only called from dentry_iput() or from
sillyrename(). In either case, there is no possibility for SMP races.
>
> Are you sure? nfs_dentry_iput explicitly does a lock_kernel before
calling
> nfs_complete_unlink. I think that's because dentry_iput explicitly drops
the dcache_lock before calling dentry->d_op->d_iput. I wouldn't remove
that BKL :)
Yep, but there is only one inode per dentry, and the VFS rules forbids you
to convert a negative dentry into a positive one (the contrary is allowed,
but only under certain restricted situations).
> And even if you leave that BKL and just remove the one in rpciod, you
may
> still have a race between rpciod and dput. One CPU may be adding entries
inside nfs_async_unlink (called from dentry_iput()), and at the same
time rpciod can run on another CPU, calling nfs_async_unlink_release,
removing stuff from the nfs_deletes list. The reference counting on
nfs_unlinkdata isn't atomic either.
Sure... The unlink.c global lists definitely depend on the BKL for
protection. The latter would need to be replaced by a new global
spinlock.
Cheers,
Trond
|