Something very wrong with layout_recall of RETURN_FILE

If any one is interested the below is the behavior I'm striving for.

----
From: Boaz Harrosh <[email protected]>
Subject: [PATCH] pnfsd-exofs: Two clients must not write to the same RAID stripe

If we have file redundancy RAID1/4/5/6 then two clients cannot write to the same stripe/region.

We take care of this by giving out smaller regions of the file. Before any layout_get we make sure to recall the same exact region from any client. If a recall was issued we return NFS4ERR_RECALLCONFLICT. The client will come again later for it's layout.

Meanwhile the fist client can flush data and release the layout. The next time the segment might be free and the lo_get succeed.

It is very possible that multiple writers will fight and some clients will starve forever. But the smaller the region, and if the clients randomize a wait, it should statistically be OK.
(We could manage a fairness queue. What about an lo_available
notification)

On the other hand a very small segment will hurt performance, so default size is now set to 8 stripes.
TODO: Let segment size be set in sysfs.

TODO:
For debugging we always give out small segments. But we should only start giving out small segments on a shared file. The first/single writer should get a large seg as before.

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/exofs/export.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

fs/exofs/export.c +42 -6

index c5712f3..a1f112f 100644
@@ -29,6 +29,9 @@
 
 #include "linux/nfsd/pnfs_osd_xdr_srv.h"
 
+/* TODO: put in sysfs per sb */
+const static unsigned sb_shared_num_stripes = 8;
+
 static int exofs_layout_type(struct super_block *sb)
 {
 	return LAYOUT_OSD2_OBJECTS;

@@ -94,14 +97,27 @@ void ore_layout_2_pnfs_layout(struct pnfs_osd_layout *pl,
 	}
 }
 
-static void _align_io(struct ore_layout *layout, u64 *offset, u64 *length)
+static bool _align_io(struct ore_layout *layout, struct nfsd4_layout_seg *lseg,
+		      bool shared)
 {
 	u64 stripe_size = (layout->group_width - layout->parity) *
 							layout->stripe_unit;
 	u64 group_size = stripe_size * layout->group_depth;
 
-	*offset = div64_u64(*offset, group_size) * group_size;
-	*length = group_size;
+	/* TODO: Don't ignore shared flag. Single writer can get a full group */
+	if (lseg->iomode != IOMODE_READ &&
+	    (layout->parity || (layout->mirrors_p1 > 1))) {
+		/* RAID writes */
+		lseg->offset = div64_u64(lseg->offset, stripe_size) *
+								stripe_size;
+		lseg->length = stripe_size * sb_shared_num_stripes;
+		return true;
+	} else {
+		/* reads or no data redundancy */
+		lseg->offset = div64_u64(lseg->offset, group_size) * group_size;
+		lseg->length = group_size;
+		return false;
+	}
 }
 
 static enum nfsstat4 exofs_layout_get(

@@ -116,15 +132,15 @@ static enum nfsstat4 exofs_layout_get(
 	struct pnfs_osd_layout layout;
 	__be32 *start;
 	unsigned i;
-	bool in_recall;
+	bool in_recall, need_recall;
 	enum nfsstat4 nfserr;
 
 	EXOFS_DBGMSG("(0x%lx) REQUESTED offset=0x%llx len=0x%llx iomod=0x%x\n",
 		     inode->i_ino, res->lg_seg.offset,
 		     res->lg_seg.length, res->lg_seg.iomode);
 
-	_align_io(&sbi->layout, &res->lg_seg.offset, &res->lg_seg.length);
-	res->lg_seg.iomode = IOMODE_RW;
+	need_recall = _align_io(&sbi->layout, &res->lg_seg,
+				test_bit(OBJ_LAYOUT_IS_GIVEN, &oi->i_flags));
 	res->lg_return_on_close = true;
 	res->lg_lo_cookie = inode; /* Just for debug prints */
 

@@ -132,6 +148,26 @@ static enum nfsstat4 exofs_layout_get(
 		     inode->i_ino, res->lg_seg.offset,
 		     res->lg_seg.length, res->lg_seg.iomode);
 
+	if (need_recall) {
+		int rc = cb_layout_recall(inode, IOMODE_RW, res->lg_seg.offset,
+				      res->lg_seg.length, (void *)0x17);
+		switch (rc) {
+		case 0:
+		case -EAGAIN:
+			EXOFS_DBGMSG("(0x%lx) @@@ Sharing of RAID5/1 stripe\n",
+				     inode->i_ino);
+			return NFS4ERR_RECALLCONFLICT;
+		default:
+			/* This is fine for now */
+			/* TODO: Fence object off */
+			EXOFS_DBGMSG("(0x%lx) !!!cb_layout_recall => %d\n",
+				     inode->i_ino, rc);
+			/*fallthrough*/
+		case -ENOENT:
+			break;
+		}
+	}
+
 	/* skip opaque size, will be filled-in later */
 	start = exp_xdr_reserve_qwords(xdr, 1);
 	if (!start) {

In patch:
pnfsd: layout recall layout state

the cl_has_file_layout() is no longer inspecting the layout structures added per file but is inspecting if file has layout_state.

So it is counting layout_states and not layouts

This is bad because the addition of the layout_states on the file is done before the call to the filesystem so if the FS does a recall, the nfsd is confused thinking it already has a layout and issues a recall. Instead of returning -ENOENT, ie list is empty. The client then truly returns nomaching_layout and when the lo_return(s) are emulated the system gets stuck is some reference miss-match. (UML so no crash trace)

Now lets say that the state should be set before the call to the FS. Then I don't see where the state is removed in the case of an ERROR return from FS->layout_get. Meaning cl_has_file_layout() will always think it has some count.

Also When a layout is returned it is the layout list that is inspected and freed, so how is the cl_has_file_layout() emptied ?

In any way. I do not agree that it is the state that is needed to be searched in cl_has_file_layout() but it is layouts that are needed, otherwise the all layout <---> recall very delicate dance is totally broken.

What was the meaning of the Poet?

I reverted the cl_has_file_layout() to historical processing and am debugging Will probably now get the state processing wrong.

Also cl_has_file_layout() returns true for any layout on a file, but we must inspect IO_MODE and LSEG for a partial-match, as well.

The below works for me. State also looks good
(lightly tested, bug above is fixed, Have not tried multiple clients shared
same-stripe writes)

Thanks
Boaz

------
git diff --stat -p -M fs/nfsd/nfs4pnfsd.c

 fs/nfsd/nfs4pnfsd.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

------

fs/nfsd/nfs4pnfsd.c +16 -14

index f90f3a7..b421437 100644
@@ -1179,24 +1179,27 @@ out:
 }
 
 static bool
-cl_has_file_layout(struct nfs4_client *clp, struct nfs4_file *fp, stateid_t *lsid)
+cl_has_file_layout(struct nfs4_client *clp, struct nfs4_file *fp,
+		   stateid_t *lsid, struct nfsd4_pnfs_cb_layout *cbl)
 {
-	struct nfs4_layout_state *ls;
+	struct nfs4_layout *lo;
+	bool ret = false;
 
 	spin_lock(&layout_lock);
-	list_for_each_entry (ls, &fp->fi_layout_states, ls_perfile)
-		if (same_clid(&ls->ls_stid.sc_stateid.si_opaque.so_clid,
-			      &clp->cl_clientid)) {
+	list_for_each_entry (lo, &fp->fi_layouts, lo_perfile) {
+		if (same_clid(&lo->lo_client->cl_clientid, &clp->cl_clientid) &&
+		    lo_seg_overlapping(&cbl->cbl_seg, &lo->lo_seg) &&
+		    (cbl->cbl_seg.iomode & lo->lo_seg.iomode))
 			goto found;
-		}
-	spin_unlock(&layout_lock);
-	return false;
-
+	}
+	goto unlock;
 found:
-	update_layout_stateid_locked(ls, lsid);
+	/* Im going to send a recall on this latout update state */
+	update_layout_stateid_locked(lo->lo_state, lsid);
+	ret = true;
+unlock:
 	spin_unlock(&layout_lock);
-
-	return true;
+	return ret;
 }
 
 static int

@@ -1228,7 +1231,7 @@ cl_has_layout(struct nfs4_client *clp, struct nfsd4_pnfs_cb_layout *cbl,
 {
 	switch (cbl->cbl_recall_type) {
 	case RETURN_FILE:
-		return cl_has_file_layout(clp, lrfile, lsid);
+		return cl_has_file_layout(clp, lrfile, lsid, cbl);
 	case RETURN_FSID:
 		return cl_has_fsid_layout(clp, &cbl->cbl_fsid);
 	default:
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

This should be fixed regardless so that exofs is more tolerant to "phantom" layout returns.

This wasn't the original intent for cl_has_file_layout.

I agree the requirement changed when we added the cookie magic and the reliance of exofs on the layout recall process to be precise about detecting the no layout case. So on one hand the process needs to be more robust and on the other we can lookup the exact region as you suggest below.

Then, we should be able to get rid of the layout states list altogether. (practically reverting "pnfsd: layout recall layout state")

Benny

Quoted 3 lines · Benny Halevy

This should be fixed regardless so that exofs is more tolerant to "phantom" layout returns.

It's not a crash at exofs, it's a crash at nfsd do to reference miss-match.

<>

Quoted 8 lines

This wasn't the original intent for cl_has_file_layout.

I agree the requirement changed when we added the cookie magic and the reliance of exofs on the layout recall process to be precise about detecting the no layout case. So on one hand the process needs to be more robust and on the other we can lookup the exact region as you suggest below.

I have lots of code changes to this, which works very well, to my satisfaction. It fixes the above and many other problems and is also a cleanup and fixture additions.

I would have sent it, if I was not busy with clients bugs found which are more urgent. The code is there and is being heavily tested as we speak. (mainly the client code, the server code is very good)

It'll take a few more days to send all this, in. Needs SPLITMEs and cleanup. (Tell me if you want RFC level code which will be harder for review, before hand)

Quoted 3 lines

Then, we should be able to get rid of the layout states list altogether. (practically reverting "pnfsd: layout recall layout state")

I have not removed this. As you say it's by now dead code. I'll send in what I have and we can surgically revert that thing as well. It will all be in SQUASHMEs and we can later re-arrange the patches for this to naturally fall off the patchlist. (I intend to help a bit with this work, in the areas these touch)

From 537ddc4c2d35c820a5c74b783b139dbc11a36531 Mon Sep 17 00:00:00 2001

Quoted 3 lines

Benny

Thanks
Boaz

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html