Re: [PATCH v1 02/16] nfsd: remove unneeded spinlock in nfsd_cache_update

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Jan 28, 2013 at 02:41:08PM -0500, Jeff Layton wrote:
> The locking rules for cache entries say that locking the cache_lock
> isn't needed if you're just touching the current entry. Earlier
> in this function we set rp->c_state to RC_UNUSED without any locking,
> so I believe it's ok to do the same here.

Also, the previous assignment to cachv->iov_base is a write to the same
entry without a lock.

It's still a little odd to be writing to this entry without holding any
lock.  I have to stare at it for a while to convince myself it's OK, but
I guess it is: this entry is transitioning from RC_INUSE to RC_UNUSED,
but it's not modifying the entry otherwise (that cachv->iov_base
assignment was actually a no-op in this case since it had to be NULL
already).  So it shouldn't matter that there's no write barrier or
anything.  At worst another thread may be a little late to notice this
entry is free.

OK, applying.  Could use a comment or something though.

--b.

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/nfsd/nfscache.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> index 5dd9ec2..972c14a 100644
> --- a/fs/nfsd/nfscache.c
> +++ b/fs/nfsd/nfscache.c
> @@ -286,9 +286,7 @@ nfsd_cache_update(struct svc_rqst *rqstp, int cachetype, __be32 *statp)
>  		cachv = &rp->c_replvec;
>  		cachv->iov_base = kmalloc(len << 2, GFP_KERNEL);
>  		if (!cachv->iov_base) {
> -			spin_lock(&cache_lock);
>  			rp->c_state = RC_UNUSED;
> -			spin_unlock(&cache_lock);
>  			return;
>  		}
>  		cachv->iov_len = len << 2;
> -- 
> 1.7.11.7
>