Hi, Gusev, > -----Original Message----- > From: Vitaliy Gusev [mailto:gusev.vitaliy@xxxxxxxxxxx] > Sent: Wednesday, September 07, 2011 6:14 AM > To: Trond Myklebust > Cc: Vitaliy Gusev; Peng, Tao; linux-nfs@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] nfs: fix inifinite loop at nfs4_layoutcommit_release > > >> @@ -1376,7 +1376,8 @@ static void pnfs_list_write_lseg(struct inode *inode, > struct list_head *listp) > >> > >> list_for_each_entry(lseg,&NFS_I(inode)->layout->plh_segs, pls_list) { > >> if (lseg->pls_range.iomode == IOMODE_RW&& > >> - test_bit(NFS_LSEG_LAYOUTCOMMIT,&lseg->pls_flags)) > >> + test_bit(NFS_LSEG_LAYOUTCOMMIT,&lseg->pls_flags)&& > >> + list_empty(&lseg->pls_lc_list)) > >> list_add(&lseg->pls_lc_list, listp); > >> } > >> } > > > > If the lseg is already part of one layoutcommit, but we're sending a > > second one for the same range (presumably because we wrote more data in > > the same region), then the above causes the lseg to be excluded. > > > Yes, lseg is excluded, This patch does exactly only exclusion of lseg. > lseg is used here only to get/put reference on this lseg, so skipping is > correct. > > > However, checking on list_empty can occur (on other CPU) in the middle: > > list_del_init(&lseg->pls_lc_list); > Here >>>>>> > if (test_and_clear_bit(NFS_LSEG_LAYOUTCOMMIT, > &lseg->pls_flags)) > put_lseg(lseg); > > > So list_del_init must be executed under the same lock as > pnfs_list_write_lseg, i.e. inode->i_lock. Yes, you are right. How about following patch? >From 14c6da67565fb31c2d2775ccefd93251f348910d Mon Sep 17 00:00:00 2001 From: Peng Tao <bergwolf@xxxxxxxxx> 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@xxxxxxxxxxx> Signed-off-by: Peng Tao <peng_tao@xxxxxxx> --- 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); /* 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); } -- 1.7.4.2 > > > > > > I agree that the current code causes list corruption, but before > > applying something like the above patch, I'd like to understand why it > > is correct. > > > > Trond > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
