[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <200707271144.31550.arnd@arndb.de>
Date: Fri, 27 Jul 2007 11:44:30 +0200
From: Arnd Bergmann <arnd@...db.de>
To: Trond Myklebust <trond.myklebust@....uio.no>
Cc: Christian Krafft <krafft@...ibm.com>, linux-kernel@...r.kernel.org,
stable@...nel.org
Subject: Re: [patch] nfs: fix locking in nfs/inode.c in nfs_free_open_context
On Thursday 26 July 2007, Trond Myklebust wrote:
> Really ugly. Here's an alternative that is a lot more palatable.
Yes, much better than mine.
> ---------------------
> From: Trond Myklebust <Trond.Myklebust@...app.com>
> Date: Thu, 26 Jul 2007 12:06:17 -0400
> NFS: Fix put_nfs_open_context
>
> We need to grab the inode->i_lock atomically with the last reference put in
> order to remove the open context that is being freed from the
> nfsi->open_files list.
>
> Fix by converting the kref to a standard atomic counter and then using
> atomic_dec_and_lock()...
>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@...app.com>
Acked-by: Arnd Bergmann <arnd@...db.de>
>From the bit of research I did on the bug yesterday, it seems
that the race has been in there for ages, but may have become easier
to hit with the change to kref after 2.6.22. I don't really
understand why we didn't hit it in RHEL5/2.6.18 with the same
test case, but in Fedora 7/2.6.21.
Should a patch like the one below (I said like, you know you
can't trust my patches for your code ;-) be put into 2.6.22.x
and updated distro kernels?
Arnd <><
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -482,21 +482,19 @@ struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx)
void put_nfs_open_context(struct nfs_open_context *ctx)
{
- if (atomic_dec_and_test(&ctx->count)) {
- if (!list_empty(&ctx->list)) {
- struct inode *inode = ctx->dentry->d_inode;
- spin_lock(&inode->i_lock);
- list_del(&ctx->list);
- spin_unlock(&inode->i_lock);
- }
- if (ctx->state != NULL)
- nfs4_close_state(ctx->state, ctx->mode);
- if (ctx->cred != NULL)
- put_rpccred(ctx->cred);
- dput(ctx->dentry);
- mntput(ctx->vfsmnt);
- kfree(ctx);
- }
+ struct inode *inode = ctx->dentry->d_inode;
+
+ if (!atomic_dec_and_lock(&ctx->count, &inode->i_lock))
+ return;
+ list_del(&ctx->list);
+ spin_unlock(&inode->i_lock);
+ if (ctx->state != NULL)
+ nfs4_close_state(ctx->state, ctx->mode);
+ if (ctx->cred != NULL)
+ put_rpccred(ctx->cred);
+ dput(ctx->dentry);
+ mntput(ctx->vfsmnt);
+ kfree(ctx);
}
/*
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists