[PATCH 0/5] pnfs-submit: embed pnfs_layout_type in per layout structure

[PATCH 0/5] pnfs-submit: embed pnfs_layout_type in per layout structure

[andros]

A cleanup from Fred.
0001-SQUASHME-pnfs-submit-add-state-flag-for-layoutcommit.patch

These patches introduce the allocation of the pnfs_layout_type changing the
static declaration in struct nfs_inode to a pointer.

0002-SQUASHME-pnfs_submit-move-pnfs_layout_state-back-to-.patch
0003-SQUASHME-pnfs-submit-move-pnfs_layout_suspend-back-t.patch
0004-SQUASHME-pnfs-submit-embed-pnfs_layout_type.patch
0005-SQUASHME-pnfs-submit-filelayout-use-new-alloc_layout.patch

Testing

CONFIG_NFS_V4_1 set:
Connectathon passes on a VM SMP client against GFS2/pNFS with return_on_close
and write layouts turned on, and against the pyNFS file layout server without
return on close set.

CONFIG_NFS_V4_1 not set
NFSv4.0 Cthon tests pass


-->Andy

[PATCH 1/5] SQUASHME pnfs-submit: add state flag for layoutcommit_needed

[andros]

From: Fred Isaman <iisaman@netapp.com>

This avoids locking and existance of layout issues

Signed-off-by: Fred Isaman <iisaman@netapp.com>
---
 fs/nfs/pnfs.c             |    2 ++
 include/linux/nfs4_pnfs.h |    2 +-
 include/linux/nfs_fs.h    |    1 +
 3 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 9a295cf..bf15b5c 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -154,6 +154,7 @@ pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
 	spin_lock(&nfsi->vfs_inode.i_lock);
 	if (has_layout(nfsi) && !layoutcommit_needed(nfsi)) {
 		nfsi->layout.lo_cred = get_rpccred(ctx->state->owner->so_cred);
+		__set_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->pnfs_layout_state);
 		nfsi->change_attr++;
 		spin_unlock(&nfsi->vfs_inode.i_lock);
 		dprintk("%s: Set layoutcommit\n", __func__);
@@ -1635,6 +1636,7 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
 	nfsi->layout.pnfs_write_begin_pos = 0;
 	nfsi->layout.pnfs_write_end_pos = 0;
 	nfsi->layout.lo_cred = NULL;
+	__clear_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->pnfs_layout_state);
 	pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
 
 	spin_unlock(&inode->i_lock);
diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
index d20b5de..4d9b15c 100644
--- a/include/linux/nfs4_pnfs.h
+++ b/include/linux/nfs4_pnfs.h
@@ -83,7 +83,7 @@ has_layout(struct nfs_inode *nfsi)
 static inline bool
 layoutcommit_needed(struct nfs_inode *nfsi)
 {
-	return nfsi->layout.lo_cred != NULL;
+	return test_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->pnfs_layout_state);
 }
 
 #else /* CONFIG_NFS_V4_1 */
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index a60d880..a33e86e 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -109,6 +109,7 @@ struct pnfs_layout_type {
 	#define NFS_INO_RO_LAYOUT_FAILED 0      /* get ro layout failed stop trying */
 	#define NFS_INO_RW_LAYOUT_FAILED 1      /* get rw layout failed stop trying */
 	#define NFS_INO_LAYOUT_ALLOC     2      /* bit lock for layout allocation */
+	#define NFS_INO_LAYOUTCOMMIT     3	/* LAYOUTCOMMIT needed */
 	time_t pnfs_layout_suspend;
 	struct rpc_cred         *lo_cred; /* layoutcommit credential */
 	/* DH: These vars keep track of the maximum write range
-- 
1.6.6

[PATCH 2/5] SQUASHME: pnfs_submit: move pnfs_layout_state back to nfs_inode

[andros]

From: Andy Adamson <andros@netapp.com>

Prepare to embed struct pnfs_layout_tupe into layout driver private structs
and to make layout a pointer: state flags need to be accessible before
allocation.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/inode.c         |    2 +-
 fs/nfs/pnfs.c          |   16 ++++++++--------
 include/linux/nfs_fs.h |   10 +++++-----
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 7989dea..66d7be2 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1364,7 +1364,6 @@ void nfs4_clear_inode(struct inode *inode)
 static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
 {
 #ifdef CONFIG_NFS_V4_1
-	nfsi->layout.pnfs_layout_state = 0;
 	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
 	nfsi->layout.roc_iomode = 0;
 	nfsi->layout.lo_cred = NULL;
@@ -1420,6 +1419,7 @@ static void pnfs_init_once(struct nfs_inode *nfsi)
 {
 #ifdef CONFIG_NFS_V4_1
 	init_waitqueue_head(&nfsi->lo_waitq);
+	nfsi->pnfs_layout_state = 0;
 	seqlock_init(&nfsi->layout.seqlock);
 	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
 	INIT_LIST_HEAD(&nfsi->layout.segs);
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index bf15b5c..d05cb03 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -951,7 +951,7 @@ get_lock_alloc_layout(struct inode *ino)
 		 * wait until bit is cleared if we lost this race.
 		 */
 		res = wait_on_bit_lock(
-			&nfsi->layout.pnfs_layout_state,
+			&nfsi->pnfs_layout_state,
 			NFS_INO_LAYOUT_ALLOC,
 			pnfs_wait_schedule, TASK_KILLABLE);
 		if (res) {
@@ -975,8 +975,8 @@ get_lock_alloc_layout(struct inode *ino)
 
 		/* release the NFS_INO_LAYOUT_ALLOC bit and wake up waiters */
 		clear_bit_unlock(NFS_INO_LAYOUT_ALLOC,
-				 &nfsi->layout.pnfs_layout_state);
-		wake_up_bit(&nfsi->layout.pnfs_layout_state,
+				 &nfsi->pnfs_layout_state);
+		wake_up_bit(&nfsi->pnfs_layout_state,
 			    NFS_INO_LAYOUT_ALLOC);
 		break;
 	}
@@ -1104,12 +1104,12 @@ _pnfs_update_layout(struct inode *ino,
 	}
 
 	/* if get layout already failed once goto out */
-	if (test_bit(lo_fail_bit(iomode), &nfsi->layout.pnfs_layout_state)) {
+	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
 		if (unlikely(nfsi->layout.pnfs_layout_suspend &&
 		    get_seconds() >= nfsi->layout.pnfs_layout_suspend)) {
 			dprintk("%s: layout_get resumed\n", __func__);
 			clear_bit(lo_fail_bit(iomode),
-				  &nfsi->layout.pnfs_layout_state);
+				  &nfsi->pnfs_layout_state);
 			nfsi->layout.pnfs_layout_suspend = 0;
 		} else
 			goto out_put;
@@ -1121,7 +1121,7 @@ _pnfs_update_layout(struct inode *ino,
 	send_layoutget(ino, ctx, &arg, lsegpp, lo);
 out:
 	dprintk("%s end, state 0x%lx lseg %p\n", __func__,
-		nfsi->layout.pnfs_layout_state, lseg);
+		nfsi->pnfs_layout_state, lseg);
 	return;
 out_put:
 	if (lsegpp)
@@ -1226,12 +1226,12 @@ pnfs_get_layout_done(struct nfs4_pnfs_layoutget *lgp, int rpc_status)
 	/* remember that get layout failed and suspend trying */
 	nfsi->layout.pnfs_layout_suspend = suspend;
 	set_bit(lo_fail_bit(lgp->args.lseg.iomode),
-		&nfsi->layout.pnfs_layout_state);
+		&nfsi->pnfs_layout_state);
 	dprintk("%s: layout_get suspended until %ld\n",
 		__func__, suspend);
 out:
 	dprintk("%s end (err:%d) state 0x%lx lseg %p\n",
-		__func__, lgp->status, nfsi->layout.pnfs_layout_state, lseg);
+		__func__, lgp->status, nfsi->pnfs_layout_state, lseg);
 	return;
 }
 
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index a33e86e..e216e24 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -105,11 +105,6 @@ struct pnfs_layout_type {
 	seqlock_t seqlock;		/* Protects the stateid */
 	nfs4_stateid stateid;
 	void *ld_data;			/* layout driver private data */
-	unsigned long pnfs_layout_state;
-	#define NFS_INO_RO_LAYOUT_FAILED 0      /* get ro layout failed stop trying */
-	#define NFS_INO_RW_LAYOUT_FAILED 1      /* get rw layout failed stop trying */
-	#define NFS_INO_LAYOUT_ALLOC     2      /* bit lock for layout allocation */
-	#define NFS_INO_LAYOUTCOMMIT     3	/* LAYOUTCOMMIT needed */
 	time_t pnfs_layout_suspend;
 	struct rpc_cred         *lo_cred; /* layoutcommit credential */
 	/* DH: These vars keep track of the maximum write range
@@ -208,6 +203,11 @@ struct nfs_inode {
 #if defined(CONFIG_NFS_V4_1)
 	wait_queue_head_t lo_waitq;
 	struct pnfs_layout_type layout;
+	unsigned long pnfs_layout_state;
+	#define NFS_INO_RO_LAYOUT_FAILED 0 /* ro LAYOUTGET failed stop trying */
+	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop trying */
+	#define NFS_INO_LAYOUT_ALLOC     2 /* bit lock for layout allocation */
+	#define NFS_INO_LAYOUTCOMMIT     3 /* LAYOUTCOMMIT needed */
 #endif /* CONFIG_NFS_V4_1 */
 #endif /* CONFIG_NFS_V4*/
 #ifdef CONFIG_NFS_FSCACHE
-- 
1.6.6

[PATCH 3/5] SQUASHME: pnfs-submit: move pnfs_layout_suspend back to nfs_inode

[andros]

From: Fred Isaman <iisaman@netapp.com>

Prepare to embed struct pnfs_layout_tupe into layout driver private structs
and to make layout a pointer. Since a temp error on first LAYOUTGET
erases the layout, the suspend timer needs to be moved.

Signed-off-by: Fred Isaman <iisaman@netapp.com>
---
 fs/nfs/inode.c         |    1 +
 fs/nfs/pnfs.c          |    8 ++++----
 include/linux/nfs_fs.h |    2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 66d7be2..cb12d33 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1420,6 +1420,7 @@ static void pnfs_init_once(struct nfs_inode *nfsi)
 #ifdef CONFIG_NFS_V4_1
 	init_waitqueue_head(&nfsi->lo_waitq);
 	nfsi->pnfs_layout_state = 0;
+	nfsi->pnfs_layout_suspend = 0;
 	seqlock_init(&nfsi->layout.seqlock);
 	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
 	INIT_LIST_HEAD(&nfsi->layout.segs);
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index d05cb03..7f6ace2 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1105,12 +1105,12 @@ _pnfs_update_layout(struct inode *ino,
 
 	/* if get layout already failed once goto out */
 	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
-		if (unlikely(nfsi->layout.pnfs_layout_suspend &&
-		    get_seconds() >= nfsi->layout.pnfs_layout_suspend)) {
+		if (unlikely(nfsi->pnfs_layout_suspend &&
+		    get_seconds() >= nfsi->pnfs_layout_suspend)) {
 			dprintk("%s: layout_get resumed\n", __func__);
 			clear_bit(lo_fail_bit(iomode),
 				  &nfsi->pnfs_layout_state);
-			nfsi->layout.pnfs_layout_suspend = 0;
+			nfsi->pnfs_layout_suspend = 0;
 		} else
 			goto out_put;
 	}
@@ -1224,7 +1224,7 @@ pnfs_get_layout_done(struct nfs4_pnfs_layoutget *lgp, int rpc_status)
 	}
 
 	/* remember that get layout failed and suspend trying */
-	nfsi->layout.pnfs_layout_suspend = suspend;
+	nfsi->pnfs_layout_suspend = suspend;
 	set_bit(lo_fail_bit(lgp->args.lseg.iomode),
 		&nfsi->pnfs_layout_state);
 	dprintk("%s: layout_get suspended until %ld\n",
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index e216e24..cebc958 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -105,7 +105,6 @@ struct pnfs_layout_type {
 	seqlock_t seqlock;		/* Protects the stateid */
 	nfs4_stateid stateid;
 	void *ld_data;			/* layout driver private data */
-	time_t pnfs_layout_suspend;
 	struct rpc_cred         *lo_cred; /* layoutcommit credential */
 	/* DH: These vars keep track of the maximum write range
 	 * so the values can be used for layoutcommit.
@@ -208,6 +207,7 @@ struct nfs_inode {
 	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop trying */
 	#define NFS_INO_LAYOUT_ALLOC     2 /* bit lock for layout allocation */
 	#define NFS_INO_LAYOUTCOMMIT     3 /* LAYOUTCOMMIT needed */
+	time_t pnfs_layout_suspend;
 #endif /* CONFIG_NFS_V4_1 */
 #endif /* CONFIG_NFS_V4*/
 #ifdef CONFIG_NFS_FSCACHE
-- 
1.6.6

[PATCH 4/5] SQUASHME pnfs-submit embed pnfs_layout_type

[andros]

From: Andy Adamson <andros@netapp.com>

Each layoutdriver embeds the pnfs_layout_type in it's private layout cache
head structure, and allocates them both with the alloc_layout
layoutdriver_io_operation.

Move all nfs inode pnfs field initialization into nfs4_init_once.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/callback_proc.c    |    2 +-
 fs/nfs/inode.c            |   43 ++++---------------------
 fs/nfs/nfs4proc.c         |    2 +-
 fs/nfs/nfs4state.c        |    4 +-
 fs/nfs/nfs4xdr.c          |    2 +-
 fs/nfs/pnfs.c             |   78 +++++++++++++++++++++++++--------------------
 include/linux/nfs4_pnfs.h |   14 ++------
 include/linux/nfs_fs.h    |    4 +-
 8 files changed, 61 insertions(+), 88 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index 4766695..d999ea8 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -233,7 +233,7 @@ static int pnfs_recall_layout(void *data)
 	rl.cbl_seg.length = NFS4_MAX_UINT64;
 
 	if (rl.cbl_recall_type == RETURN_FILE) {
-		if (pnfs_is_next_layout_stateid(&NFS_I(inode)->layout,
+		if (pnfs_is_next_layout_stateid(NFS_I(inode)->layout,
 						rl.cbl_stateid))
 			status = pnfs_return_layout(inode, &rl.cbl_seg,
 						    &rl.cbl_stateid, RETURN_FILE,
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index cb12d33..c290806 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1361,17 +1361,6 @@ void nfs4_clear_inode(struct inode *inode)
 }
 #endif
 
-static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
-{
-#ifdef CONFIG_NFS_V4_1
-	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
-	nfsi->layout.roc_iomode = 0;
-	nfsi->layout.lo_cred = NULL;
-	nfsi->layout.pnfs_write_begin_pos = 0;
-	nfsi->layout.pnfs_write_end_pos = 0;
-#endif /* CONFIG_NFS_V4_1 */
-}
-
 struct inode *nfs_alloc_inode(struct super_block *sb)
 {
 	struct nfs_inode *nfsi;
@@ -1387,23 +1376,14 @@ struct inode *nfs_alloc_inode(struct super_block *sb)
 #ifdef CONFIG_NFS_V4
 	nfsi->nfs4_acl = NULL;
 #endif /* CONFIG_NFS_V4 */
-	pnfs_alloc_init_inode(nfsi);
 	return &nfsi->vfs_inode;
 }
 
 static void pnfs_destroy_inode(struct nfs_inode *nfsi)
 {
 #ifdef CONFIG_NFS_V4_1
-	if (!list_empty(&nfsi->layout.segs))
+	if (has_layout(nfsi))
 		pnfs_destroy_layout(nfsi);
-
-	WARN_ON(!list_empty(&nfsi->layout.segs));
-	if (nfsi->layout.refcount)
-		printk("%s: WARNING: layout.refcount %d\n", __func__,
-			nfsi->layout.refcount);
-	WARN_ON(nfsi->layout.refcount);
-	WARN_ON(!list_empty(&nfsi->layout.lo_layouts));
-	WARN_ON(nfsi->layout.ld_data);
 #endif /* CONFIG_NFS_V4_1 */
 }
 
@@ -1415,20 +1395,6 @@ void nfs_destroy_inode(struct inode *inode)
 	kmem_cache_free(nfs_inode_cachep, nfsi);
 }
 
-static void pnfs_init_once(struct nfs_inode *nfsi)
-{
-#ifdef CONFIG_NFS_V4_1
-	init_waitqueue_head(&nfsi->lo_waitq);
-	nfsi->pnfs_layout_state = 0;
-	nfsi->pnfs_layout_suspend = 0;
-	seqlock_init(&nfsi->layout.seqlock);
-	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
-	INIT_LIST_HEAD(&nfsi->layout.segs);
-	nfsi->layout.refcount = 0;
-	nfsi->layout.ld_data = NULL;
-#endif /* CONFIG_NFS_V4_1 */
-}
-
 static inline void nfs4_init_once(struct nfs_inode *nfsi)
 {
 #ifdef CONFIG_NFS_V4
@@ -1436,6 +1402,12 @@ static inline void nfs4_init_once(struct nfs_inode *nfsi)
 	nfsi->delegation = NULL;
 	nfsi->delegation_state = 0;
 	init_rwsem(&nfsi->rwsem);
+#ifdef CONFIG_NFS_V4_1
+	init_waitqueue_head(&nfsi->lo_waitq);
+	nfsi->layout = NULL;
+	nfsi->pnfs_layout_state = 0;
+	nfsi->pnfs_layout_suspend = 0;
+#endif /* CONFIG_NFS_V4_1 */
 #endif
 }
 
@@ -1454,7 +1426,6 @@ static void init_once(void *foo)
 	INIT_HLIST_HEAD(&nfsi->silly_list);
 	init_waitqueue_head(&nfsi->waitqueue);
 	nfs4_init_once(nfsi);
-	pnfs_init_once(nfsi);
 }
 
 static int __init nfs_init_inodecache(void)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 6283996..61f4aa9 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1086,7 +1086,7 @@ static void pnfs4_layout_reclaim(struct nfs4_state *state)
 	 * flag during grace period
 	 */
 	pnfs_destroy_layout(NFS_I(state->inode));
-	pnfs_set_layout_stateid(&NFS_I(state->inode)->layout, &zero_stateid);
+	pnfs_set_layout_stateid(NFS_I(state->inode)->layout, &zero_stateid);
 #endif /* CONFIG_NFS_V4_1 */
 }
 
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index bfe679b..6f44cb0 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -597,10 +597,10 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state,
 #ifdef CONFIG_NFS_V4_1
 		struct nfs_inode *nfsi = NFS_I(state->inode);
 
-		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
+		if (has_layout(nfsi) && nfsi->layout->roc_iomode) {
 			struct nfs4_pnfs_layout_segment range;
 
-			range.iomode = nfsi->layout.roc_iomode;
+			range.iomode = nfsi->layout->roc_iomode;
 			range.offset = 0;
 			range.length = NFS4_MAX_UINT64;
 			pnfs_return_layout(state->inode, &range, NULL,
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index eeee855..0fd02b1 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1886,7 +1886,7 @@ encode_layoutreturn(struct xdr_stream *xdr,
 		ld_io_ops->encode_layoutreturn);
 		if (ld_io_ops->encode_layoutreturn) {
 			ld_io_ops->encode_layoutreturn(
-				&NFS_I(args->inode)->layout, xdr, args);
+				NFS_I(args->inode)->layout, xdr, args);
 		} else {
 			p = reserve_space(xdr, 4);
 			*p = cpu_to_be32(0);
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 7f6ace2..9275140 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -153,7 +153,7 @@ pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
 	dprintk("%s: has_layout=%d ctx=%p\n", __func__, has_layout(nfsi), ctx);
 	spin_lock(&nfsi->vfs_inode.i_lock);
 	if (has_layout(nfsi) && !layoutcommit_needed(nfsi)) {
-		nfsi->layout.lo_cred = get_rpccred(ctx->state->owner->so_cred);
+		nfsi->layout->lo_cred = get_rpccred(ctx->state->owner->so_cred);
 		__set_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->pnfs_layout_state);
 		nfsi->change_attr++;
 		spin_unlock(&nfsi->vfs_inode.i_lock);
@@ -174,17 +174,17 @@ pnfs_update_last_write(struct nfs_inode *nfsi, loff_t offset, size_t extent)
 	loff_t end_pos;
 
 	spin_lock(&nfsi->vfs_inode.i_lock);
-	if (offset < nfsi->layout.pnfs_write_begin_pos)
-		nfsi->layout.pnfs_write_begin_pos = offset;
+	if (offset < nfsi->layout->pnfs_write_begin_pos)
+		nfsi->layout->pnfs_write_begin_pos = offset;
 	end_pos = offset + extent - 1; /* I'm being inclusive */
-	if (end_pos > nfsi->layout.pnfs_write_end_pos)
-		nfsi->layout.pnfs_write_end_pos = end_pos;
+	if (end_pos > nfsi->layout->pnfs_write_end_pos)
+		nfsi->layout->pnfs_write_end_pos = end_pos;
 	dprintk("%s: Wrote %lu@%lu bpos %lu, epos: %lu\n",
 		__func__,
 		(unsigned long) extent,
 		(unsigned long) offset ,
-		(unsigned long) nfsi->layout.pnfs_write_begin_pos,
-		(unsigned long) nfsi->layout.pnfs_write_end_pos);
+		(unsigned long) nfsi->layout->pnfs_write_begin_pos,
+		(unsigned long) nfsi->layout->pnfs_write_end_pos);
 	spin_unlock(&nfsi->vfs_inode.i_lock);
 }
 
@@ -325,10 +325,9 @@ get_layout(struct pnfs_layout_type *lo)
 static inline struct pnfs_layout_type *
 grab_current_layout(struct nfs_inode *nfsi)
 {
-	struct pnfs_layout_type *lo = &nfsi->layout;
+	struct pnfs_layout_type *lo = nfsi->layout;
 
-	BUG_ON_UNLOCKED_LO(lo);
-	if (!lo->ld_data)
+	if (!lo)
 		return NULL;
 	get_layout(lo);
 	return lo;
@@ -342,13 +341,13 @@ put_layout(struct pnfs_layout_type *lo)
 
 	lo->refcount--;
 	if (!lo->refcount) {
-		struct layoutdriver_io_operations *io_ops =
-			PNFS_LD_IO_OPS(lo);
+		struct layoutdriver_io_operations *io_ops = PNFS_LD_IO_OPS(lo);
+		struct nfs_inode *nfsi = PNFS_NFS_INODE(lo);
 
-		dprintk("%s: freeing layout %p\n", __func__, lo->ld_data);
+		dprintk("%s: freeing layout cache %p\n", __func__, lo);
 		WARN_ON(!list_empty(&lo->lo_layouts));
-		io_ops->free_layout(lo->ld_data);
-		lo->ld_data = NULL;
+		io_ops->free_layout(lo);
+		nfsi->layout = NULL;
 	}
 }
 
@@ -688,7 +687,7 @@ pnfs_return_layout_barrier(struct nfs_inode *nfsi,
 	bool ret = false;
 
 	spin_lock(&nfsi->vfs_inode.i_lock);
-	list_for_each_entry (lseg, &nfsi->layout.segs, fi_list) {
+	list_for_each_entry(lseg, &nfsi->layout->segs, fi_list) {
 		if (!should_free_lseg(lseg, range))
 			continue;
 		lseg->valid = false;
@@ -894,17 +893,20 @@ pnfs_insert_layout(struct pnfs_layout_type *lo,
 	dprintk("%s:Return\n", __func__);
 }
 
+/*
+ * Each layoutdriver embeds pnfs_layout_type in it's per-layout type layout
+ * cache structure. Initialize the common pnfs_layout_type fields.
+ */
 static struct pnfs_layout_type *
 alloc_init_layout(struct inode *ino)
 {
-	void *ld_data;
 	struct pnfs_layout_type *lo;
 	struct layoutdriver_io_operations *io_ops;
 
 	io_ops = NFS_SERVER(ino)->pnfs_curr_ld->ld_io_ops;
-	lo = &NFS_I(ino)->layout;
-	ld_data = io_ops->alloc_layout(ino);
-	if (!ld_data) {
+	lo = NFS_I(ino)->layout;
+	lo = io_ops->alloc_layout(ino);
+	if (!lo) {
 		printk(KERN_ERR
 			"%s: out of memory: io_ops->alloc_layout failed\n",
 			__func__);
@@ -912,11 +914,17 @@ alloc_init_layout(struct inode *ino)
 	}
 
 	spin_lock(&ino->i_lock);
-	BUG_ON(lo->ld_data != NULL);
-	lo->ld_data = ld_data;
-	memset(&lo->stateid, 0, NFS4_STATEID_SIZE);
 	lo->refcount = 1;
+	INIT_LIST_HEAD(&lo->lo_layouts);
+	INIT_LIST_HEAD(&lo->segs);
 	lo->roc_iomode = 0;
+	seqlock_init(&lo->seqlock);
+	memset(&lo->stateid, 0, NFS4_STATEID_SIZE);
+	lo->lo_cred = NULL;
+	lo->pnfs_write_begin_pos = 0;
+	lo->pnfs_write_end_pos = 0;
+	lo->lo_inode = ino;
+	NFS_I(ino)->layout = lo;
 	spin_unlock(&ino->i_lock);
 	return lo;
 }
@@ -962,7 +970,7 @@ get_lock_alloc_layout(struct inode *ino)
 		/* Was current_layout already allocated while we slept?
 		 * If so, retry get_lock'ing it. Otherwise, allocate it.
 		 */
-		if (nfsi->layout.ld_data) {
+		if (nfsi->layout) {
 			spin_lock(&ino->i_lock);
 			continue;
 		}
@@ -1312,7 +1320,7 @@ pnfs_set_pg_test(struct inode *inode, struct nfs_pageio_descriptor *pgio)
 
 	pgio->pg_test = NULL;
 
-	laytype = &NFS_I(inode)->layout;
+	laytype = NFS_I(inode)->layout;
 	ld = NFS_SERVER(inode)->pnfs_curr_ld;
 	if (!pnfs_enabled_sb(NFS_SERVER(inode)) || !laytype)
 		return;
@@ -1445,7 +1453,7 @@ pnfs_writepages(struct nfs_write_data *wdata, int how)
 		numpages);
 	wdata->pdata.lseg = lseg;
 	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->write_pagelist(
-							&nfsi->layout,
+							nfsi->layout,
 							args->pages,
 							args->pgbase,
 							numpages,
@@ -1498,7 +1506,7 @@ pnfs_readpages(struct nfs_read_data *rdata)
 		__func__, numpages);
 	rdata->pdata.lseg = lseg;
 	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->read_pagelist(
-							&nfsi->layout,
+							nfsi->layout,
 							args->pages,
 							args->pgbase,
 							numpages,
@@ -1559,7 +1567,7 @@ pnfs_commit(struct nfs_write_data *data, int sync)
 	 * This will be done by passing the buck to the layout driver.
 	 */
 	data->pdata.lseg = NULL;
-	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->commit(&nfsi->layout,
+	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->commit(nfsi->layout,
 							sync, data);
 	dprintk("%s End (trypnfs:%d)\n", __func__, trypnfs);
 	return trypnfs;
@@ -1630,14 +1638,14 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
 	/* Clear layoutcommit properties in the inode so
 	 * new lc info can be generated
 	 */
-	write_begin_pos = nfsi->layout.pnfs_write_begin_pos;
-	write_end_pos = nfsi->layout.pnfs_write_end_pos;
-	data->cred = nfsi->layout.lo_cred;
-	nfsi->layout.pnfs_write_begin_pos = 0;
-	nfsi->layout.pnfs_write_end_pos = 0;
-	nfsi->layout.lo_cred = NULL;
+	write_begin_pos = nfsi->layout->pnfs_write_begin_pos;
+	write_end_pos = nfsi->layout->pnfs_write_end_pos;
+	data->cred = nfsi->layout->lo_cred;
+	nfsi->layout->pnfs_write_begin_pos = 0;
+	nfsi->layout->pnfs_write_end_pos = 0;
+	nfsi->layout->lo_cred = NULL;
 	__clear_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->pnfs_layout_state);
-	pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
+	pnfs_get_layout_stateid(&data->args.stateid, nfsi->layout);
 
 	spin_unlock(&inode->i_lock);
 
diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
index 4d9b15c..6e46589 100644
--- a/include/linux/nfs4_pnfs.h
+++ b/include/linux/nfs4_pnfs.h
@@ -35,13 +35,13 @@ struct pnfs_layoutdriver_type {
 static inline struct nfs_inode *
 PNFS_NFS_INODE(struct pnfs_layout_type *lo)
 {
-	return container_of(lo, struct nfs_inode, layout);
+	return NFS_I(lo->lo_inode);
 }
 
 static inline struct inode *
 PNFS_INODE(struct pnfs_layout_type *lo)
 {
-	return &PNFS_NFS_INODE(lo)->vfs_inode;
+	return lo->lo_inode;
 }
 
 static inline struct nfs_server *
@@ -50,12 +50,6 @@ PNFS_NFS_SERVER(struct pnfs_layout_type *lo)
 	return NFS_SERVER(PNFS_INODE(lo));
 }
 
-static inline void *
-PNFS_LD_DATA(struct pnfs_layout_type *lo)
-{
-	return lo->ld_data;
-}
-
 static inline struct pnfs_layoutdriver_type *
 PNFS_LD(struct pnfs_layout_type *lo)
 {
@@ -77,7 +71,7 @@ PNFS_LD_POLICY_OPS(struct pnfs_layout_type *lo)
 static inline bool
 has_layout(struct nfs_inode *nfsi)
 {
-	return nfsi->layout.ld_data != NULL;
+	return nfsi->layout != NULL;
 }
 
 static inline bool
@@ -149,7 +143,7 @@ struct layoutdriver_io_operations {
 	/* Layout information. For each inode, alloc_layout is executed once to retrieve an
 	 * inode specific layout structure.  Each subsequent layoutget operation results in
 	 * a set_layout call to set the opaque layout in the layout driver.*/
-	void * (*alloc_layout) (struct inode *inode);
+	struct pnfs_layout_type * (*alloc_layout) (struct inode *inode);
 	void (*free_layout) (void *layoutid);
 	struct pnfs_layout_segment * (*alloc_lseg) (struct pnfs_layout_type *layoutid, struct nfs4_pnfs_layoutget_res *lgr);
 	void (*free_lseg) (struct pnfs_layout_segment *lseg);
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index cebc958..43d516e 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -104,13 +104,13 @@ struct pnfs_layout_type {
 	int roc_iomode;			/* iomode to return on close, 0=none */
 	seqlock_t seqlock;		/* Protects the stateid */
 	nfs4_stateid stateid;
-	void *ld_data;			/* layout driver private data */
 	struct rpc_cred         *lo_cred; /* layoutcommit credential */
 	/* DH: These vars keep track of the maximum write range
 	 * so the values can be used for layoutcommit.
 	 */
 	loff_t                  pnfs_write_begin_pos;
 	loff_t                  pnfs_write_end_pos;
+	struct inode		*lo_inode;
 };
 
 /*
@@ -201,7 +201,7 @@ struct nfs_inode {
 	/* pNFS layout information */
 #if defined(CONFIG_NFS_V4_1)
 	wait_queue_head_t lo_waitq;
-	struct pnfs_layout_type layout;
+	struct pnfs_layout_type *layout;
 	unsigned long pnfs_layout_state;
 	#define NFS_INO_RO_LAYOUT_FAILED 0 /* ro LAYOUTGET failed stop trying */
 	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop trying */
-- 
1.6.6

[PATCH 5/5] SQUASHME pnfs-submit: filelayout: use new alloc_layout API

[andros]

From: Andy Adamson <andros@netapp.com>

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4filelayout.c |   11 +++++++----
 fs/nfs/nfs4filelayout.h |    7 +++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index 57a0010..07d5bf6 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -301,11 +301,14 @@ filelayout_write_pagelist(struct pnfs_layout_type *layoutid,
  * will use the pnfs_layout_type type to refer to the layout for this
  * inode from now on.
  */
-static void *
+static struct pnfs_layout_type *
 filelayout_alloc_layout(struct inode *inode)
 {
+	struct nfs4_filelayout *flp;
+
 	dprintk("NFS_FILELAYOUT: allocating layout\n");
-	return kzalloc(sizeof(struct nfs4_filelayout), GFP_KERNEL);
+	flp =  kzalloc(sizeof(struct nfs4_filelayout), GFP_KERNEL);
+	return flp ? &flp->fl_cache : NULL;
 }
 
 /* Free a filelayout layout structure
@@ -471,7 +474,7 @@ static struct pnfs_layout_segment *
 filelayout_alloc_lseg(struct pnfs_layout_type *layoutid,
 		      struct nfs4_pnfs_layoutget_res *lgr)
 {
-	struct nfs4_filelayout *flo = PNFS_LD_DATA(layoutid);
+	struct nfs4_filelayout *flo = FILE_LO_CACHE(layoutid);
 	struct pnfs_layout_segment *lseg;
 	int rc;
 
@@ -693,7 +696,7 @@ filelayout_commit(struct pnfs_layout_type *layoutid, int sync,
 ssize_t
 filelayout_get_stripesize(struct pnfs_layout_type *layoutid)
 {
-	struct nfs4_filelayout *flo = PNFS_LD_DATA(layoutid);
+	struct nfs4_filelayout *flo = FILE_LO_CACHE(layoutid);
 
 	return flo->stripe_unit;
 }
diff --git a/fs/nfs/nfs4filelayout.h b/fs/nfs/nfs4filelayout.h
index de8391f..eca459f 100644
--- a/fs/nfs/nfs4filelayout.h
+++ b/fs/nfs/nfs4filelayout.h
@@ -68,6 +68,7 @@ struct nfs4_filelayout_segment {
 };
 
 struct nfs4_filelayout {
+	struct pnfs_layout_type fl_cache;
 	int uncommitted_write;
 	loff_t last_commit_size;
 	u64 layout_id;
@@ -77,6 +78,12 @@ struct nfs4_filelayout {
 extern struct nfs_fh *
 nfs4_fl_select_ds_fh(struct pnfs_layout_segment *lseg, loff_t offset);
 
+static inline struct nfs4_filelayout *
+FILE_LO_CACHE(struct pnfs_layout_type *lo_cache)
+{
+	return container_of(lo_cache, struct nfs4_filelayout, fl_cache);
+}
+
 extern struct pnfs_client_operations *pnfs_callback_ops;
 
 extern void nfs4_fl_free_deviceid_callback(struct kref *);
-- 
1.6.6

Re: [PATCH 5/5] SQUASHME pnfs-submit: filelayout: use new alloc_layout API

[Boaz Harrosh]

On 06/29/2010 07:42 PM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/nfs4filelayout.c |   11 +++++++----
>  fs/nfs/nfs4filelayout.h |    7 +++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 

Won't you go head and submit an API change to the other two layout drivers?

> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> index 57a0010..07d5bf6 100644
> --- a/fs/nfs/nfs4filelayout.c
> +++ b/fs/nfs/nfs4filelayout.c
> @@ -301,11 +301,14 @@ filelayout_write_pagelist(struct pnfs_layout_type *layoutid,
>   * will use the pnfs_layout_type type to refer to the layout for this
>   * inode from now on.
>   */
> -static void *
> +static struct pnfs_layout_type *
>  filelayout_alloc_layout(struct inode *inode)
>  {
> +	struct nfs4_filelayout *flp;
> +
>  	dprintk("NFS_FILELAYOUT: allocating layout\n");
> -	return kzalloc(sizeof(struct nfs4_filelayout), GFP_KERNEL);
> +	flp =  kzalloc(sizeof(struct nfs4_filelayout), GFP_KERNEL);
> +	return flp ? &flp->fl_cache : NULL;
>  }
>  
>  /* Free a filelayout layout structure
> @@ -471,7 +474,7 @@ static struct pnfs_layout_segment *
>  filelayout_alloc_lseg(struct pnfs_layout_type *layoutid,
>  		      struct nfs4_pnfs_layoutget_res *lgr)
>  {
> -	struct nfs4_filelayout *flo = PNFS_LD_DATA(layoutid);
> +	struct nfs4_filelayout *flo = FILE_LO_CACHE(layoutid);
>  	struct pnfs_layout_segment *lseg;
>  	int rc;
>  
> @@ -693,7 +696,7 @@ filelayout_commit(struct pnfs_layout_type *layoutid, int sync,
>  ssize_t
>  filelayout_get_stripesize(struct pnfs_layout_type *layoutid)
>  {
> -	struct nfs4_filelayout *flo = PNFS_LD_DATA(layoutid);
> +	struct nfs4_filelayout *flo = FILE_LO_CACHE(layoutid);
>  
>  	return flo->stripe_unit;
>  }
> diff --git a/fs/nfs/nfs4filelayout.h b/fs/nfs/nfs4filelayout.h
> index de8391f..eca459f 100644
> --- a/fs/nfs/nfs4filelayout.h
> +++ b/fs/nfs/nfs4filelayout.h
> @@ -68,6 +68,7 @@ struct nfs4_filelayout_segment {
>  };
>  
>  struct nfs4_filelayout {
> +	struct pnfs_layout_type fl_cache;
>  	int uncommitted_write;
>  	loff_t last_commit_size;
>  	u64 layout_id;
> @@ -77,6 +78,12 @@ struct nfs4_filelayout {
>  extern struct nfs_fh *
>  nfs4_fl_select_ds_fh(struct pnfs_layout_segment *lseg, loff_t offset);
>  
> +static inline struct nfs4_filelayout *
> +FILE_LO_CACHE(struct pnfs_layout_type *lo_cache)
> +{
> +	return container_of(lo_cache, struct nfs4_filelayout, fl_cache);
> +}
> +

The name? why the __CACHE? why not just FILE_LO()? the struct it returns
name is nfs4_filelayout, simple.

Boaz

>  extern struct pnfs_client_operations *pnfs_callback_ops;
>  
>  extern void nfs4_fl_free_deviceid_callback(struct kref *);

Re: [PATCH 5/5] SQUASHME pnfs-submit: filelayout: use new alloc_layout API

[Andy Adamson]

On Jun 30, 2010, at 5:06 AM, Boaz Harrosh wrote:

> On 06/29/2010 07:42 PM, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> fs/nfs/nfs4filelayout.c |   11 +++++++----
>> fs/nfs/nfs4filelayout.h |    7 +++++++
>> 2 files changed, 14 insertions(+), 4 deletions(-)
>>
>
> Won't you go head and submit an API change to the other two layout  
> drivers?

OK. Just wanted comments first.

-->Andy

>
>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>> index 57a0010..07d5bf6 100644
>> --- a/fs/nfs/nfs4filelayout.c
>> +++ b/fs/nfs/nfs4filelayout.c
>> @@ -301,11 +301,14 @@ filelayout_write_pagelist(struct  
>> pnfs_layout_type *layoutid,
>>  * will use the pnfs_layout_type type to refer to the layout for this
>>  * inode from now on.
>>  */
>> -static void *
>> +static struct pnfs_layout_type *
>> filelayout_alloc_layout(struct inode *inode)
>> {
>> +	struct nfs4_filelayout *flp;
>> +
>> 	dprintk("NFS_FILELAYOUT: allocating layout\n");
>> -	return kzalloc(sizeof(struct nfs4_filelayout), GFP_KERNEL);
>> +	flp =  kzalloc(sizeof(struct nfs4_filelayout), GFP_KERNEL);
>> +	return flp ? &flp->fl_cache : NULL;
>> }
>>
>> /* Free a filelayout layout structure
>> @@ -471,7 +474,7 @@ static struct pnfs_layout_segment *
>> filelayout_alloc_lseg(struct pnfs_layout_type *layoutid,
>> 		      struct nfs4_pnfs_layoutget_res *lgr)
>> {
>> -	struct nfs4_filelayout *flo = PNFS_LD_DATA(layoutid);
>> +	struct nfs4_filelayout *flo = FILE_LO_CACHE(layoutid);
>> 	struct pnfs_layout_segment *lseg;
>> 	int rc;
>>
>> @@ -693,7 +696,7 @@ filelayout_commit(struct pnfs_layout_type  
>> *layoutid, int sync,
>> ssize_t
>> filelayout_get_stripesize(struct pnfs_layout_type *layoutid)
>> {
>> -	struct nfs4_filelayout *flo = PNFS_LD_DATA(layoutid);
>> +	struct nfs4_filelayout *flo = FILE_LO_CACHE(layoutid);
>>
>> 	return flo->stripe_unit;
>> }
>> diff --git a/fs/nfs/nfs4filelayout.h b/fs/nfs/nfs4filelayout.h
>> index de8391f..eca459f 100644
>> --- a/fs/nfs/nfs4filelayout.h
>> +++ b/fs/nfs/nfs4filelayout.h
>> @@ -68,6 +68,7 @@ struct nfs4_filelayout_segment {
>> };
>>
>> struct nfs4_filelayout {
>> +	struct pnfs_layout_type fl_cache;
>> 	int uncommitted_write;
>> 	loff_t last_commit_size;
>> 	u64 layout_id;
>> @@ -77,6 +78,12 @@ struct nfs4_filelayout {
>> extern struct nfs_fh *
>> nfs4_fl_select_ds_fh(struct pnfs_layout_segment *lseg, loff_t  
>> offset);
>>
>> +static inline struct nfs4_filelayout *
>> +FILE_LO_CACHE(struct pnfs_layout_type *lo_cache)
>> +{
>> +	return container_of(lo_cache, struct nfs4_filelayout, fl_cache);
>> +}
>> +
>
> The name? why the __CACHE? why not just FILE_LO()? the struct it  
> returns
> name is nfs4_filelayout, simple.
>
> Boaz
>
>> extern struct pnfs_client_operations *pnfs_callback_ops;
>>
>> extern void nfs4_fl_free_deviceid_callback(struct kref *);
>

Re: [PATCH 4/5] SQUASHME pnfs-submit embed pnfs_layout_type

[Boaz Harrosh]

On 06/29/2010 07:42 PM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Each layoutdriver embeds the pnfs_layout_type in it's private layout cache
> head structure, and allocates them both with the alloc_layout
> layoutdriver_io_operation.
> 
> Move all nfs inode pnfs field initialization into nfs4_init_once.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/callback_proc.c    |    2 +-
>  fs/nfs/inode.c            |   43 ++++---------------------
>  fs/nfs/nfs4proc.c         |    2 +-
>  fs/nfs/nfs4state.c        |    4 +-
>  fs/nfs/nfs4xdr.c          |    2 +-
>  fs/nfs/pnfs.c             |   78 +++++++++++++++++++++++++--------------------
>  include/linux/nfs4_pnfs.h |   14 ++------
>  include/linux/nfs_fs.h    |    4 +-
>  8 files changed, 61 insertions(+), 88 deletions(-)
> 
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index 4766695..d999ea8 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -233,7 +233,7 @@ static int pnfs_recall_layout(void *data)
>  	rl.cbl_seg.length = NFS4_MAX_UINT64;
>  
>  	if (rl.cbl_recall_type == RETURN_FILE) {
> -		if (pnfs_is_next_layout_stateid(&NFS_I(inode)->layout,
> +		if (pnfs_is_next_layout_stateid(NFS_I(inode)->layout,
>  						rl.cbl_stateid))
>  			status = pnfs_return_layout(inode, &rl.cbl_seg,
>  						    &rl.cbl_stateid, RETURN_FILE,
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index cb12d33..c290806 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1361,17 +1361,6 @@ void nfs4_clear_inode(struct inode *inode)
>  }
>  #endif
>  
> -static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
> -{
> -#ifdef CONFIG_NFS_V4_1
> -	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
> -	nfsi->layout.roc_iomode = 0;
> -	nfsi->layout.lo_cred = NULL;
> -	nfsi->layout.pnfs_write_begin_pos = 0;
> -	nfsi->layout.pnfs_write_end_pos = 0;
> -#endif /* CONFIG_NFS_V4_1 */
> -}
> -
>  struct inode *nfs_alloc_inode(struct super_block *sb)
>  {
>  	struct nfs_inode *nfsi;
> @@ -1387,23 +1376,14 @@ struct inode *nfs_alloc_inode(struct super_block *sb)
>  #ifdef CONFIG_NFS_V4
>  	nfsi->nfs4_acl = NULL;
>  #endif /* CONFIG_NFS_V4 */
> -	pnfs_alloc_init_inode(nfsi);
>  	return &nfsi->vfs_inode;
>  }
>  
>  static void pnfs_destroy_inode(struct nfs_inode *nfsi)
>  {
>  #ifdef CONFIG_NFS_V4_1
> -	if (!list_empty(&nfsi->layout.segs))
> +	if (has_layout(nfsi))
>  		pnfs_destroy_layout(nfsi);
> -
> -	WARN_ON(!list_empty(&nfsi->layout.segs));
> -	if (nfsi->layout.refcount)
> -		printk("%s: WARNING: layout.refcount %d\n", __func__,
> -			nfsi->layout.refcount);
> -	WARN_ON(nfsi->layout.refcount);
> -	WARN_ON(!list_empty(&nfsi->layout.lo_layouts));

Andy, these problems did not go away with this patch. Please re-instate
them for the new code. All 3 WARN_ONs still apply. Perhaps in pnfs_destroy_layout()

> -	WARN_ON(nfsi->layout.ld_data);
>  #endif /* CONFIG_NFS_V4_1 */
>  }
>  
> @@ -1415,20 +1395,6 @@ void nfs_destroy_inode(struct inode *inode)
>  	kmem_cache_free(nfs_inode_cachep, nfsi);
>  }
>  
> -static void pnfs_init_once(struct nfs_inode *nfsi)
> -{
> -#ifdef CONFIG_NFS_V4_1
> -	init_waitqueue_head(&nfsi->lo_waitq);
> -	nfsi->pnfs_layout_state = 0;
> -	nfsi->pnfs_layout_suspend = 0;
> -	seqlock_init(&nfsi->layout.seqlock);
> -	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
> -	INIT_LIST_HEAD(&nfsi->layout.segs);
> -	nfsi->layout.refcount = 0;
> -	nfsi->layout.ld_data = NULL;
> -#endif /* CONFIG_NFS_V4_1 */
> -}
> -
>  static inline void nfs4_init_once(struct nfs_inode *nfsi)
>  {
>  #ifdef CONFIG_NFS_V4
> @@ -1436,6 +1402,12 @@ static inline void nfs4_init_once(struct nfs_inode *nfsi)
>  	nfsi->delegation = NULL;
>  	nfsi->delegation_state = 0;
>  	init_rwsem(&nfsi->rwsem);
> +#ifdef CONFIG_NFS_V4_1
> +	init_waitqueue_head(&nfsi->lo_waitq);
> +	nfsi->layout = NULL;
> +	nfsi->pnfs_layout_state = 0;
> +	nfsi->pnfs_layout_suspend = 0;
> +#endif /* CONFIG_NFS_V4_1 */
>  #endif
>  }
>  
> @@ -1454,7 +1426,6 @@ static void init_once(void *foo)
>  	INIT_HLIST_HEAD(&nfsi->silly_list);
>  	init_waitqueue_head(&nfsi->waitqueue);
>  	nfs4_init_once(nfsi);
> -	pnfs_init_once(nfsi);
>  }
>  
>  static int __init nfs_init_inodecache(void)
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 6283996..61f4aa9 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1086,7 +1086,7 @@ static void pnfs4_layout_reclaim(struct nfs4_state *state)
>  	 * flag during grace period
>  	 */
>  	pnfs_destroy_layout(NFS_I(state->inode));
> -	pnfs_set_layout_stateid(&NFS_I(state->inode)->layout, &zero_stateid);
> +	pnfs_set_layout_stateid(NFS_I(state->inode)->layout, &zero_stateid);
>  #endif /* CONFIG_NFS_V4_1 */
>  }
>  
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index bfe679b..6f44cb0 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -597,10 +597,10 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state,
>  #ifdef CONFIG_NFS_V4_1
>  		struct nfs_inode *nfsi = NFS_I(state->inode);
>  
> -		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
> +		if (has_layout(nfsi) && nfsi->layout->roc_iomode) {
>  			struct nfs4_pnfs_layout_segment range;
>  
> -			range.iomode = nfsi->layout.roc_iomode;
> +			range.iomode = nfsi->layout->roc_iomode;
>  			range.offset = 0;
>  			range.length = NFS4_MAX_UINT64;
>  			pnfs_return_layout(state->inode, &range, NULL,
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index eeee855..0fd02b1 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -1886,7 +1886,7 @@ encode_layoutreturn(struct xdr_stream *xdr,
>  		ld_io_ops->encode_layoutreturn);
>  		if (ld_io_ops->encode_layoutreturn) {
>  			ld_io_ops->encode_layoutreturn(
> -				&NFS_I(args->inode)->layout, xdr, args);
> +				NFS_I(args->inode)->layout, xdr, args);
>  		} else {
>  			p = reserve_space(xdr, 4);
>  			*p = cpu_to_be32(0);
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 7f6ace2..9275140 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -153,7 +153,7 @@ pnfs_need_layoutcommit(struct nfs_inode *nfsi, struct nfs_open_context *ctx)
>  	dprintk("%s: has_layout=%d ctx=%p\n", __func__, has_layout(nfsi), ctx);
>  	spin_lock(&nfsi->vfs_inode.i_lock);
>  	if (has_layout(nfsi) && !layoutcommit_needed(nfsi)) {
> -		nfsi->layout.lo_cred = get_rpccred(ctx->state->owner->so_cred);
> +		nfsi->layout->lo_cred = get_rpccred(ctx->state->owner->so_cred);
>  		__set_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->pnfs_layout_state);
>  		nfsi->change_attr++;
>  		spin_unlock(&nfsi->vfs_inode.i_lock);
> @@ -174,17 +174,17 @@ pnfs_update_last_write(struct nfs_inode *nfsi, loff_t offset, size_t extent)
>  	loff_t end_pos;
>  
>  	spin_lock(&nfsi->vfs_inode.i_lock);
> -	if (offset < nfsi->layout.pnfs_write_begin_pos)
> -		nfsi->layout.pnfs_write_begin_pos = offset;
> +	if (offset < nfsi->layout->pnfs_write_begin_pos)
> +		nfsi->layout->pnfs_write_begin_pos = offset;
>  	end_pos = offset + extent - 1; /* I'm being inclusive */
> -	if (end_pos > nfsi->layout.pnfs_write_end_pos)
> -		nfsi->layout.pnfs_write_end_pos = end_pos;
> +	if (end_pos > nfsi->layout->pnfs_write_end_pos)
> +		nfsi->layout->pnfs_write_end_pos = end_pos;
>  	dprintk("%s: Wrote %lu@%lu bpos %lu, epos: %lu\n",
>  		__func__,
>  		(unsigned long) extent,
>  		(unsigned long) offset ,
> -		(unsigned long) nfsi->layout.pnfs_write_begin_pos,
> -		(unsigned long) nfsi->layout.pnfs_write_end_pos);
> +		(unsigned long) nfsi->layout->pnfs_write_begin_pos,
> +		(unsigned long) nfsi->layout->pnfs_write_end_pos);
>  	spin_unlock(&nfsi->vfs_inode.i_lock);
>  }
>  
> @@ -325,10 +325,9 @@ get_layout(struct pnfs_layout_type *lo)
>  static inline struct pnfs_layout_type *
>  grab_current_layout(struct nfs_inode *nfsi)
>  {
> -	struct pnfs_layout_type *lo = &nfsi->layout;
> +	struct pnfs_layout_type *lo = nfsi->layout;
>  
> -	BUG_ON_UNLOCKED_LO(lo);

Why did you drop this one?

> -	if (!lo->ld_data)
> +	if (!lo)
>  		return NULL;
>  	get_layout(lo);
>  	return lo;
> @@ -342,13 +341,13 @@ put_layout(struct pnfs_layout_type *lo)
>  
>  	lo->refcount--;
>  	if (!lo->refcount) {
> -		struct layoutdriver_io_operations *io_ops =
> -			PNFS_LD_IO_OPS(lo);
> +		struct layoutdriver_io_operations *io_ops = PNFS_LD_IO_OPS(lo);
> +		struct nfs_inode *nfsi = PNFS_NFS_INODE(lo);
>  
> -		dprintk("%s: freeing layout %p\n", __func__, lo->ld_data);
> +		dprintk("%s: freeing layout cache %p\n", __func__, lo);
>  		WARN_ON(!list_empty(&lo->lo_layouts));
> -		io_ops->free_layout(lo->ld_data);
> -		lo->ld_data = NULL;
> +		io_ops->free_layout(lo);
> +		nfsi->layout = NULL;
>  	}
>  }
>  
> @@ -688,7 +687,7 @@ pnfs_return_layout_barrier(struct nfs_inode *nfsi,
>  	bool ret = false;
>  
>  	spin_lock(&nfsi->vfs_inode.i_lock);
> -	list_for_each_entry (lseg, &nfsi->layout.segs, fi_list) {
> +	list_for_each_entry(lseg, &nfsi->layout->segs, fi_list) {

White space cleanup. actually I like the space. I thought checkpatch
was fixed to accept these spaces?

>  		if (!should_free_lseg(lseg, range))
>  			continue;
>  		lseg->valid = false;
> @@ -894,17 +893,20 @@ pnfs_insert_layout(struct pnfs_layout_type *lo,
>  	dprintk("%s:Return\n", __func__);
>  }
>  
> +/*
> + * Each layoutdriver embeds pnfs_layout_type in it's per-layout type layout
> + * cache structure. Initialize the common pnfs_layout_type fields.
> + */
>  static struct pnfs_layout_type *
>  alloc_init_layout(struct inode *ino)
>  {
> -	void *ld_data;
>  	struct pnfs_layout_type *lo;
>  	struct layoutdriver_io_operations *io_ops;
>  
>  	io_ops = NFS_SERVER(ino)->pnfs_curr_ld->ld_io_ops;
> -	lo = &NFS_I(ino)->layout;
> -	ld_data = io_ops->alloc_layout(ino);
> -	if (!ld_data) {
> +	lo = NFS_I(ino)->layout;
> +	lo = io_ops->alloc_layout(ino);

Oops, ;-)

> +	if (!lo) {
>  		printk(KERN_ERR
>  			"%s: out of memory: io_ops->alloc_layout failed\n",
>  			__func__);
> @@ -912,11 +914,17 @@ alloc_init_layout(struct inode *ino)
>  	}
>  
>  	spin_lock(&ino->i_lock);
> -	BUG_ON(lo->ld_data != NULL);

You could still do a BUG_ON(nfsi->layout) above before the io_ops->alloc_layout(ino)
call.

> -	lo->ld_data = ld_data;
> -	memset(&lo->stateid, 0, NFS4_STATEID_SIZE);
>  	lo->refcount = 1;
> +	INIT_LIST_HEAD(&lo->lo_layouts);
> +	INIT_LIST_HEAD(&lo->segs);
>  	lo->roc_iomode = 0;
> +	seqlock_init(&lo->seqlock);

All these below, I'd drop. The comment above io_ops->alloc_layout should
say that it must return a ZEROed out structure, which it is in your
files-layout patch.

> +	memset(&lo->stateid, 0, NFS4_STATEID_SIZE);
> +	lo->lo_cred = NULL;
> +	lo->pnfs_write_begin_pos = 0;
> +	lo->pnfs_write_end_pos = 0;

> +	lo->lo_inode = ino;
> +	NFS_I(ino)->layout = lo;
>  	spin_unlock(&ino->i_lock);

Just as a note:
  A pointer/word atomic inspection theory is based on the fact that the set is
  done with a memory barrier. Same as the test/set_bit operations. So here
  the spin_unlock provides that for us, naturally. (*Not* that the inode state will
  really need it, because no one will actually care before the allocation is done)

>  	return lo;
>  }
> @@ -962,7 +970,7 @@ get_lock_alloc_layout(struct inode *ino)
>  		/* Was current_layout already allocated while we slept?
>  		 * If so, retry get_lock'ing it. Otherwise, allocate it.
>  		 */
> -		if (nfsi->layout.ld_data) {
> +		if (nfsi->layout) {
>  			spin_lock(&ino->i_lock);
>  			continue;
>  		}
> @@ -1312,7 +1320,7 @@ pnfs_set_pg_test(struct inode *inode, struct nfs_pageio_descriptor *pgio)
>  
>  	pgio->pg_test = NULL;
>  
> -	laytype = &NFS_I(inode)->layout;
> +	laytype = NFS_I(inode)->layout;
>  	ld = NFS_SERVER(inode)->pnfs_curr_ld;
>  	if (!pnfs_enabled_sb(NFS_SERVER(inode)) || !laytype)
>  		return;
> @@ -1445,7 +1453,7 @@ pnfs_writepages(struct nfs_write_data *wdata, int how)
>  		numpages);
>  	wdata->pdata.lseg = lseg;
>  	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->write_pagelist(
> -							&nfsi->layout,
> +							nfsi->layout,
>  							args->pages,
>  							args->pgbase,
>  							numpages,
> @@ -1498,7 +1506,7 @@ pnfs_readpages(struct nfs_read_data *rdata)
>  		__func__, numpages);
>  	rdata->pdata.lseg = lseg;
>  	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->read_pagelist(
> -							&nfsi->layout,
> +							nfsi->layout,
>  							args->pages,
>  							args->pgbase,
>  							numpages,
> @@ -1559,7 +1567,7 @@ pnfs_commit(struct nfs_write_data *data, int sync)
>  	 * This will be done by passing the buck to the layout driver.
>  	 */
>  	data->pdata.lseg = NULL;
> -	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->commit(&nfsi->layout,
> +	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->commit(nfsi->layout,
>  							sync, data);
>  	dprintk("%s End (trypnfs:%d)\n", __func__, trypnfs);
>  	return trypnfs;
> @@ -1630,14 +1638,14 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
>  	/* Clear layoutcommit properties in the inode so
>  	 * new lc info can be generated
>  	 */
> -	write_begin_pos = nfsi->layout.pnfs_write_begin_pos;
> -	write_end_pos = nfsi->layout.pnfs_write_end_pos;
> -	data->cred = nfsi->layout.lo_cred;
> -	nfsi->layout.pnfs_write_begin_pos = 0;
> -	nfsi->layout.pnfs_write_end_pos = 0;
> -	nfsi->layout.lo_cred = NULL;
> +	write_begin_pos = nfsi->layout->pnfs_write_begin_pos;
> +	write_end_pos = nfsi->layout->pnfs_write_end_pos;
> +	data->cred = nfsi->layout->lo_cred;
> +	nfsi->layout->pnfs_write_begin_pos = 0;
> +	nfsi->layout->pnfs_write_end_pos = 0;
> +	nfsi->layout->lo_cred = NULL;
>  	__clear_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->pnfs_layout_state);
> -	pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
> +	pnfs_get_layout_stateid(&data->args.stateid, nfsi->layout);
>  
>  	spin_unlock(&inode->i_lock);
>  
> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
> index 4d9b15c..6e46589 100644
> --- a/include/linux/nfs4_pnfs.h
> +++ b/include/linux/nfs4_pnfs.h
> @@ -35,13 +35,13 @@ struct pnfs_layoutdriver_type {
>  static inline struct nfs_inode *
>  PNFS_NFS_INODE(struct pnfs_layout_type *lo)
>  {
> -	return container_of(lo, struct nfs_inode, layout);
> +	return NFS_I(lo->lo_inode);
>  }
>  
>  static inline struct inode *
>  PNFS_INODE(struct pnfs_layout_type *lo)
>  {
> -	return &PNFS_NFS_INODE(lo)->vfs_inode;
> +	return lo->lo_inode;
>  }
>  
>  static inline struct nfs_server *
> @@ -50,12 +50,6 @@ PNFS_NFS_SERVER(struct pnfs_layout_type *lo)
>  	return NFS_SERVER(PNFS_INODE(lo));
>  }
>  
> -static inline void *
> -PNFS_LD_DATA(struct pnfs_layout_type *lo)
> -{
> -	return lo->ld_data;
> -}
> -
>  static inline struct pnfs_layoutdriver_type *
>  PNFS_LD(struct pnfs_layout_type *lo)
>  {
> @@ -77,7 +71,7 @@ PNFS_LD_POLICY_OPS(struct pnfs_layout_type *lo)
>  static inline bool
>  has_layout(struct nfs_inode *nfsi)
>  {
> -	return nfsi->layout.ld_data != NULL;
> +	return nfsi->layout != NULL;
>  }
>  
>  static inline bool
> @@ -149,7 +143,7 @@ struct layoutdriver_io_operations {
>  	/* Layout information. For each inode, alloc_layout is executed once to retrieve an
>  	 * inode specific layout structure.  Each subsequent layoutget operation results in
>  	 * a set_layout call to set the opaque layout in the layout driver.*/
> -	void * (*alloc_layout) (struct inode *inode);
> +	struct pnfs_layout_type * (*alloc_layout) (struct inode *inode);
>  	void (*free_layout) (void *layoutid);
>  	struct pnfs_layout_segment * (*alloc_lseg) (struct pnfs_layout_type *layoutid, struct nfs4_pnfs_layoutget_res *lgr);
>  	void (*free_lseg) (struct pnfs_layout_segment *lseg);
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index cebc958..43d516e 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -104,13 +104,13 @@ struct pnfs_layout_type {
>  	int roc_iomode;			/* iomode to return on close, 0=none */
>  	seqlock_t seqlock;		/* Protects the stateid */
>  	nfs4_stateid stateid;
> -	void *ld_data;			/* layout driver private data */
>  	struct rpc_cred         *lo_cred; /* layoutcommit credential */
>  	/* DH: These vars keep track of the maximum write range
>  	 * so the values can be used for layoutcommit.
>  	 */
>  	loff_t                  pnfs_write_begin_pos;
>  	loff_t                  pnfs_write_end_pos;
> +	struct inode		*lo_inode;
>  };
>  
>  /*
> @@ -201,7 +201,7 @@ struct nfs_inode {
>  	/* pNFS layout information */
>  #if defined(CONFIG_NFS_V4_1)
>  	wait_queue_head_t lo_waitq;
> -	struct pnfs_layout_type layout;
> +	struct pnfs_layout_type *layout;
>  	unsigned long pnfs_layout_state;
>  	#define NFS_INO_RO_LAYOUT_FAILED 0 /* ro LAYOUTGET failed stop trying */
>  	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop trying */

Boaz

Re: [PATCH 4/5] SQUASHME pnfs-submit embed pnfs_layout_type

[Andy Adamson]

On Jun 30, 2010, at 6:02 AM, Boaz Harrosh wrote:

> On 06/29/2010 07:42 PM, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Each layoutdriver embeds the pnfs_layout_type in it's private  
>> layout cache
>> head structure, and allocates them both with the alloc_layout
>> layoutdriver_io_operation.
>>
>> Move all nfs inode pnfs field initialization into nfs4_init_once.
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> fs/nfs/callback_proc.c    |    2 +-
>> fs/nfs/inode.c            |   43 ++++---------------------
>> fs/nfs/nfs4proc.c         |    2 +-
>> fs/nfs/nfs4state.c        |    4 +-
>> fs/nfs/nfs4xdr.c          |    2 +-
>> fs/nfs/pnfs.c             |   78 ++++++++++++++++++++++++ 
>> +--------------------
>> include/linux/nfs4_pnfs.h |   14 ++------
>> include/linux/nfs_fs.h    |    4 +-
>> 8 files changed, 61 insertions(+), 88 deletions(-)
>>
>> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
>> index 4766695..d999ea8 100644
>> --- a/fs/nfs/callback_proc.c
>> +++ b/fs/nfs/callback_proc.c
>> @@ -233,7 +233,7 @@ static int pnfs_recall_layout(void *data)
>> 	rl.cbl_seg.length = NFS4_MAX_UINT64;
>>
>> 	if (rl.cbl_recall_type == RETURN_FILE) {
>> -		if (pnfs_is_next_layout_stateid(&NFS_I(inode)->layout,
>> +		if (pnfs_is_next_layout_stateid(NFS_I(inode)->layout,
>> 						rl.cbl_stateid))
>> 			status = pnfs_return_layout(inode, &rl.cbl_seg,
>> 						    &rl.cbl_stateid, RETURN_FILE,
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index cb12d33..c290806 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -1361,17 +1361,6 @@ void nfs4_clear_inode(struct inode *inode)
>> }
>> #endif
>>
>> -static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
>> -{
>> -#ifdef CONFIG_NFS_V4_1
>> -	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>> -	nfsi->layout.roc_iomode = 0;
>> -	nfsi->layout.lo_cred = NULL;
>> -	nfsi->layout.pnfs_write_begin_pos = 0;
>> -	nfsi->layout.pnfs_write_end_pos = 0;
>> -#endif /* CONFIG_NFS_V4_1 */
>> -}
>> -
>> struct inode *nfs_alloc_inode(struct super_block *sb)
>> {
>> 	struct nfs_inode *nfsi;
>> @@ -1387,23 +1376,14 @@ struct inode *nfs_alloc_inode(struct  
>> super_block *sb)
>> #ifdef CONFIG_NFS_V4
>> 	nfsi->nfs4_acl = NULL;
>> #endif /* CONFIG_NFS_V4 */
>> -	pnfs_alloc_init_inode(nfsi);
>> 	return &nfsi->vfs_inode;
>> }
>>
>> static void pnfs_destroy_inode(struct nfs_inode *nfsi)
>> {
>> #ifdef CONFIG_NFS_V4_1
>> -	if (!list_empty(&nfsi->layout.segs))
>> +	if (has_layout(nfsi))
>> 		pnfs_destroy_layout(nfsi);
>> -
>> -	WARN_ON(!list_empty(&nfsi->layout.segs));
>> -	if (nfsi->layout.refcount)
>> -		printk("%s: WARNING: layout.refcount %d\n", __func__,
>> -			nfsi->layout.refcount);
>> -	WARN_ON(nfsi->layout.refcount);
>> -	WARN_ON(!list_empty(&nfsi->layout.lo_layouts));
>
> Andy, these problems did not go away with this patch. Please re- 
> instate
> them for the new code. All 3 WARN_ONs still apply. Perhaps in  
> pnfs_destroy_layout()

OK - good idea.
>
>> -	WARN_ON(nfsi->layout.ld_data);
>> #endif /* CONFIG_NFS_V4_1 */
>> }
>>
>> @@ -1415,20 +1395,6 @@ void nfs_destroy_inode(struct inode *inode)
>> 	kmem_cache_free(nfs_inode_cachep, nfsi);
>> }
>>
>> -static void pnfs_init_once(struct nfs_inode *nfsi)
>> -{
>> -#ifdef CONFIG_NFS_V4_1
>> -	init_waitqueue_head(&nfsi->lo_waitq);
>> -	nfsi->pnfs_layout_state = 0;
>> -	nfsi->pnfs_layout_suspend = 0;
>> -	seqlock_init(&nfsi->layout.seqlock);
>> -	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
>> -	INIT_LIST_HEAD(&nfsi->layout.segs);
>> -	nfsi->layout.refcount = 0;
>> -	nfsi->layout.ld_data = NULL;
>> -#endif /* CONFIG_NFS_V4_1 */
>> -}
>> -
>> static inline void nfs4_init_once(struct nfs_inode *nfsi)
>> {
>> #ifdef CONFIG_NFS_V4
>> @@ -1436,6 +1402,12 @@ static inline void nfs4_init_once(struct  
>> nfs_inode *nfsi)
>> 	nfsi->delegation = NULL;
>> 	nfsi->delegation_state = 0;
>> 	init_rwsem(&nfsi->rwsem);
>> +#ifdef CONFIG_NFS_V4_1
>> +	init_waitqueue_head(&nfsi->lo_waitq);
>> +	nfsi->layout = NULL;
>> +	nfsi->pnfs_layout_state = 0;
>> +	nfsi->pnfs_layout_suspend = 0;
>> +#endif /* CONFIG_NFS_V4_1 */
>> #endif
>> }
>>
>> @@ -1454,7 +1426,6 @@ static void init_once(void *foo)
>> 	INIT_HLIST_HEAD(&nfsi->silly_list);
>> 	init_waitqueue_head(&nfsi->waitqueue);
>> 	nfs4_init_once(nfsi);
>> -	pnfs_init_once(nfsi);
>> }
>>
>> static int __init nfs_init_inodecache(void)
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 6283996..61f4aa9 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -1086,7 +1086,7 @@ static void pnfs4_layout_reclaim(struct  
>> nfs4_state *state)
>> 	 * flag during grace period
>> 	 */
>> 	pnfs_destroy_layout(NFS_I(state->inode));
>> -	pnfs_set_layout_stateid(&NFS_I(state->inode)->layout,  
>> &zero_stateid);
>> +	pnfs_set_layout_stateid(NFS_I(state->inode)->layout,  
>> &zero_stateid);
>> #endif /* CONFIG_NFS_V4_1 */
>> }
>>
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index bfe679b..6f44cb0 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -597,10 +597,10 @@ static void __nfs4_close(struct path *path,  
>> struct nfs4_state *state,
>> #ifdef CONFIG_NFS_V4_1
>> 		struct nfs_inode *nfsi = NFS_I(state->inode);
>>
>> -		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
>> +		if (has_layout(nfsi) && nfsi->layout->roc_iomode) {
>> 			struct nfs4_pnfs_layout_segment range;
>>
>> -			range.iomode = nfsi->layout.roc_iomode;
>> +			range.iomode = nfsi->layout->roc_iomode;
>> 			range.offset = 0;
>> 			range.length = NFS4_MAX_UINT64;
>> 			pnfs_return_layout(state->inode, &range, NULL,
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index eeee855..0fd02b1 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -1886,7 +1886,7 @@ encode_layoutreturn(struct xdr_stream *xdr,
>> 		ld_io_ops->encode_layoutreturn);
>> 		if (ld_io_ops->encode_layoutreturn) {
>> 			ld_io_ops->encode_layoutreturn(
>> -				&NFS_I(args->inode)->layout, xdr, args);
>> +				NFS_I(args->inode)->layout, xdr, args);
>> 		} else {
>> 			p = reserve_space(xdr, 4);
>> 			*p = cpu_to_be32(0);
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 7f6ace2..9275140 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -153,7 +153,7 @@ pnfs_need_layoutcommit(struct nfs_inode *nfsi,  
>> struct nfs_open_context *ctx)
>> 	dprintk("%s: has_layout=%d ctx=%p\n", __func__, has_layout(nfsi),  
>> ctx);
>> 	spin_lock(&nfsi->vfs_inode.i_lock);
>> 	if (has_layout(nfsi) && !layoutcommit_needed(nfsi)) {
>> -		nfsi->layout.lo_cred = get_rpccred(ctx->state->owner->so_cred);
>> +		nfsi->layout->lo_cred = get_rpccred(ctx->state->owner->so_cred);
>> 		__set_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->pnfs_layout_state);
>> 		nfsi->change_attr++;
>> 		spin_unlock(&nfsi->vfs_inode.i_lock);
>> @@ -174,17 +174,17 @@ pnfs_update_last_write(struct nfs_inode  
>> *nfsi, loff_t offset, size_t extent)
>> 	loff_t end_pos;
>>
>> 	spin_lock(&nfsi->vfs_inode.i_lock);
>> -	if (offset < nfsi->layout.pnfs_write_begin_pos)
>> -		nfsi->layout.pnfs_write_begin_pos = offset;
>> +	if (offset < nfsi->layout->pnfs_write_begin_pos)
>> +		nfsi->layout->pnfs_write_begin_pos = offset;
>> 	end_pos = offset + extent - 1; /* I'm being inclusive */
>> -	if (end_pos > nfsi->layout.pnfs_write_end_pos)
>> -		nfsi->layout.pnfs_write_end_pos = end_pos;
>> +	if (end_pos > nfsi->layout->pnfs_write_end_pos)
>> +		nfsi->layout->pnfs_write_end_pos = end_pos;
>> 	dprintk("%s: Wrote %lu@%lu bpos %lu, epos: %lu\n",
>> 		__func__,
>> 		(unsigned long) extent,
>> 		(unsigned long) offset ,
>> -		(unsigned long) nfsi->layout.pnfs_write_begin_pos,
>> -		(unsigned long) nfsi->layout.pnfs_write_end_pos);
>> +		(unsigned long) nfsi->layout->pnfs_write_begin_pos,
>> +		(unsigned long) nfsi->layout->pnfs_write_end_pos);
>> 	spin_unlock(&nfsi->vfs_inode.i_lock);
>> }
>>
>> @@ -325,10 +325,9 @@ get_layout(struct pnfs_layout_type *lo)
>> static inline struct pnfs_layout_type *
>> grab_current_layout(struct nfs_inode *nfsi)
>> {
>> -	struct pnfs_layout_type *lo = &nfsi->layout;
>> +	struct pnfs_layout_type *lo = nfsi->layout;
>>
>> -	BUG_ON_UNLOCKED_LO(lo);
>
> Why did you drop this one?

It is called in get_layout.

>
>> -	if (!lo->ld_data)
>> +	if (!lo)
>> 		return NULL;
>> 	get_layout(lo);
>> 	return lo;
>> @@ -342,13 +341,13 @@ put_layout(struct pnfs_layout_type *lo)
>>
>> 	lo->refcount--;
>> 	if (!lo->refcount) {
>> -		struct layoutdriver_io_operations *io_ops =
>> -			PNFS_LD_IO_OPS(lo);
>> +		struct layoutdriver_io_operations *io_ops = PNFS_LD_IO_OPS(lo);
>> +		struct nfs_inode *nfsi = PNFS_NFS_INODE(lo);
>>
>> -		dprintk("%s: freeing layout %p\n", __func__, lo->ld_data);
>> +		dprintk("%s: freeing layout cache %p\n", __func__, lo);
>> 		WARN_ON(!list_empty(&lo->lo_layouts));
>> -		io_ops->free_layout(lo->ld_data);
>> -		lo->ld_data = NULL;
>> +		io_ops->free_layout(lo);
>> +		nfsi->layout = NULL;
>> 	}
>> }
>>
>> @@ -688,7 +687,7 @@ pnfs_return_layout_barrier(struct nfs_inode  
>> *nfsi,
>> 	bool ret = false;
>>
>> 	spin_lock(&nfsi->vfs_inode.i_lock);
>> -	list_for_each_entry (lseg, &nfsi->layout.segs, fi_list) {
>> +	list_for_each_entry(lseg, &nfsi->layout->segs, fi_list) {
>
> White space cleanup. actually I like the space. I thought checkpatch
> was fixed to accept these spaces?

Nope - still there in Fedora 12.
>
>> 		if (!should_free_lseg(lseg, range))
>> 			continue;
>> 		lseg->valid = false;
>> @@ -894,17 +893,20 @@ pnfs_insert_layout(struct pnfs_layout_type *lo,
>> 	dprintk("%s:Return\n", __func__);
>> }
>>
>> +/*
>> + * Each layoutdriver embeds pnfs_layout_type in it's per-layout  
>> type layout
>> + * cache structure. Initialize the common pnfs_layout_type fields.
>> + */
>> static struct pnfs_layout_type *
>> alloc_init_layout(struct inode *ino)
>> {
>> -	void *ld_data;
>> 	struct pnfs_layout_type *lo;
>> 	struct layoutdriver_io_operations *io_ops;
>>
>> 	io_ops = NFS_SERVER(ino)->pnfs_curr_ld->ld_io_ops;
>> -	lo = &NFS_I(ino)->layout;
>> -	ld_data = io_ops->alloc_layout(ino);
>> -	if (!ld_data) {
>> +	lo = NFS_I(ino)->layout;
>> +	lo = io_ops->alloc_layout(ino);
>
> Oops, ;-)

Yeah... :)
>
>> +	if (!lo) {
>> 		printk(KERN_ERR
>> 			"%s: out of memory: io_ops->alloc_layout failed\n",
>> 			__func__);
>> @@ -912,11 +914,17 @@ alloc_init_layout(struct inode *ino)
>> 	}
>>
>> 	spin_lock(&ino->i_lock);
>> -	BUG_ON(lo->ld_data != NULL);
>
> You could still do a BUG_ON(nfsi->layout) above before the io_ops- 
> >alloc_layout(ino)
> call.

OK.

>
>> -	lo->ld_data = ld_data;
>> -	memset(&lo->stateid, 0, NFS4_STATEID_SIZE);
>> 	lo->refcount = 1;
>> +	INIT_LIST_HEAD(&lo->lo_layouts);
>> +	INIT_LIST_HEAD(&lo->segs);
>> 	lo->roc_iomode = 0;
>> +	seqlock_init(&lo->seqlock);
>
> All these below, I'd drop. The comment above io_ops->alloc_layout  
> should
> say that it must return a ZEROed out structure, which it is in your
> files-layout patch.

Alright.

-->Andy

>
>> +	memset(&lo->stateid, 0, NFS4_STATEID_SIZE);
>> +	lo->lo_cred = NULL;
>> +	lo->pnfs_write_begin_pos = 0;
>> +	lo->pnfs_write_end_pos = 0;
>
>> +	lo->lo_inode = ino;
>> +	NFS_I(ino)->layout = lo;
>> 	spin_unlock(&ino->i_lock);
>
> Just as a note:
>  A pointer/word atomic inspection theory is based on the fact that  
> the set is
>  done with a memory barrier. Same as the test/set_bit operations. So  
> here
>  the spin_unlock provides that for us, naturally. (*Not* that the  
> inode state will
>  really need it, because no one will actually care before the  
> allocation is done)
>
>> 	return lo;
>> }
>> @@ -962,7 +970,7 @@ get_lock_alloc_layout(struct inode *ino)
>> 		/* Was current_layout already allocated while we slept?
>> 		 * If so, retry get_lock'ing it. Otherwise, allocate it.
>> 		 */
>> -		if (nfsi->layout.ld_data) {
>> +		if (nfsi->layout) {
>> 			spin_lock(&ino->i_lock);
>> 			continue;
>> 		}
>> @@ -1312,7 +1320,7 @@ pnfs_set_pg_test(struct inode *inode, struct  
>> nfs_pageio_descriptor *pgio)
>>
>> 	pgio->pg_test = NULL;
>>
>> -	laytype = &NFS_I(inode)->layout;
>> +	laytype = NFS_I(inode)->layout;
>> 	ld = NFS_SERVER(inode)->pnfs_curr_ld;
>> 	if (!pnfs_enabled_sb(NFS_SERVER(inode)) || !laytype)
>> 		return;
>> @@ -1445,7 +1453,7 @@ pnfs_writepages(struct nfs_write_data *wdata,  
>> int how)
>> 		numpages);
>> 	wdata->pdata.lseg = lseg;
>> 	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->write_pagelist(
>> -							&nfsi->layout,
>> +							nfsi->layout,
>> 							args->pages,
>> 							args->pgbase,
>> 							numpages,
>> @@ -1498,7 +1506,7 @@ pnfs_readpages(struct nfs_read_data *rdata)
>> 		__func__, numpages);
>> 	rdata->pdata.lseg = lseg;
>> 	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->read_pagelist(
>> -							&nfsi->layout,
>> +							nfsi->layout,
>> 							args->pages,
>> 							args->pgbase,
>> 							numpages,
>> @@ -1559,7 +1567,7 @@ pnfs_commit(struct nfs_write_data *data, int  
>> sync)
>> 	 * This will be done by passing the buck to the layout driver.
>> 	 */
>> 	data->pdata.lseg = NULL;
>> -	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->commit(&nfsi->layout,
>> +	trypnfs = nfss->pnfs_curr_ld->ld_io_ops->commit(nfsi->layout,
>> 							sync, data);
>> 	dprintk("%s End (trypnfs:%d)\n", __func__, trypnfs);
>> 	return trypnfs;
>> @@ -1630,14 +1638,14 @@ pnfs_layoutcommit_inode(struct inode  
>> *inode, int sync)
>> 	/* Clear layoutcommit properties in the inode so
>> 	 * new lc info can be generated
>> 	 */
>> -	write_begin_pos = nfsi->layout.pnfs_write_begin_pos;
>> -	write_end_pos = nfsi->layout.pnfs_write_end_pos;
>> -	data->cred = nfsi->layout.lo_cred;
>> -	nfsi->layout.pnfs_write_begin_pos = 0;
>> -	nfsi->layout.pnfs_write_end_pos = 0;
>> -	nfsi->layout.lo_cred = NULL;
>> +	write_begin_pos = nfsi->layout->pnfs_write_begin_pos;
>> +	write_end_pos = nfsi->layout->pnfs_write_end_pos;
>> +	data->cred = nfsi->layout->lo_cred;
>> +	nfsi->layout->pnfs_write_begin_pos = 0;
>> +	nfsi->layout->pnfs_write_end_pos = 0;
>> +	nfsi->layout->lo_cred = NULL;
>> 	__clear_bit(NFS_INO_LAYOUTCOMMIT, &nfsi->pnfs_layout_state);
>> -	pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
>> +	pnfs_get_layout_stateid(&data->args.stateid, nfsi->layout);
>>
>> 	spin_unlock(&inode->i_lock);
>>
>> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
>> index 4d9b15c..6e46589 100644
>> --- a/include/linux/nfs4_pnfs.h
>> +++ b/include/linux/nfs4_pnfs.h
>> @@ -35,13 +35,13 @@ struct pnfs_layoutdriver_type {
>> static inline struct nfs_inode *
>> PNFS_NFS_INODE(struct pnfs_layout_type *lo)
>> {
>> -	return container_of(lo, struct nfs_inode, layout);
>> +	return NFS_I(lo->lo_inode);
>> }
>>
>> static inline struct inode *
>> PNFS_INODE(struct pnfs_layout_type *lo)
>> {
>> -	return &PNFS_NFS_INODE(lo)->vfs_inode;
>> +	return lo->lo_inode;
>> }
>>
>> static inline struct nfs_server *
>> @@ -50,12 +50,6 @@ PNFS_NFS_SERVER(struct pnfs_layout_type *lo)
>> 	return NFS_SERVER(PNFS_INODE(lo));
>> }
>>
>> -static inline void *
>> -PNFS_LD_DATA(struct pnfs_layout_type *lo)
>> -{
>> -	return lo->ld_data;
>> -}
>> -
>> static inline struct pnfs_layoutdriver_type *
>> PNFS_LD(struct pnfs_layout_type *lo)
>> {
>> @@ -77,7 +71,7 @@ PNFS_LD_POLICY_OPS(struct pnfs_layout_type *lo)
>> static inline bool
>> has_layout(struct nfs_inode *nfsi)
>> {
>> -	return nfsi->layout.ld_data != NULL;
>> +	return nfsi->layout != NULL;
>> }
>>
>> static inline bool
>> @@ -149,7 +143,7 @@ struct layoutdriver_io_operations {
>> 	/* Layout information. For each inode, alloc_layout is executed  
>> once to retrieve an
>> 	 * inode specific layout structure.  Each subsequent layoutget  
>> operation results in
>> 	 * a set_layout call to set the opaque layout in the layout  
>> driver.*/
>> -	void * (*alloc_layout) (struct inode *inode);
>> +	struct pnfs_layout_type * (*alloc_layout) (struct inode *inode);
>> 	void (*free_layout) (void *layoutid);
>> 	struct pnfs_layout_segment * (*alloc_lseg) (struct  
>> pnfs_layout_type *layoutid, struct nfs4_pnfs_layoutget_res *lgr);
>> 	void (*free_lseg) (struct pnfs_layout_segment *lseg);
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index cebc958..43d516e 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -104,13 +104,13 @@ struct pnfs_layout_type {
>> 	int roc_iomode;			/* iomode to return on close, 0=none */
>> 	seqlock_t seqlock;		/* Protects the stateid */
>> 	nfs4_stateid stateid;
>> -	void *ld_data;			/* layout driver private data */
>> 	struct rpc_cred         *lo_cred; /* layoutcommit credential */
>> 	/* DH: These vars keep track of the maximum write range
>> 	 * so the values can be used for layoutcommit.
>> 	 */
>> 	loff_t                  pnfs_write_begin_pos;
>> 	loff_t                  pnfs_write_end_pos;
>> +	struct inode		*lo_inode;
>> };
>>
>> /*
>> @@ -201,7 +201,7 @@ struct nfs_inode {
>> 	/* pNFS layout information */
>> #if defined(CONFIG_NFS_V4_1)
>> 	wait_queue_head_t lo_waitq;
>> -	struct pnfs_layout_type layout;
>> +	struct pnfs_layout_type *layout;
>> 	unsigned long pnfs_layout_state;
>> 	#define NFS_INO_RO_LAYOUT_FAILED 0 /* ro LAYOUTGET failed stop  
>> trying */
>> 	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop  
>> trying */
>
> Boaz

Re: [PATCH 3/5] SQUASHME: pnfs-submit: move pnfs_layout_suspend back to nfs_inode

[Boaz Harrosh]

On 06/29/2010 07:42 PM, andros@netapp.com wrote:
> From: Fred Isaman <iisaman@netapp.com>
> 
> Prepare to embed struct pnfs_layout_tupe into layout driver private structs
> and to make layout a pointer. Since a temp error on first LAYOUTGET
> erases the layout, the suspend timer needs to be moved.
> 

OK Fred, Andy. (I'm also referring to the 1/5 and 2/5 issues here)

There is a total mess up. LAYOUTGET has nothing to do with layout
(.eg struct pnfs_layout_type) and the layout must not be deallocated
if there is any kind of error and/or if IO is actually to be done with
MDS or not. LAYOUTGET corresponds to layout_seg held on a list in layout.
The list might be empty and so it's flags and state.

Half of the problem was before and this here made it worse. The
life time of nfsi->layout should be the same as nfsi itself just
as it was before.

Once the life_time of nfsi && nfsi->layout are unified then all your
problems go away because you are not checking for existence of nfsi
anywhere right? that's because there is no such problem.

Look at it this way. If a layout_driver is in the game it gets to
allocate it's own area in NFS_I. part of that area is for generic
pnfs.c use, i.e struct pnfs_layout_type, the rest is for private use.
once existing it is part of the NFS_I life up till death.

If it's not so it should be fixed, not some members leaking out
to generic structures.

Boaz

> Signed-off-by: Fred Isaman <iisaman@netapp.com>
> ---
>  fs/nfs/inode.c         |    1 +
>  fs/nfs/pnfs.c          |    8 ++++----
>  include/linux/nfs_fs.h |    2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 66d7be2..cb12d33 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1420,6 +1420,7 @@ static void pnfs_init_once(struct nfs_inode *nfsi)
>  #ifdef CONFIG_NFS_V4_1
>  	init_waitqueue_head(&nfsi->lo_waitq);
>  	nfsi->pnfs_layout_state = 0;
> +	nfsi->pnfs_layout_suspend = 0;
>  	seqlock_init(&nfsi->layout.seqlock);
>  	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
>  	INIT_LIST_HEAD(&nfsi->layout.segs);
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index d05cb03..7f6ace2 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -1105,12 +1105,12 @@ _pnfs_update_layout(struct inode *ino,
>  
>  	/* if get layout already failed once goto out */
>  	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
> -		if (unlikely(nfsi->layout.pnfs_layout_suspend &&
> -		    get_seconds() >= nfsi->layout.pnfs_layout_suspend)) {
> +		if (unlikely(nfsi->pnfs_layout_suspend &&
> +		    get_seconds() >= nfsi->pnfs_layout_suspend)) {
>  			dprintk("%s: layout_get resumed\n", __func__);
>  			clear_bit(lo_fail_bit(iomode),
>  				  &nfsi->pnfs_layout_state);
> -			nfsi->layout.pnfs_layout_suspend = 0;
> +			nfsi->pnfs_layout_suspend = 0;
>  		} else
>  			goto out_put;
>  	}
> @@ -1224,7 +1224,7 @@ pnfs_get_layout_done(struct nfs4_pnfs_layoutget *lgp, int rpc_status)
>  	}
>  
>  	/* remember that get layout failed and suspend trying */
> -	nfsi->layout.pnfs_layout_suspend = suspend;
> +	nfsi->pnfs_layout_suspend = suspend;
>  	set_bit(lo_fail_bit(lgp->args.lseg.iomode),
>  		&nfsi->pnfs_layout_state);
>  	dprintk("%s: layout_get suspended until %ld\n",
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index e216e24..cebc958 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -105,7 +105,6 @@ struct pnfs_layout_type {
>  	seqlock_t seqlock;		/* Protects the stateid */
>  	nfs4_stateid stateid;
>  	void *ld_data;			/* layout driver private data */
> -	time_t pnfs_layout_suspend;
>  	struct rpc_cred         *lo_cred; /* layoutcommit credential */
>  	/* DH: These vars keep track of the maximum write range
>  	 * so the values can be used for layoutcommit.
> @@ -208,6 +207,7 @@ struct nfs_inode {
>  	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop trying */
>  	#define NFS_INO_LAYOUT_ALLOC     2 /* bit lock for layout allocation */
>  	#define NFS_INO_LAYOUTCOMMIT     3 /* LAYOUTCOMMIT needed */
> +	time_t pnfs_layout_suspend;
>  #endif /* CONFIG_NFS_V4_1 */
>  #endif /* CONFIG_NFS_V4*/
>  #ifdef CONFIG_NFS_FSCACHE

Re: [PATCH 3/5] SQUASHME: pnfs-submit: move pnfs_layout_suspend back to nfs_inode

[Andy Adamson]

On Jun 30, 2010, at 10:49 AM, Boaz Harrosh wrote:

> On 06/29/2010 07:42 PM, andros@netapp.com wrote:
>> From: Fred Isaman <iisaman@netapp.com>
>>
>> Prepare to embed struct pnfs_layout_tupe into layout driver private  
>> structs
>> and to make layout a pointer. Since a temp error on first LAYOUTGET
>> erases the layout, the suspend timer needs to be moved.
>>
>
> OK Fred, Andy. (I'm also referring to the 1/5 and 2/5 issues here)
>
> There is a total mess up. LAYOUTGET has nothing to do with layout
> (.eg struct pnfs_layout_type) and the layout must not be deallocated
> if there is any kind of error and/or if IO is actually to be done with
> MDS or not. LAYOUTGET corresponds to layout_seg held on a list in  
> layout.
> The list might be empty and so it's flags and state.

pnfs_layout_type is the layout cache head structure which has no  
business in nfsi unless there are layout segments populating it.

>
> Half of the problem was before and this here made it worse. The
> life time of nfsi->layout should be the same as nfsi itself just
> as it was before.

No, the nfs_inode exists with no regard to pnfs_layout_type (horrible  
name).

>
> Once the life_time of nfsi && nfsi->layout are unified then all your
> problems go away because you are not checking for existence of nfsi
> anywhere right? that's because there is no such problem.

No, the problems don't go away simply because you statically allocate  
a cache head struct. We have problems with races and reference  
counting that exist independently of how a structure is allocated.

We don't need to check for the existence of nfsi, The inode is  
guaranteed to exist until nfs_destroy_inode is called by the VFS.

>
> Look at it this way. If a layout_driver is in the game it gets to
> allocate it's own area in NFS_I. part of that area is for generic
> pnfs.c use, i.e struct pnfs_layout_type, the rest is for private use.
> once existing it is part of the NFS_I life up till death.

No. It should only exists when needed - just like nfsi->nfs_acl and  
nfsi->delegation etc.

>
> If it's not so it should be fixed, not some members leaking out
> to generic structures.
>
> Boaz
>
>> Signed-off-by: Fred Isaman <iisaman@netapp.com>
>> ---
>> fs/nfs/inode.c         |    1 +
>> fs/nfs/pnfs.c          |    8 ++++----
>> include/linux/nfs_fs.h |    2 +-
>> 3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index 66d7be2..cb12d33 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -1420,6 +1420,7 @@ static void pnfs_init_once(struct nfs_inode  
>> *nfsi)
>> #ifdef CONFIG_NFS_V4_1
>> 	init_waitqueue_head(&nfsi->lo_waitq);
>> 	nfsi->pnfs_layout_state = 0;
>> +	nfsi->pnfs_layout_suspend = 0;
>> 	seqlock_init(&nfsi->layout.seqlock);
>> 	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
>> 	INIT_LIST_HEAD(&nfsi->layout.segs);
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index d05cb03..7f6ace2 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -1105,12 +1105,12 @@ _pnfs_update_layout(struct inode *ino,
>>
>> 	/* if get layout already failed once goto out */
>> 	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
>> -		if (unlikely(nfsi->layout.pnfs_layout_suspend &&
>> -		    get_seconds() >= nfsi->layout.pnfs_layout_suspend)) {
>> +		if (unlikely(nfsi->pnfs_layout_suspend &&
>> +		    get_seconds() >= nfsi->pnfs_layout_suspend)) {
>> 			dprintk("%s: layout_get resumed\n", __func__);
>> 			clear_bit(lo_fail_bit(iomode),
>> 				  &nfsi->pnfs_layout_state);
>> -			nfsi->layout.pnfs_layout_suspend = 0;
>> +			nfsi->pnfs_layout_suspend = 0;
>> 		} else
>> 			goto out_put;
>> 	}
>> @@ -1224,7 +1224,7 @@ pnfs_get_layout_done(struct  
>> nfs4_pnfs_layoutget *lgp, int rpc_status)
>> 	}
>>
>> 	/* remember that get layout failed and suspend trying */
>> -	nfsi->layout.pnfs_layout_suspend = suspend;
>> +	nfsi->pnfs_layout_suspend = suspend;
>> 	set_bit(lo_fail_bit(lgp->args.lseg.iomode),
>> 		&nfsi->pnfs_layout_state);
>> 	dprintk("%s: layout_get suspended until %ld\n",
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index e216e24..cebc958 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -105,7 +105,6 @@ struct pnfs_layout_type {
>> 	seqlock_t seqlock;		/* Protects the stateid */
>> 	nfs4_stateid stateid;
>> 	void *ld_data;			/* layout driver private data */
>> -	time_t pnfs_layout_suspend;
>> 	struct rpc_cred         *lo_cred; /* layoutcommit credential */
>> 	/* DH: These vars keep track of the maximum write range
>> 	 * so the values can be used for layoutcommit.
>> @@ -208,6 +207,7 @@ struct nfs_inode {
>> 	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop  
>> trying */
>> 	#define NFS_INO_LAYOUT_ALLOC     2 /* bit lock for layout  
>> allocation */
>> 	#define NFS_INO_LAYOUTCOMMIT     3 /* LAYOUTCOMMIT needed */
>> +	time_t pnfs_layout_suspend;
>> #endif /* CONFIG_NFS_V4_1 */
>> #endif /* CONFIG_NFS_V4*/
>> #ifdef CONFIG_NFS_FSCACHE
>

Re: [PATCH 3/5] SQUASHME: pnfs-submit: move pnfs_layout_suspend back to nfs_inode

[Boaz Harrosh]

On 06/30/2010 06:13 PM, Andy Adamson wrote:
> 
> On Jun 30, 2010, at 10:49 AM, Boaz Harrosh wrote:
> 
>> On 06/29/2010 07:42 PM, andros@netapp.com wrote:
>>> From: Fred Isaman <iisaman@netapp.com>
>>>
>>> Prepare to embed struct pnfs_layout_tupe into layout driver private  
>>> structs
>>> and to make layout a pointer. Since a temp error on first LAYOUTGET
>>> erases the layout, the suspend timer needs to be moved.
>>>
>>
>> OK Fred, Andy. (I'm also referring to the 1/5 and 2/5 issues here)
>>
>> There is a total mess up. LAYOUTGET has nothing to do with layout
>> (.eg struct pnfs_layout_type) and the layout must not be deallocated
>> if there is any kind of error and/or if IO is actually to be done with
>> MDS or not. LAYOUTGET corresponds to layout_seg held on a list in  
>> layout.
>> The list might be empty and so it's flags and state.
> 
> pnfs_layout_type is the layout cache head structure which has no  

Why are you calling this "cache" head? Are you referring to the
segments list .ie layout->lo_layouts. For me it's a list head
why is it a cache ?

> business in nfsi unless there are layout segments populating it.
> 

This is where we disagree. You say layout is lo_layouts. I say
it is more, it is that plus the state and additional information.
An empty list is not a none-existent list. There are two states
here.

>>
>> Half of the problem was before and this here made it worse. The
>> life time of nfsi->layout should be the same as nfsi itself just
>> as it was before.
> 
> No, the nfs_inode exists with no regard to pnfs_layout_type (horrible  
> name).
> 

Yes

>>
>> Once the life_time of nfsi && nfsi->layout are unified then all your
>> problems go away because you are not checking for existence of nfsi
>> anywhere right? that's because there is no such problem.
> 
> No, the problems don't go away simply because you statically allocate  
> a cache head struct. We have problems with races and reference  
> counting that exist independently of how a structure is allocated.
> 
> We don't need to check for the existence of nfsi, The inode is  
> guaranteed to exist until nfs_destroy_inode is called by the VFS.
> 

And so if you restrict the life time of layout with nfsi your problem
will go away as well, no?

>>
>> Look at it this way. If a layout_driver is in the game it gets to
>> allocate it's own area in NFS_I. part of that area is for generic
>> pnfs.c use, i.e struct pnfs_layout_type, the rest is for private use.
>> once existing it is part of the NFS_I life up till death.
> 
> No. It should only exists when needed - just like nfsi->nfs_acl and  
> nfsi->delegation etc.
> 

It is the same problem with what we had with commit. We had it all over
the place and had races refcounting and shit for ever, until we simplified
it and moved it to the proper NFS checkpoint.

Here too we can just do much better and drop lots of accounting by saying:

  If this nfsi is eligible for pnfs_io and layouts. We allocate a layout
  governing structure for it. That will be freed at the end together with
  nfsi. These that never need it like directories links and so on will not
  have one.

  Now if that first layout_get() never gets through, well nu, we only used
  it to say "Don't have any".

Don't you think that is a much simpler model. Why should one shoot one's foot?

Boaz

>>
>> If it's not so it should be fixed, not some members leaking out
>> to generic structures.
>>
>> Boaz
>>
>>> Signed-off-by: Fred Isaman <iisaman@netapp.com>
>>> ---
>>> fs/nfs/inode.c         |    1 +
>>> fs/nfs/pnfs.c          |    8 ++++----
>>> include/linux/nfs_fs.h |    2 +-
>>> 3 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>>> index 66d7be2..cb12d33 100644
>>> --- a/fs/nfs/inode.c
>>> +++ b/fs/nfs/inode.c
>>> @@ -1420,6 +1420,7 @@ static void pnfs_init_once(struct nfs_inode  
>>> *nfsi)
>>> #ifdef CONFIG_NFS_V4_1
>>> 	init_waitqueue_head(&nfsi->lo_waitq);
>>> 	nfsi->pnfs_layout_state = 0;
>>> +	nfsi->pnfs_layout_suspend = 0;
>>> 	seqlock_init(&nfsi->layout.seqlock);
>>> 	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
>>> 	INIT_LIST_HEAD(&nfsi->layout.segs);
>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>> index d05cb03..7f6ace2 100644
>>> --- a/fs/nfs/pnfs.c
>>> +++ b/fs/nfs/pnfs.c
>>> @@ -1105,12 +1105,12 @@ _pnfs_update_layout(struct inode *ino,
>>>
>>> 	/* if get layout already failed once goto out */
>>> 	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
>>> -		if (unlikely(nfsi->layout.pnfs_layout_suspend &&
>>> -		    get_seconds() >= nfsi->layout.pnfs_layout_suspend)) {
>>> +		if (unlikely(nfsi->pnfs_layout_suspend &&
>>> +		    get_seconds() >= nfsi->pnfs_layout_suspend)) {
>>> 			dprintk("%s: layout_get resumed\n", __func__);
>>> 			clear_bit(lo_fail_bit(iomode),
>>> 				  &nfsi->pnfs_layout_state);
>>> -			nfsi->layout.pnfs_layout_suspend = 0;
>>> +			nfsi->pnfs_layout_suspend = 0;
>>> 		} else
>>> 			goto out_put;
>>> 	}
>>> @@ -1224,7 +1224,7 @@ pnfs_get_layout_done(struct  
>>> nfs4_pnfs_layoutget *lgp, int rpc_status)
>>> 	}
>>>
>>> 	/* remember that get layout failed and suspend trying */
>>> -	nfsi->layout.pnfs_layout_suspend = suspend;
>>> +	nfsi->pnfs_layout_suspend = suspend;
>>> 	set_bit(lo_fail_bit(lgp->args.lseg.iomode),
>>> 		&nfsi->pnfs_layout_state);
>>> 	dprintk("%s: layout_get suspended until %ld\n",
>>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>>> index e216e24..cebc958 100644
>>> --- a/include/linux/nfs_fs.h
>>> +++ b/include/linux/nfs_fs.h
>>> @@ -105,7 +105,6 @@ struct pnfs_layout_type {
>>> 	seqlock_t seqlock;		/* Protects the stateid */
>>> 	nfs4_stateid stateid;
>>> 	void *ld_data;			/* layout driver private data */
>>> -	time_t pnfs_layout_suspend;
>>> 	struct rpc_cred         *lo_cred; /* layoutcommit credential */
>>> 	/* DH: These vars keep track of the maximum write range
>>> 	 * so the values can be used for layoutcommit.
>>> @@ -208,6 +207,7 @@ struct nfs_inode {
>>> 	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop  
>>> trying */
>>> 	#define NFS_INO_LAYOUT_ALLOC     2 /* bit lock for layout  
>>> allocation */
>>> 	#define NFS_INO_LAYOUTCOMMIT     3 /* LAYOUTCOMMIT needed */
>>> +	time_t pnfs_layout_suspend;
>>> #endif /* CONFIG_NFS_V4_1 */
>>> #endif /* CONFIG_NFS_V4*/
>>> #ifdef CONFIG_NFS_FSCACHE
>>
> 

Re: [PATCH 3/5] SQUASHME: pnfs-submit: move pnfs_layout_suspend back to nfs_inode

[Andy Adamson]

On Jun 30, 2010, at 11:56 AM, Boaz Harrosh wrote:

> On 06/30/2010 06:13 PM, Andy Adamson wrote:
>>
>> On Jun 30, 2010, at 10:49 AM, Boaz Harrosh wrote:
>>
>>> On 06/29/2010 07:42 PM, andros@netapp.com wrote:
>>>> From: Fred Isaman <iisaman@netapp.com>
>>>>
>>>> Prepare to embed struct pnfs_layout_tupe into layout driver private
>>>> structs
>>>> and to make layout a pointer. Since a temp error on first LAYOUTGET
>>>> erases the layout, the suspend timer needs to be moved.
>>>>
>>>
>>> OK Fred, Andy. (I'm also referring to the 1/5 and 2/5 issues here)
>>>
>>> There is a total mess up. LAYOUTGET has nothing to do with layout
>>> (.eg struct pnfs_layout_type) and the layout must not be deallocated
>>> if there is any kind of error and/or if IO is actually to be done  
>>> with
>>> MDS or not. LAYOUTGET corresponds to layout_seg held on a list in
>>> layout.
>>> The list might be empty and so it's flags and state.
>>
>> pnfs_layout_type is the layout cache head structure which has no

Hi Boaz

>
> Why are you calling this "cache" head? Are you referring to the
> segments list .ie layout->lo_layouts. For me it's a list head
> why is it a cache ?

It's a cache of layout segments implemented as a list. We may at some  
point use a b-tree - whatever.

>
>> business in nfsi unless there are layout segments populating it.
>>
>
> This is where we disagree. You say layout is lo_layouts. I say
> it is more, it is that plus the state and additional information.
> An empty list is not a none-existent list. There are two states
> here.

The pnfs_layout_state is in the nfsi.  For file layout, there is no  
need for pnfs_layout_type when there are no layout segments.

What does the object layout driver need in pnfs_layout_type (and/or in  
private data) when there are no layout segments?

>
>>>
>>> Half of the problem was before and this here made it worse. The
>>> life time of nfsi->layout should be the same as nfsi itself just
>>> as it was before.
>>
>> No, the nfs_inode exists with no regard to pnfs_layout_type (horrible
>> name).
>>
>
> Yes
>
>>>
>>> Once the life_time of nfsi && nfsi->layout are unified then all your
>>> problems go away because you are not checking for existence of nfsi
>>> anywhere right? that's because there is no such problem.
>>
>> No, the problems don't go away simply because you statically allocate
>> a cache head struct. We have problems with races and reference
>> counting that exist independently of how a structure is allocated.
>>
>> We don't need to check for the existence of nfsi, The inode is
>> guaranteed to exist until nfs_destroy_inode is called by the VFS.
>>
>
> And so if you restrict the life time of layout with nfsi your problem
> will go away as well, no?

>
>>>
>>> Look at it this way. If a layout_driver is in the game it gets to
>>> allocate it's own area in NFS_I. part of that area is for generic
>>> pnfs.c use, i.e struct pnfs_layout_type, the rest is for private  
>>> use.
>>> once existing it is part of the NFS_I life up till death.
>>
>> No. It should only exists when needed - just like nfsi->nfs_acl and
>> nfsi->delegation etc.
>>
>
> It is the same problem with what we had with commit. We had it all  
> over
> the place and had races refcounting and shit for ever, until we  
> simplified
> it and moved it to the proper NFS checkpoint.
>
> Here too we can just do much better and drop lots of accounting by  
> saying:
>
>  If this nfsi is eligible for pnfs_io and layouts. We allocate a  
> layout
>  governing structure for it. That will be freed at the end together  
> with
>  nfsi. These that never need it like directories links and so on  
> will not
>  have one.

What accounting are you removing with your scheme? If at  
nfs_destroy_inode you have LAYOUTRETURNS (or any LAYOUTXXX) on the  
wire, you have to wait until they are resolved. Exactly the same  
accounting as with my patch - which by the way, did not change any  
refcounting so I'm confused as to your complaint. Note that prior to  
my patches, pnfs_layout_type->lo_data was freed at the last reference!  
All I did was include the pnfs_layout_type because just like the  
lo_data, it is no longer needed.

>
>  Now if that first layout_get() never gets through, well nu, we only  
> used
>  it to say "Don't have any".

A NULL nfsi->layout and/or a state in nfsi->pnfs_layout_state can  
indicate "don't have any layout segments in the cache".

If pNFS is never used,  why have the pnfs_layout_type? Should  
NFSv3/4.0/4.1 mounts have all these extra fields in the inode (which  
is a precious resource)?

>
> Don't you think that is a much simpler model. Why should one shoot  
> one's foot?

No, I don't think it's any simpler. Allocating the structure when it's  
needed and freeing it when it's not is the normal way to do this -  
just like delegations.

I do agree that we need to review the refcounting.

-->Andy

>
> Boaz
>
>>>
>>> If it's not so it should be fixed, not some members leaking out
>>> to generic structures.
>>>
>>> Boaz
>>>
>>>> Signed-off-by: Fred Isaman <iisaman@netapp.com>
>>>> ---
>>>> fs/nfs/inode.c         |    1 +
>>>> fs/nfs/pnfs.c          |    8 ++++----
>>>> include/linux/nfs_fs.h |    2 +-
>>>> 3 files changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>>>> index 66d7be2..cb12d33 100644
>>>> --- a/fs/nfs/inode.c
>>>> +++ b/fs/nfs/inode.c
>>>> @@ -1420,6 +1420,7 @@ static void pnfs_init_once(struct nfs_inode
>>>> *nfsi)
>>>> #ifdef CONFIG_NFS_V4_1
>>>> 	init_waitqueue_head(&nfsi->lo_waitq);
>>>> 	nfsi->pnfs_layout_state = 0;
>>>> +	nfsi->pnfs_layout_suspend = 0;
>>>> 	seqlock_init(&nfsi->layout.seqlock);
>>>> 	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
>>>> 	INIT_LIST_HEAD(&nfsi->layout.segs);
>>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>>> index d05cb03..7f6ace2 100644
>>>> --- a/fs/nfs/pnfs.c
>>>> +++ b/fs/nfs/pnfs.c
>>>> @@ -1105,12 +1105,12 @@ _pnfs_update_layout(struct inode *ino,
>>>>
>>>> 	/* if get layout already failed once goto out */
>>>> 	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
>>>> -		if (unlikely(nfsi->layout.pnfs_layout_suspend &&
>>>> -		    get_seconds() >= nfsi->layout.pnfs_layout_suspend)) {
>>>> +		if (unlikely(nfsi->pnfs_layout_suspend &&
>>>> +		    get_seconds() >= nfsi->pnfs_layout_suspend)) {
>>>> 			dprintk("%s: layout_get resumed\n", __func__);
>>>> 			clear_bit(lo_fail_bit(iomode),
>>>> 				  &nfsi->pnfs_layout_state);
>>>> -			nfsi->layout.pnfs_layout_suspend = 0;
>>>> +			nfsi->pnfs_layout_suspend = 0;
>>>> 		} else
>>>> 			goto out_put;
>>>> 	}
>>>> @@ -1224,7 +1224,7 @@ pnfs_get_layout_done(struct
>>>> nfs4_pnfs_layoutget *lgp, int rpc_status)
>>>> 	}
>>>>
>>>> 	/* remember that get layout failed and suspend trying */
>>>> -	nfsi->layout.pnfs_layout_suspend = suspend;
>>>> +	nfsi->pnfs_layout_suspend = suspend;
>>>> 	set_bit(lo_fail_bit(lgp->args.lseg.iomode),
>>>> 		&nfsi->pnfs_layout_state);
>>>> 	dprintk("%s: layout_get suspended until %ld\n",
>>>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>>>> index e216e24..cebc958 100644
>>>> --- a/include/linux/nfs_fs.h
>>>> +++ b/include/linux/nfs_fs.h
>>>> @@ -105,7 +105,6 @@ struct pnfs_layout_type {
>>>> 	seqlock_t seqlock;		/* Protects the stateid */
>>>> 	nfs4_stateid stateid;
>>>> 	void *ld_data;			/* layout driver private data */
>>>> -	time_t pnfs_layout_suspend;
>>>> 	struct rpc_cred         *lo_cred; /* layoutcommit credential */
>>>> 	/* DH: These vars keep track of the maximum write range
>>>> 	 * so the values can be used for layoutcommit.
>>>> @@ -208,6 +207,7 @@ struct nfs_inode {
>>>> 	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop
>>>> trying */
>>>> 	#define NFS_INO_LAYOUT_ALLOC     2 /* bit lock for layout
>>>> allocation */
>>>> 	#define NFS_INO_LAYOUTCOMMIT     3 /* LAYOUTCOMMIT needed */
>>>> +	time_t pnfs_layout_suspend;
>>>> #endif /* CONFIG_NFS_V4_1 */
>>>> #endif /* CONFIG_NFS_V4*/
>>>> #ifdef CONFIG_NFS_FSCACHE
>>>
>>
>

Re: [PATCH 3/5] SQUASHME: pnfs-submit: move pnfs_layout_suspend back to nfs_inode

[Boaz Harrosh]

On 06/30/2010 07:38 PM, Andy Adamson wrote:
> 
> On Jun 30, 2010, at 11:56 AM, Boaz Harrosh wrote:
> 
>> On 06/30/2010 06:13 PM, Andy Adamson wrote:
>>>
>>> On Jun 30, 2010, at 10:49 AM, Boaz Harrosh wrote:
>>>
>>>> On 06/29/2010 07:42 PM, andros@netapp.com wrote:
>>>>> From: Fred Isaman <iisaman@netapp.com>
>>>>>
>>>>> Prepare to embed struct pnfs_layout_tupe into layout driver private
>>>>> structs
>>>>> and to make layout a pointer. Since a temp error on first LAYOUTGET
>>>>> erases the layout, the suspend timer needs to be moved.
>>>>>
>>>>
>>>> OK Fred, Andy. (I'm also referring to the 1/5 and 2/5 issues here)
>>>>
>>>> There is a total mess up. LAYOUTGET has nothing to do with layout
>>>> (.eg struct pnfs_layout_type) and the layout must not be deallocated
>>>> if there is any kind of error and/or if IO is actually to be done  
>>>> with
>>>> MDS or not. LAYOUTGET corresponds to layout_seg held on a list in
>>>> layout.
>>>> The list might be empty and so it's flags and state.
>>>
>>> pnfs_layout_type is the layout cache head structure which has no
> 
> Hi Boaz
> 
>>
>> Why are you calling this "cache" head? Are you referring to the
>> segments list .ie layout->lo_layouts. For me it's a list head
>> why is it a cache ?
> 
> It's a cache of layout segments implemented as a list. We may at some  
> point use a b-tree - whatever.
> 

First of all It's not my day. layout->lo_layouts is that client list
head which keeps the global list of nfsi's that have layouts.

So we are talking about pnfs_layout_type->segs (Why don't you fix me ;-))

cache for me is a bag-of-objects that is kept around for later use even
though I was done with it. Well it might be that. We could theoretically
call layout_get for every IO and immediately return it after commit, so
we keep it until close or destroy, so it's a cache. (I never looked at
it this way. I always look at it as a file's state. now I'm open, now I'm
active IO...)

OK got you

>>
>>> business in nfsi unless there are layout segments populating it.
>>>
>>
>> This is where we disagree. You say layout is lo_layouts. I say
>> it is more, it is that plus the state and additional information.
>> An empty list is not a none-existent list. There are two states
>> here.
> 
> The pnfs_layout_state is in the nfsi.  For file layout, there is no  
> need for pnfs_layout_type when there are no layout segments.
> 

There is in the generic layer. The before-layout-get state and the
after-layout-return state. The layout-segments "potential" is part
of the game. see your own patches. why did you have to leak some of
these layout members to outside?

> What does the object layout driver need in pnfs_layout_type (and/or in  
> private data) when there are no layout segments?
> 

Nothing

>>
>>>>
>>>> Half of the problem was before and this here made it worse. The
>>>> life time of nfsi->layout should be the same as nfsi itself just
>>>> as it was before.
>>>
>>> No, the nfs_inode exists with no regard to pnfs_layout_type (horrible
>>> name).
>>>
>>
>> Yes
>>
>>>>
>>>> Once the life_time of nfsi && nfsi->layout are unified then all your
>>>> problems go away because you are not checking for existence of nfsi
>>>> anywhere right? that's because there is no such problem.
>>>
>>> No, the problems don't go away simply because you statically allocate
>>> a cache head struct. We have problems with races and reference
>>> counting that exist independently of how a structure is allocated.
>>>
>>> We don't need to check for the existence of nfsi, The inode is
>>> guaranteed to exist until nfs_destroy_inode is called by the VFS.
>>>
>>
>> And so if you restrict the life time of layout with nfsi your problem
>> will go away as well, no?
> 
>>
>>>>
>>>> Look at it this way. If a layout_driver is in the game it gets to
>>>> allocate it's own area in NFS_I. part of that area is for generic
>>>> pnfs.c use, i.e struct pnfs_layout_type, the rest is for private  
>>>> use.
>>>> once existing it is part of the NFS_I life up till death.
>>>
>>> No. It should only exists when needed - just like nfsi->nfs_acl and
>>> nfsi->delegation etc.
>>>
>>
>> It is the same problem with what we had with commit. We had it all  
>> over
>> the place and had races refcounting and shit for ever, until we  
>> simplified
>> it and moved it to the proper NFS checkpoint.
>>
>> Here too we can just do much better and drop lots of accounting by  
>> saying:
>>
>>  If this nfsi is eligible for pnfs_io and layouts. We allocate a  
>> layout
>>  governing structure for it. That will be freed at the end together  
>> with
>>  nfsi. These that never need it like directories links and so on  
>> will not
>>  have one.
> 
> What accounting are you removing with your scheme? If at  
> nfs_destroy_inode you have LAYOUTRETURNS (or any LAYOUTXXX) on the  
> wire, you have to wait until they are resolved. Exactly the same  
> accounting as with my patch - which by the way, did not change any  
> refcounting so I'm confused as to your complaint. Note that prior to  
> my patches, pnfs_layout_type->lo_data was freed at the last reference!  
> All I did was include the pnfs_layout_type because just like the  
> lo_data, it is no longer needed.
> 

Yes the pnfs_layout_type->lo_data was half used as a state variable.
We don't have to do that. We can just keep it around up until nfsi
destructor. This one is surly properly ref-counted, right. So we can
just drop our private ref-counting.

>>
>>  Now if that first layout_get() never gets through, well nu, we only  
>> used
>>  it to say "Don't have any".
> 
> A NULL nfsi->layout and/or a state in nfsi->pnfs_layout_state can  
> indicate "don't have any layout segments in the cache".
> 
> If pNFS is never used,  why have the pnfs_layout_type? Should  
> NFSv3/4.0/4.1 mounts have all these extra fields in the inode (which  
> is a precious resource)?
> 

What? I thought it is understood that these patches was to remove that,
sure, and I agree.
For all modes other then pnfs+regular-rw-files this structure pointer is NULL.
For pnfs-IO-potential files this structure is allocated, together with all
it's members and glory.

so:
* nfsi->layout == NULL - This file will not need pnfs (v234 other)
* nfsi->layout->segs == EMPTY - Before layout_get or after layout_return
* nfsi->layout->XXX - other layout state global to any segments.

>>
>> Don't you think that is a much simpler model. Why should one shoot  
>> one's foot?
> 
> No, I don't think it's any simpler. Allocating the structure when it's  
> needed and freeing it when it's not is the normal way to do this -  
> just like delegations.
> 

I don't know about delegations. Just here. What I'm saying is lets collapse
some of the extra stuff, by using existing life-time rules.

You know what, it looks like this is a big jump for you right now. Leave it
like that. Lets run and stabilize this stage. I will put it on my todo list
to try and sanitize it farther before we submit.

> I do agree that we need to review the refcounting.
> 
> -->Andy
> 
>>

Thanks, this all is moving in the right directions, eventually we will
get there.

Boaz

Re: [PATCH 2/5] SQUASHME: pnfs_submit: move pnfs_layout_state back to nfs_inode

[Boaz Harrosh]

On 06/29/2010 07:42 PM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Prepare to embed struct pnfs_layout_tupe into layout driver private structs
> and to make layout a pointer: state flags need to be accessible before
> allocation.
> 

I don't get that. A pointer inspection operation is atomic. (gone are the
days of 16-bit large/huge models where a pointer is two registers.)

So an: if (lo->pointer) is just as atomic as test_bit()

And I don't see, not here, and not in the next patch, where are these used before
the allocation. if you throw away Fred's first patch and make:
 layoutcommit_needed(struct nfs_inode *nfsi)
 {
-	return nfsi->layout.lo_cred != NULL;
+	return nfsi->layout != NULL;
 }

Then all three patches (first one thrown away) can just collapse to a single
embedded-to-pointer conversion.

Boaz
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/inode.c         |    2 +-
>  fs/nfs/pnfs.c          |   16 ++++++++--------
>  include/linux/nfs_fs.h |   10 +++++-----
>  3 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 7989dea..66d7be2 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -1364,7 +1364,6 @@ void nfs4_clear_inode(struct inode *inode)
>  static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
>  {
>  #ifdef CONFIG_NFS_V4_1
> -	nfsi->layout.pnfs_layout_state = 0;
>  	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>  	nfsi->layout.roc_iomode = 0;
>  	nfsi->layout.lo_cred = NULL;
> @@ -1420,6 +1419,7 @@ static void pnfs_init_once(struct nfs_inode *nfsi)
>  {
>  #ifdef CONFIG_NFS_V4_1
>  	init_waitqueue_head(&nfsi->lo_waitq);
> +	nfsi->pnfs_layout_state = 0;
>  	seqlock_init(&nfsi->layout.seqlock);
>  	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
>  	INIT_LIST_HEAD(&nfsi->layout.segs);
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index bf15b5c..d05cb03 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -951,7 +951,7 @@ get_lock_alloc_layout(struct inode *ino)
>  		 * wait until bit is cleared if we lost this race.
>  		 */
>  		res = wait_on_bit_lock(
> -			&nfsi->layout.pnfs_layout_state,
> +			&nfsi->pnfs_layout_state,
>  			NFS_INO_LAYOUT_ALLOC,
>  			pnfs_wait_schedule, TASK_KILLABLE);
>  		if (res) {
> @@ -975,8 +975,8 @@ get_lock_alloc_layout(struct inode *ino)
>  
>  		/* release the NFS_INO_LAYOUT_ALLOC bit and wake up waiters */
>  		clear_bit_unlock(NFS_INO_LAYOUT_ALLOC,
> -				 &nfsi->layout.pnfs_layout_state);
> -		wake_up_bit(&nfsi->layout.pnfs_layout_state,
> +				 &nfsi->pnfs_layout_state);
> +		wake_up_bit(&nfsi->pnfs_layout_state,
>  			    NFS_INO_LAYOUT_ALLOC);
>  		break;
>  	}
> @@ -1104,12 +1104,12 @@ _pnfs_update_layout(struct inode *ino,
>  	}
>  
>  	/* if get layout already failed once goto out */
> -	if (test_bit(lo_fail_bit(iomode), &nfsi->layout.pnfs_layout_state)) {
> +	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
>  		if (unlikely(nfsi->layout.pnfs_layout_suspend &&
>  		    get_seconds() >= nfsi->layout.pnfs_layout_suspend)) {
>  			dprintk("%s: layout_get resumed\n", __func__);
>  			clear_bit(lo_fail_bit(iomode),
> -				  &nfsi->layout.pnfs_layout_state);
> +				  &nfsi->pnfs_layout_state);
>  			nfsi->layout.pnfs_layout_suspend = 0;
>  		} else
>  			goto out_put;
> @@ -1121,7 +1121,7 @@ _pnfs_update_layout(struct inode *ino,
>  	send_layoutget(ino, ctx, &arg, lsegpp, lo);
>  out:
>  	dprintk("%s end, state 0x%lx lseg %p\n", __func__,
> -		nfsi->layout.pnfs_layout_state, lseg);
> +		nfsi->pnfs_layout_state, lseg);
>  	return;
>  out_put:
>  	if (lsegpp)
> @@ -1226,12 +1226,12 @@ pnfs_get_layout_done(struct nfs4_pnfs_layoutget *lgp, int rpc_status)
>  	/* remember that get layout failed and suspend trying */
>  	nfsi->layout.pnfs_layout_suspend = suspend;
>  	set_bit(lo_fail_bit(lgp->args.lseg.iomode),
> -		&nfsi->layout.pnfs_layout_state);
> +		&nfsi->pnfs_layout_state);
>  	dprintk("%s: layout_get suspended until %ld\n",
>  		__func__, suspend);
>  out:
>  	dprintk("%s end (err:%d) state 0x%lx lseg %p\n",
> -		__func__, lgp->status, nfsi->layout.pnfs_layout_state, lseg);
> +		__func__, lgp->status, nfsi->pnfs_layout_state, lseg);
>  	return;
>  }
>  
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index a33e86e..e216e24 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -105,11 +105,6 @@ struct pnfs_layout_type {
>  	seqlock_t seqlock;		/* Protects the stateid */
>  	nfs4_stateid stateid;
>  	void *ld_data;			/* layout driver private data */
> -	unsigned long pnfs_layout_state;
> -	#define NFS_INO_RO_LAYOUT_FAILED 0      /* get ro layout failed stop trying */
> -	#define NFS_INO_RW_LAYOUT_FAILED 1      /* get rw layout failed stop trying */
> -	#define NFS_INO_LAYOUT_ALLOC     2      /* bit lock for layout allocation */
> -	#define NFS_INO_LAYOUTCOMMIT     3	/* LAYOUTCOMMIT needed */
>  	time_t pnfs_layout_suspend;
>  	struct rpc_cred         *lo_cred; /* layoutcommit credential */
>  	/* DH: These vars keep track of the maximum write range
> @@ -208,6 +203,11 @@ struct nfs_inode {
>  #if defined(CONFIG_NFS_V4_1)
>  	wait_queue_head_t lo_waitq;
>  	struct pnfs_layout_type layout;
> +	unsigned long pnfs_layout_state;
> +	#define NFS_INO_RO_LAYOUT_FAILED 0 /* ro LAYOUTGET failed stop trying */
> +	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop trying */
> +	#define NFS_INO_LAYOUT_ALLOC     2 /* bit lock for layout allocation */
> +	#define NFS_INO_LAYOUTCOMMIT     3 /* LAYOUTCOMMIT needed */
>  #endif /* CONFIG_NFS_V4_1 */
>  #endif /* CONFIG_NFS_V4*/
>  #ifdef CONFIG_NFS_FSCACHE

Re: [PATCH 2/5] SQUASHME: pnfs_submit: move pnfs_layout_state back to nfs_inode

[Boaz Harrosh]

On 06/30/2010 12:05 PM, Boaz Harrosh wrote:
> On 06/29/2010 07:42 PM, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Prepare to embed struct pnfs_layout_tupe into layout driver private structs
>> and to make layout a pointer: state flags need to be accessible before
>> allocation.
>>
> 
> I don't get that. A pointer inspection operation is atomic. (gone are the
> days of 16-bit large/huge models where a pointer is two registers.)
> 
> So an: if (lo->pointer) is just as atomic as test_bit()
> 
> And I don't see, not here, and not in the next patch, where are these used before
> the allocation. if you throw away Fred's first patch and make:
>  layoutcommit_needed(struct nfs_inode *nfsi)
>  {
> -	return nfsi->layout.lo_cred != NULL;
> +	return nfsi->layout != NULL;
>  }
> 

Rrrr need my morning coffee, sorry. 

But still, Fred's patch can go both sites of the pointer assignment have an
MB and a pointer inspection is there-for atomic. Above can then be:

 layoutcommit_needed(struct nfs_inode *nfsi)
 {
-	return nfsi->layout.lo_cred != NULL;
+	return nfsi->layout && (nfsi->layout->lo_cred != NULL);
 }


> Then all three patches (first one thrown away) can just collapse to a single
> embedded-to-pointer conversion.
> 

This still applies

Boaz
> Boaz
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>>  fs/nfs/inode.c         |    2 +-
>>  fs/nfs/pnfs.c          |   16 ++++++++--------
>>  include/linux/nfs_fs.h |   10 +++++-----
>>  3 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index 7989dea..66d7be2 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -1364,7 +1364,6 @@ void nfs4_clear_inode(struct inode *inode)
>>  static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
>>  {
>>  #ifdef CONFIG_NFS_V4_1
>> -	nfsi->layout.pnfs_layout_state = 0;
>>  	memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>>  	nfsi->layout.roc_iomode = 0;
>>  	nfsi->layout.lo_cred = NULL;
>> @@ -1420,6 +1419,7 @@ static void pnfs_init_once(struct nfs_inode *nfsi)
>>  {
>>  #ifdef CONFIG_NFS_V4_1
>>  	init_waitqueue_head(&nfsi->lo_waitq);
>> +	nfsi->pnfs_layout_state = 0;
>>  	seqlock_init(&nfsi->layout.seqlock);
>>  	INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
>>  	INIT_LIST_HEAD(&nfsi->layout.segs);
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index bf15b5c..d05cb03 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -951,7 +951,7 @@ get_lock_alloc_layout(struct inode *ino)
>>  		 * wait until bit is cleared if we lost this race.
>>  		 */
>>  		res = wait_on_bit_lock(
>> -			&nfsi->layout.pnfs_layout_state,
>> +			&nfsi->pnfs_layout_state,
>>  			NFS_INO_LAYOUT_ALLOC,
>>  			pnfs_wait_schedule, TASK_KILLABLE);
>>  		if (res) {
>> @@ -975,8 +975,8 @@ get_lock_alloc_layout(struct inode *ino)
>>  
>>  		/* release the NFS_INO_LAYOUT_ALLOC bit and wake up waiters */
>>  		clear_bit_unlock(NFS_INO_LAYOUT_ALLOC,
>> -				 &nfsi->layout.pnfs_layout_state);
>> -		wake_up_bit(&nfsi->layout.pnfs_layout_state,
>> +				 &nfsi->pnfs_layout_state);
>> +		wake_up_bit(&nfsi->pnfs_layout_state,
>>  			    NFS_INO_LAYOUT_ALLOC);
>>  		break;
>>  	}
>> @@ -1104,12 +1104,12 @@ _pnfs_update_layout(struct inode *ino,
>>  	}
>>  
>>  	/* if get layout already failed once goto out */
>> -	if (test_bit(lo_fail_bit(iomode), &nfsi->layout.pnfs_layout_state)) {
>> +	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
>>  		if (unlikely(nfsi->layout.pnfs_layout_suspend &&
>>  		    get_seconds() >= nfsi->layout.pnfs_layout_suspend)) {
>>  			dprintk("%s: layout_get resumed\n", __func__);
>>  			clear_bit(lo_fail_bit(iomode),
>> -				  &nfsi->layout.pnfs_layout_state);
>> +				  &nfsi->pnfs_layout_state);
>>  			nfsi->layout.pnfs_layout_suspend = 0;
>>  		} else
>>  			goto out_put;
>> @@ -1121,7 +1121,7 @@ _pnfs_update_layout(struct inode *ino,
>>  	send_layoutget(ino, ctx, &arg, lsegpp, lo);
>>  out:
>>  	dprintk("%s end, state 0x%lx lseg %p\n", __func__,
>> -		nfsi->layout.pnfs_layout_state, lseg);
>> +		nfsi->pnfs_layout_state, lseg);
>>  	return;
>>  out_put:
>>  	if (lsegpp)
>> @@ -1226,12 +1226,12 @@ pnfs_get_layout_done(struct nfs4_pnfs_layoutget *lgp, int rpc_status)
>>  	/* remember that get layout failed and suspend trying */
>>  	nfsi->layout.pnfs_layout_suspend = suspend;
>>  	set_bit(lo_fail_bit(lgp->args.lseg.iomode),
>> -		&nfsi->layout.pnfs_layout_state);
>> +		&nfsi->pnfs_layout_state);
>>  	dprintk("%s: layout_get suspended until %ld\n",
>>  		__func__, suspend);
>>  out:
>>  	dprintk("%s end (err:%d) state 0x%lx lseg %p\n",
>> -		__func__, lgp->status, nfsi->layout.pnfs_layout_state, lseg);
>> +		__func__, lgp->status, nfsi->pnfs_layout_state, lseg);
>>  	return;
>>  }
>>  
>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>> index a33e86e..e216e24 100644
>> --- a/include/linux/nfs_fs.h
>> +++ b/include/linux/nfs_fs.h
>> @@ -105,11 +105,6 @@ struct pnfs_layout_type {
>>  	seqlock_t seqlock;		/* Protects the stateid */
>>  	nfs4_stateid stateid;
>>  	void *ld_data;			/* layout driver private data */
>> -	unsigned long pnfs_layout_state;
>> -	#define NFS_INO_RO_LAYOUT_FAILED 0      /* get ro layout failed stop trying */
>> -	#define NFS_INO_RW_LAYOUT_FAILED 1      /* get rw layout failed stop trying */
>> -	#define NFS_INO_LAYOUT_ALLOC     2      /* bit lock for layout allocation */
>> -	#define NFS_INO_LAYOUTCOMMIT     3	/* LAYOUTCOMMIT needed */
>>  	time_t pnfs_layout_suspend;
>>  	struct rpc_cred         *lo_cred; /* layoutcommit credential */
>>  	/* DH: These vars keep track of the maximum write range
>> @@ -208,6 +203,11 @@ struct nfs_inode {
>>  #if defined(CONFIG_NFS_V4_1)
>>  	wait_queue_head_t lo_waitq;
>>  	struct pnfs_layout_type layout;
>> +	unsigned long pnfs_layout_state;
>> +	#define NFS_INO_RO_LAYOUT_FAILED 0 /* ro LAYOUTGET failed stop trying */
>> +	#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed stop trying */
>> +	#define NFS_INO_LAYOUT_ALLOC     2 /* bit lock for layout allocation */
>> +	#define NFS_INO_LAYOUTCOMMIT     3 /* LAYOUTCOMMIT needed */
>>  #endif /* CONFIG_NFS_V4_1 */
>>  #endif /* CONFIG_NFS_V4*/
>>  #ifdef CONFIG_NFS_FSCACHE
> 

Re: [PATCH 2/5] SQUASHME: pnfs_submit: move pnfs_layout_state back to nfs_inode

[Fred Isaman]

On Wed, Jun 30, 2010 at 5:31 AM, Boaz Harrosh <bharrosh@panasas.com> wr=
ote:
> On 06/30/2010 12:05 PM, Boaz Harrosh wrote:
>> On 06/29/2010 07:42 PM, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> Prepare to embed struct pnfs_layout_tupe into layout driver private=
 structs
>>> and to make layout a pointer: state flags need to be accessible bef=
ore
>>> allocation.
>>>
>>
>> I don't get that. A pointer inspection operation is atomic. (gone ar=
e the
>> days of 16-bit large/huge models where a pointer is two registers.)
>>
>> So an: if (lo->pointer) is just as atomic as test_bit()
>>
>> And I don't see, not here, and not in the next patch, where are thes=
e used before
>> the allocation. if you throw away Fred's first patch and make:
>> =A0layoutcommit_needed(struct nfs_inode *nfsi)
>> =A0{
>> - =A0 =A0 return nfsi->layout.lo_cred !=3D NULL;
>> + =A0 =A0 return nfsi->layout !=3D NULL;
>> =A0}
>>
>
> Rrrr need my morning coffee, sorry.
>
> But still, Fred's patch can go both sites of the pointer assignment h=
ave an
> MB and a pointer inspection is there-for atomic. Above can then be:
>
> =A0layoutcommit_needed(struct nfs_inode *nfsi)
> =A0{
> - =A0 =A0 =A0 return nfsi->layout.lo_cred !=3D NULL;
> + =A0 =A0 =A0 return nfsi->layout && (nfsi->layout->lo_cred !=3D NULL=
);
> =A0}
>

This is called outside of pnfs.c, without lock.  Thus there is no
guarantee that nfsi->layout won't go to to NULL during the '&&'.

=46red

>
>> Then all three patches (first one thrown away) can just collapse to =
a single
>> embedded-to-pointer conversion.
>>
>
> This still applies
>
> Boaz
>> Boaz
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> =A0fs/nfs/inode.c =A0 =A0 =A0 =A0 | =A0 =A02 +-
>>> =A0fs/nfs/pnfs.c =A0 =A0 =A0 =A0 =A0| =A0 16 ++++++++--------
>>> =A0include/linux/nfs_fs.h | =A0 10 +++++-----
>>> =A03 files changed, 14 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>>> index 7989dea..66d7be2 100644
>>> --- a/fs/nfs/inode.c
>>> +++ b/fs/nfs/inode.c
>>> @@ -1364,7 +1364,6 @@ void nfs4_clear_inode(struct inode *inode)
>>> =A0static void pnfs_alloc_init_inode(struct nfs_inode *nfsi)
>>> =A0{
>>> =A0#ifdef CONFIG_NFS_V4_1
>>> - =A0 =A0nfsi->layout.pnfs_layout_state =3D 0;
>>> =A0 =A0 =A0memset(&nfsi->layout.stateid, 0, NFS4_STATEID_SIZE);
>>> =A0 =A0 =A0nfsi->layout.roc_iomode =3D 0;
>>> =A0 =A0 =A0nfsi->layout.lo_cred =3D NULL;
>>> @@ -1420,6 +1419,7 @@ static void pnfs_init_once(struct nfs_inode *=
nfsi)
>>> =A0{
>>> =A0#ifdef CONFIG_NFS_V4_1
>>> =A0 =A0 =A0init_waitqueue_head(&nfsi->lo_waitq);
>>> + =A0 =A0nfsi->pnfs_layout_state =3D 0;
>>> =A0 =A0 =A0seqlock_init(&nfsi->layout.seqlock);
>>> =A0 =A0 =A0INIT_LIST_HEAD(&nfsi->layout.lo_layouts);
>>> =A0 =A0 =A0INIT_LIST_HEAD(&nfsi->layout.segs);
>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>> index bf15b5c..d05cb03 100644
>>> --- a/fs/nfs/pnfs.c
>>> +++ b/fs/nfs/pnfs.c
>>> @@ -951,7 +951,7 @@ get_lock_alloc_layout(struct inode *ino)
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 * wait until bit is cleared if we lost =
this race.
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 */
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0res =3D wait_on_bit_lock(
>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&nfsi->layout.pnfs_layout_=
state,
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&nfsi->pnfs_layout_state,
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0NFS_INO_LAYOUT_ALLOC,
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pnfs_wait_schedule, TASK=
_KILLABLE);
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0if (res) {
>>> @@ -975,8 +975,8 @@ get_lock_alloc_layout(struct inode *ino)
>>>
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0/* release the NFS_INO_LAYOUT_ALLOC bit =
and wake up waiters */
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0clear_bit_unlock(NFS_INO_LAYOUT_ALLOC,
>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &nfsi->la=
yout.pnfs_layout_state);
>>> - =A0 =A0 =A0 =A0 =A0 =A0wake_up_bit(&nfsi->layout.pnfs_layout_stat=
e,
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &nfsi->pn=
fs_layout_state);
>>> + =A0 =A0 =A0 =A0 =A0 =A0wake_up_bit(&nfsi->pnfs_layout_state,
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0NFS_INO_LAYOUT_A=
LLOC);
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>>> =A0 =A0 =A0}
>>> @@ -1104,12 +1104,12 @@ _pnfs_update_layout(struct inode *ino,
>>> =A0 =A0 =A0}
>>>
>>> =A0 =A0 =A0/* if get layout already failed once goto out */
>>> - =A0 =A0if (test_bit(lo_fail_bit(iomode), &nfsi->layout.pnfs_layou=
t_state)) {
>>> + =A0 =A0if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state=
)) {
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0if (unlikely(nfsi->layout.pnfs_layout_su=
spend &&
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0get_seconds() >=3D nfsi->layout.=
pnfs_layout_suspend)) {
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dprintk("%s: layout_get =
resumed\n", __func__);
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0clear_bit(lo_fail_bit(io=
mode),
>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&nfsi-=
>layout.pnfs_layout_state);
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0&nfsi-=
>pnfs_layout_state);
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0nfsi->layout.pnfs_layout=
_suspend =3D 0;
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0} else
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto out_put;
>>> @@ -1121,7 +1121,7 @@ _pnfs_update_layout(struct inode *ino,
>>> =A0 =A0 =A0send_layoutget(ino, ctx, &arg, lsegpp, lo);
>>> =A0out:
>>> =A0 =A0 =A0dprintk("%s end, state 0x%lx lseg %p\n", __func__,
>>> - =A0 =A0 =A0 =A0 =A0 =A0nfsi->layout.pnfs_layout_state, lseg);
>>> + =A0 =A0 =A0 =A0 =A0 =A0nfsi->pnfs_layout_state, lseg);
>>> =A0 =A0 =A0return;
>>> =A0out_put:
>>> =A0 =A0 =A0if (lsegpp)
>>> @@ -1226,12 +1226,12 @@ pnfs_get_layout_done(struct nfs4_pnfs_layou=
tget *lgp, int rpc_status)
>>> =A0 =A0 =A0/* remember that get layout failed and suspend trying */
>>> =A0 =A0 =A0nfsi->layout.pnfs_layout_suspend =3D suspend;
>>> =A0 =A0 =A0set_bit(lo_fail_bit(lgp->args.lseg.iomode),
>>> - =A0 =A0 =A0 =A0 =A0 =A0&nfsi->layout.pnfs_layout_state);
>>> + =A0 =A0 =A0 =A0 =A0 =A0&nfsi->pnfs_layout_state);
>>> =A0 =A0 =A0dprintk("%s: layout_get suspended until %ld\n",
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0__func__, suspend);
>>> =A0out:
>>> =A0 =A0 =A0dprintk("%s end (err:%d) state 0x%lx lseg %p\n",
>>> - =A0 =A0 =A0 =A0 =A0 =A0__func__, lgp->status, nfsi->layout.pnfs_l=
ayout_state, lseg);
>>> + =A0 =A0 =A0 =A0 =A0 =A0__func__, lgp->status, nfsi->pnfs_layout_s=
tate, lseg);
>>> =A0 =A0 =A0return;
>>> =A0}
>>>
>>> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
>>> index a33e86e..e216e24 100644
>>> --- a/include/linux/nfs_fs.h
>>> +++ b/include/linux/nfs_fs.h
>>> @@ -105,11 +105,6 @@ struct pnfs_layout_type {
>>> =A0 =A0 =A0seqlock_t seqlock; =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Protect=
s the stateid */
>>> =A0 =A0 =A0nfs4_stateid stateid;
>>> =A0 =A0 =A0void *ld_data; =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* lay=
out driver private data */
>>> - =A0 =A0unsigned long pnfs_layout_state;
>>> - =A0 =A0#define NFS_INO_RO_LAYOUT_FAILED 0 =A0 =A0 =A0/* get ro la=
yout failed stop trying */
>>> - =A0 =A0#define NFS_INO_RW_LAYOUT_FAILED 1 =A0 =A0 =A0/* get rw la=
yout failed stop trying */
>>> - =A0 =A0#define NFS_INO_LAYOUT_ALLOC =A0 =A0 2 =A0 =A0 =A0/* bit l=
ock for layout allocation */
>>> - =A0 =A0#define NFS_INO_LAYOUTCOMMIT =A0 =A0 3 =A0 =A0 =A0/* LAYOU=
TCOMMIT needed */
>>> =A0 =A0 =A0time_t pnfs_layout_suspend;
>>> =A0 =A0 =A0struct rpc_cred =A0 =A0 =A0 =A0 *lo_cred; /* layoutcommi=
t credential */
>>> =A0 =A0 =A0/* DH: These vars keep track of the maximum write range
>>> @@ -208,6 +203,11 @@ struct nfs_inode {
>>> =A0#if defined(CONFIG_NFS_V4_1)
>>> =A0 =A0 =A0wait_queue_head_t lo_waitq;
>>> =A0 =A0 =A0struct pnfs_layout_type layout;
>>> + =A0 =A0unsigned long pnfs_layout_state;
>>> + =A0 =A0#define NFS_INO_RO_LAYOUT_FAILED 0 /* ro LAYOUTGET failed =
stop trying */
>>> + =A0 =A0#define NFS_INO_RW_LAYOUT_FAILED 1 /* rw LAYOUTGET failed =
stop trying */
>>> + =A0 =A0#define NFS_INO_LAYOUT_ALLOC =A0 =A0 2 /* bit lock for lay=
out allocation */
>>> + =A0 =A0#define NFS_INO_LAYOUTCOMMIT =A0 =A0 3 /* LAYOUTCOMMIT nee=
ded */
>>> =A0#endif /* CONFIG_NFS_V4_1 */
>>> =A0#endif /* CONFIG_NFS_V4*/
>>> =A0#ifdef CONFIG_NFS_FSCACHE
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html
>