Re: [PATCH] nfs: fix inifinite loop at nfs4_layoutcommit_release

[Vitaliy Gusev <gusev.vitaliy@nexenta.com>]

On 09/08/2011 02:00 PM, tao.peng@emc.com wrote:
> Hi, Gusev,

Hello Tao!

> Yes, you are right. How about following patch?
>
>  From 14c6da67565fb31c2d2775ccefd93251f348910d Mon Sep 17 00:00:00 2001
> From: Peng Tao<bergwolf@gmail.com>
> Date: Thu, 8 Sep 2011 00:57:02 -0400
> Subject: [PATCH] nfsv4: fix race in layoutcommit lseg list create/free
>
> Since there can be more than one layoutcommit proc happen the same time,
> lseg list create/free should be protected. Otherwise lseg list
> may get corrupted.
>
> Reported-by: Vitaliy Gusev<gusev.vitaliy@nexenta.com>
> Signed-off-by: Peng Tao<peng_tao@emc.com>
> ---
>   fs/nfs/nfs4proc.c |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 8c77039..da7c20c 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5964,6 +5964,7 @@ static void nfs4_layoutcommit_release(void *calldata)
>   	struct pnfs_layout_segment *lseg, *tmp;
>
>   	pnfs_cleanup_layoutcommit(data);
> +	spin_lock(&data->args.inode->i_lock);

I think lock over list_del_init(&lseg->pls_lc_list) is enough, because

1. here lseg is deleted from unique (placed in stack) data and there is 
no any race during deletion.


2. Only one thing must be protected - list_empty status at 
pnfs_list_write_lseg (after my patch).

    The problem is that list_del_init is placed before 
test_and_clear_bit and spinlock can be some kind of barrier for 
protection against adding lseg to new data->lseg_list at
pnfs_list_write_lseg.

    Do reordering list_del_init with test_and_clear_bit is not good, 
because lseg can be invalid after put_lseg.


>   	/* Matched by references in pnfs_set_layoutcommit */
>   	list_for_each_entry_safe(lseg, tmp,&data->lseg_list, pls_lc_list) {
>   		list_del_init(&lseg->pls_lc_list);
> @@ -5971,6 +5972,7 @@ static void nfs4_layoutcommit_release(void *calldata)
>   				&lseg->pls_flags))
>   			put_lseg(lseg);
>   	}
> +	spin_unlock(&data->args.inode->i_lock);
>   	put_rpccred(data->cred);
>   	kfree(data);
>   }