[PATCH] FIXME: pnfs: BUG in layout_commit logic

[PATCH] FIXME: pnfs: BUG in layout_commit logic

[Boaz Harrosh]

When segments are used with the pnfs-objects driver (which supports them)
I get a crash in layout commit do to unbalanced lseg reference counting.
(under reference)

With this patch taken from the blocks layout part of the patchset this
problem goes away. But looking at the code it looks scary to me
(I just don't understand it fully).

Fred please review this code, to see if you like what it does.

I will need a fix for the v3.0 Kernel

SOB: Boaz
---
 fs/nfs/pnfs.c |   21 ++++++++++++++++++---
 fs/nfs/pnfs.h |    1 +
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 5fc2e5d..cf048c0 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1246,10 +1246,19 @@ pnfs_try_to_read_data(struct nfs_read_data *rdata,
 static struct pnfs_layout_segment *pnfs_list_write_lseg(struct inode *inode)
 {
 	struct pnfs_layout_segment *lseg, *rv = NULL;
+	loff_t max_pos = 0;
+
+	list_for_each_entry(lseg, &NFS_I(inode)->layout->plh_segs, pls_list) {
+		if (lseg->pls_range.iomode == IOMODE_RW) {
+			if (max_pos < lseg->pls_end_pos)
+				max_pos = lseg->pls_end_pos;
+			if (test_and_clear_bit
+			    (NFS_LSEG_LAYOUTCOMMIT, &lseg->pls_flags))
+				rv = lseg;
+		}
+	}
+	rv->pls_end_pos = max_pos;
 
-	list_for_each_entry(lseg, &NFS_I(inode)->layout->plh_segs, pls_list)
-		if (lseg->pls_range.iomode == IOMODE_RW)
-			rv = lseg;
 	return rv;
 }
 
@@ -1258,21 +1267,27 @@ pnfs_set_layoutcommit(struct nfs_write_data *wdata)
 {
 	struct nfs_inode *nfsi = NFS_I(wdata->inode);
 	loff_t end_pos = wdata->mds_offset + wdata->res.count;
+	loff_t isize = i_size_read(wdata->inode);
 	bool mark_as_dirty = false;
 
 	spin_lock(&nfsi->vfs_inode.i_lock);
 	if (!test_and_set_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->flags)) {
 		/* references matched in nfs4_layoutcommit_release */
 		get_lseg(wdata->lseg);
+		set_bit(NFS_LSEG_LAYOUTCOMMIT, &wdata->lseg->pls_flags);
 		wdata->lseg->pls_lc_cred =
 			get_rpccred(wdata->args.context->state->owner->so_cred);
 		mark_as_dirty = true;
 		dprintk("%s: Set layoutcommit for inode %lu ",
 			__func__, wdata->inode->i_ino);
 	}
+	if (end_pos > isize)
+		end_pos = isize;
 	if (end_pos > wdata->lseg->pls_end_pos)
 		wdata->lseg->pls_end_pos = end_pos;
 	spin_unlock(&nfsi->vfs_inode.i_lock);
+	dprintk("%s: lseg %p end_pos %llu\n",
+		__func__, wdata->lseg, wdata->lseg->pls_end_pos);
 
 	/* if pnfs_layoutcommit_inode() runs between inode locks, the next one
 	 * will be a noop because NFS_INO_LAYOUTCOMMIT will not be set */
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index e46edac..4a30e81 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -36,6 +36,7 @@
 enum {
 	NFS_LSEG_VALID = 0,	/* cleared when lseg is recalled/returned */
 	NFS_LSEG_ROC,		/* roc bit received from server */
+	NFS_LSEG_LAYOUTCOMMIT,	/* layoutcommit bit set for layoutcommit */
 };
 
 struct pnfs_layout_segment {
-- 
1.7.3.4

Re: [PATCH] FIXME: pnfs: BUG in layout_commit logic

[Boaz Harrosh]

On 06/17/2011 01:20 PM, Boaz Harrosh wrote:
> 
> When segments are used with the pnfs-objects driver (which supports them)
> I get a crash in layout commit do to unbalanced lseg reference counting.
> (under reference)
> 
> With this patch taken from the blocks layout part of the patchset this
> problem goes away. But looking at the code it looks scary to me
> (I just don't understand it fully).
> 
> Fred please review this code, to see if you like what it does.
> 
> I will need a fix for the v3.0 Kernel
> 
> SOB: Boaz
<snip>

Benny, Fred, Hi.

This patch is from the pnfsblock branch:
	[0e5ef56b] pnfs: let layoutcommit code handle multiple segments
		   by: Peng Tao<bergwolf@gmail.com>

Except the above patch has one obvious bug where segments are not found
(See below)

Fred I'm not sure I like this patch it looks wrong to me. But it does
solve my immediate problem (Needed for this Kernel). Have you had a
chance to look at this code and think about what we should do?

Thanks
Boaz
---
SQUASHME: Fix BUG in: [0e5ef56b] pnfs: let layoutcommit code handle multiple segments

The patch: [0e5ef56b] pnfs: let layoutcommit code handle multiple segments
Broke pnfs_find_lseg because now with the new break, the smaller then last layouts
are never found.
(Though I agree that searching in reverse might be an optimization since the
 last one is usually what we need. (Perhaps reverse the offset test))

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index d9ffaab..169fe92 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -905,7 +905,7 @@ pnfs_find_lseg(struct pnfs_layout_hdr *lo,
 	dprintk("%s:Begin\n", __func__);
 
 	assert_spin_locked(&lo->plh_inode->i_lock);
-	list_for_each_entry_reverse(lseg, &lo->plh_segs, pls_list) {
+	list_for_each_entry(lseg, &lo->plh_segs, pls_list) {
 		if (test_bit(NFS_LSEG_VALID, &lseg->pls_flags) &&
 		    is_matching_lseg(&lseg->pls_range, range)) {
 			ret = get_lseg(lseg);