Re: [patch] nfs: fix locking in nfs/inode.c in nfs_free_open_context

[Trond Myklebust <trond.myklebust@fys.uio.no>]

On Thu, 2007-07-26 at 14:44 +0200, Christian Krafft wrote:
> On Thu, 26 Jul 2007 13:23:37 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > On Wednesday 25 July 2007, Trond Myklebust wrote:
> > > 
> > > On Wed, 2007-07-25 at 17:08 +0200, Christian Krafft wrote:
> > > 
> > > > Obviously the locking code in nfs_free_open_context is wrong.
> > > > Checking the list for entries and removing the entry should be an atomic operation.
> > > 
> > > Wrong. It is quite safe to test the structure member ctx->list for
> > > emptiness outside the spinlock because we have an explicit guarantee
> > > that nobody else has a reference to this structure, plus the
> > > atomic_dec_and_test() in kref_put() has acted as a memory barrier for
> > > us.
> > 
> > Well, the real question then is how the ctx can still be present in the
> > nfsi->open_files list. Since we are in nfs_free_open_context(), there
> > must not be any pointer to the ctx anywhere, but still we have this other
> > thread calling get_nfs_open_context() on it.
> > 
> > 	Arnd <><
> 
> Thanks for the pointer Arnd,
> 
> Trond, does the patch below look better to you ?

No. That is still incorrect. The list of open contexts is used for
things like NFSv4 state recovery (when we're doing background writes,
and the server happens to reboot). The lifetime of the open context may
exceed that of the struct file that created it.

Trond

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/