(unknown),

(unknown),

[andros]

This is against the pnfs-submit branch of the 2.6.34 tree. They will need to be
applied against the 2.6.35-rc1 tree which I can do after comments.

RFC: I would like comments, especially on
0006-SQUASHME-pnfs-submit-move-layoutcommit-to-nfs_write_.patch.

Remove unused layoutcommit layoutdriver_io_operations. Will be restored
in post-submit patches
0001-SQUASHME-pnfs-submit-remove-setup_layoutcommit.patch
0002-SQUASHNE-pnfs-submit-remove-cleanup_layoutcommit.patch
0003-SQUASHME-pnfs-submit-remove-encode_layoutcommit.patch

A cleanup, and call the async error handler.
0004-SQUASHME-pnfs-submit-cleanup-layoutcommit-call.patch
0005-SQUASHME-pnfs-submit-handle-async-layoutcommit-error.patch

This next  patch moves the pnfs_layoutcommit_inode call to nfs_write_inode,
and it is the only call other than in layoutreturn. (removed calls in
__nfs4_close, nfs_commit_inode, nfs_wb_sync).

This is fine for the file layout, and I think it's OK for the object and
block layouts as well.

I left the LAYOUTCOMMIT call in nfs_write_inode a synchronous call, because
nfs_commit_unstable_pages sets the FLUSH_SYNC flag. Should this
be an asyc LAYOUTCOMMIT call?

pnfs_layoutcommit_inode is called after nfs_commit_unstable_pages() so that
if LAYOUTCOMMIT fails, the unstable pages have been processed..

The error handlers (sync and async) call nfs4_map_errors, so unhandled
errors (such as NFS4ERR_BADLAYOUT) get returned to nfs_write_ioode as -EIO.

Examining the write_inode call paths, I could not see where the -EIO would
be passed back to the application.  Testing with pynfs which I
had return NFS4ERR_BADLAYOUT to the layout commit call, shows the -EIO return
not stopping the client nor is the error reported back to the application.

We will add code to the error handlers for errors such as NFS4ERR_BADLAYOUT 
that require us to stop using and free the layout, and redo the I/O through
the MDS.

Anyway, review is much appreciated.

0006-SQUASHME-pnfs-submit-move-layoutcommit-to-nfs_write_.patch

Testing:
With CONFIG_NFS_V4_1 set
NFSv4.1/pnfs passed Connectathon against write enabled GFS2/pNFS. Note: there
were exactly the same number of LAYOUTCOMMITS sent as were sent with
pnfs_layoutcommit_inode being called from __nfs4_close (never happened),
nfs_commit_inode and nfs_wb_sync.

Passed Connectathon general test against pynfs file layout server with
the NFS4ERR_BADLAYOUT being returned on every third LAYOUTCOMMIT.


-->Andy

[PATCH 1/6] SQUASHME pnfs-submit: remove setup_layoutcommit

[andros]

From: Andy Adamson <andros@netapp.com>

Not used by the file layout driver

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/pnfs.c             |    7 -------
 include/linux/nfs4_pnfs.h |    2 --
 2 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 8cc4412..23787ff 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1944,7 +1944,6 @@ pnfs_layoutcommit_setup(struct inode *inode,
 			struct pnfs_layoutcommit_data *data,
 			loff_t write_begin_pos, loff_t write_end_pos, int sync)
 {
-	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_server *nfss = NFS_SERVER(inode);
 	int result = 0;
 
@@ -1970,12 +1969,6 @@ pnfs_layoutcommit_setup(struct inode *inode,
 	data->args.bitmask = nfss->attr_bitmask;
 	data->res.server = nfss;
 
-	/* Call layout driver to set the arguments.
-	 */
-	if (nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit)
-		result = nfss->pnfs_curr_ld->ld_io_ops->setup_layoutcommit(
-				&nfsi->layout, &data->args);
-
 	dprintk("%s End Status %d\n", __func__, result);
 	return result;
 }
diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
index 84d2e95..cab9137 100644
--- a/include/linux/nfs4_pnfs.h
+++ b/include/linux/nfs4_pnfs.h
@@ -146,8 +146,6 @@ struct layoutdriver_io_operations {
 	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);
 
-	int (*setup_layoutcommit) (struct pnfs_layout_type *layoutid,
-				    struct pnfs_layoutcommit_arg *args);
 	void (*encode_layoutcommit) (struct pnfs_layout_type *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct pnfs_layoutcommit_arg *args);
-- 
1.6.6

[PATCH 2/6] SQUASHNE pnfs-submit: remove cleanup_layoutcommit

[andros]

From: Andy Adamson <andros@netapp.com>

Not used by the file layout driver

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c         |    4 ++--
 fs/nfs/pnfs.c             |   24 ------------------------
 fs/nfs/pnfs.h             |    1 -
 include/linux/nfs4_pnfs.h |    3 ---
 4 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 8b375a7..2826ada 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5600,7 +5600,7 @@ static int pnfs4_sync_layoutcommit(struct pnfs_layoutcommit_data *data)
 	} while (exception.retry);
 
 	data->status = err;
-	pnfs_layoutcommit_done(data);
+	put_rpccred(data->cred);
 	pnfs_layoutcommit_free(data);
 	return err;
 }
@@ -5625,7 +5625,7 @@ pnfs_layoutcommit_rpc_done(struct rpc_task *task, void *calldata)
 	struct nfs_server *server = NFS_SERVER(data->args.inode);
 
 	data->status = task->tk_status;
-	pnfs_layoutcommit_done(data);
+	put_rpccred(data->cred);
 
 	nfs4_sequence_done(server, &data->res.seq_res, task->tk_status);
 }
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 23787ff..def27f8 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1912,30 +1912,6 @@ out:
 	return trypnfs;
 }
 
-/* Called on completion of layoutcommit */
-void
-pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
-{
-	struct nfs_server *nfss = NFS_SERVER(data->args.inode);
-	struct nfs_inode *nfsi = NFS_I(data->args.inode);
-
-	dprintk("%s: (status %d)\n", __func__, data->status);
-
-	if (data->status < 0)
-		printk(KERN_ERR "%s, Layoutcommit Failed! = %d\n",
-		       __func__, data->status);
-
-	/* TODO: Maybe we should avoid this by allowing the layout driver
-	 * to directly xdr its layout on the wire.
-	 */
-	if (nfss->pnfs_curr_ld->ld_io_ops->cleanup_layoutcommit)
-		nfss->pnfs_curr_ld->ld_io_ops->cleanup_layoutcommit(
-							&nfsi->layout,
-							&data->args,
-							data->status);
-	put_rpccred(data->cred);
-}
-
 /*
  * Set up the argument/result storage required for the RPC call.
  */
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index c89be78..43e02f8 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -46,7 +46,6 @@ enum pnfs_try_status _pnfs_try_to_read_data(struct nfs_read_data *,
 					    const struct rpc_call_ops *);
 int pnfs_initialize(void);
 void pnfs_uninitialize(void);
-void pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data);
 void pnfs_layoutcommit_free(struct pnfs_layoutcommit_data *data);
 int pnfs_layoutcommit_inode(struct inode *inode, int sync);
 void pnfs_update_last_write(struct nfs_inode *nfsi, loff_t offset, size_t extent);
diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
index cab9137..1b017fe 100644
--- a/include/linux/nfs4_pnfs.h
+++ b/include/linux/nfs4_pnfs.h
@@ -149,9 +149,6 @@ struct layoutdriver_io_operations {
 	void (*encode_layoutcommit) (struct pnfs_layout_type *layoutid,
 				     struct xdr_stream *xdr,
 				     const struct pnfs_layoutcommit_arg *args);
-	void (*cleanup_layoutcommit) (struct pnfs_layout_type *layoutid,
-				      struct pnfs_layoutcommit_arg *args,
-				      int status);
 	void (*encode_layoutreturn) (struct pnfs_layout_type *layoutid,
 				struct xdr_stream *xdr,
 				const struct nfs4_pnfs_layoutreturn_arg *args);
-- 
1.6.6

[PATCH 3/6] SQUASHME pnfs-submit: remove encode_layoutcommit

[andros]

From: Andy Adamson <andros@netapp.com>

Not used by the file layout driver

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4xdr.c          |   11 ++---------
 include/linux/nfs4_pnfs.h |    3 ---
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index d7d41e9..72fd799 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1852,8 +1852,6 @@ encode_layoutcommit(struct xdr_stream *xdr,
 		    const struct pnfs_layoutcommit_arg *args,
 		    struct compound_hdr *hdr)
 {
-	struct layoutdriver_io_operations *ld_io_ops =
-			NFS_SERVER(args->inode)->pnfs_curr_ld->ld_io_ops;
 	__be32 *p;
 
 	dprintk("%s: %llu@%llu lbw: %llu type: %d\n", __func__,
@@ -1879,13 +1877,8 @@ encode_layoutcommit(struct xdr_stream *xdr,
 	p = reserve_space(xdr, 4);
 	*p = cpu_to_be32(args->layout_type);
 
-	if (ld_io_ops->encode_layoutcommit)
-		ld_io_ops->encode_layoutcommit(
-			&NFS_I(args->inode)->layout, xdr, args);
-	else {
-		p = reserve_space(xdr, 4);
-		xdr_encode_opaque(p, NULL, 0);
-	}
+	p = reserve_space(xdr, 4);
+	xdr_encode_opaque(p, NULL, 0);
 
 	hdr->nops++;
 	hdr->replen += decode_layoutcommit_maxsz;
diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
index 1b017fe..3c22168 100644
--- a/include/linux/nfs4_pnfs.h
+++ b/include/linux/nfs4_pnfs.h
@@ -146,9 +146,6 @@ struct layoutdriver_io_operations {
 	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);
 
-	void (*encode_layoutcommit) (struct pnfs_layout_type *layoutid,
-				     struct xdr_stream *xdr,
-				     const struct pnfs_layoutcommit_arg *args);
 	void (*encode_layoutreturn) (struct pnfs_layout_type *layoutid,
 				struct xdr_stream *xdr,
 				const struct nfs4_pnfs_layoutreturn_arg *args);
-- 
1.6.6

[PATCH 4/6] SQUASHME pnfs-submit: cleanup layoutcommit call

[andros]

From: Andy Adamson <andros@netapp.com>

pnfs_layoutcommit_inode prints its status upon exit.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/pnfs.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index def27f8..91e944a 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -766,15 +766,8 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
 				!pnfs_return_layout_barrier(nfsi, &arg));
 		}
 
-		if (layoutcommit_needed(nfsi)) {
-			status = pnfs_layoutcommit_inode(ino, wait);
-			if (status) {
-				dprintk("%s: layoutcommit failed, status=%d. "
-					"Returning layout anyway\n",
-					__func__, status);
-				status = 0;
-			}
-		}
+		if (layoutcommit_needed(nfsi))
+			pnfs_layoutcommit_inode(ino, wait);
 	}
 send_return:
 	status = return_layout(ino, &arg, stateid, type, lo, wait);
-- 
1.6.6

[PATCH 5/6] SQUASHME pnfs-submit: handle async layoutcommit errors

[andros]

From: Andy Adamson <andros@netapp.com>

nfs4_async_handle_error handles session level and grace,delay errors.

NOTE: Layout specific errors (NFS4ERR_BADIOMODE, NFS4ERR_BAD_LAYOUT,
NFS4ERR_UNKNOWN_LAYOUTTYPE) need to be handled.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4proc.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 2826ada..514c5c6 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5628,6 +5628,9 @@ pnfs_layoutcommit_rpc_done(struct rpc_task *task, void *calldata)
 	put_rpccred(data->cred);
 
 	nfs4_sequence_done(server, &data->res.seq_res, task->tk_status);
+
+	if (nfs4_async_handle_error(task, server, NULL) == -EAGAIN)
+		nfs_restart_rpc(task, server->nfs_client);
 }
 
 static void pnfs_layoutcommit_release(void *lcdata)
-- 
1.6.6

[PATCH 6/6] SQUASHME pnfs-submit: move layoutcommit to nfs_write_inode

[andros]

From: Andy Adamson <andros@netapp.com>

The LAYOUTCOMMIT call indicates an update to the file meta data is needed,
and should be called when clearing the I_DIRTY_SYNC state.

A call to LAYOUTCOMMIT in nfs_write_inode replaces the calls in nfs_wb_all,
nfs_commit_inode, and __nfs4_close.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4state.c |    2 --
 fs/nfs/write.c     |   22 ++++++++++------------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index d0dbdd4..e1207fa 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -589,8 +589,6 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state, fmode_t fm
 #ifdef CONFIG_NFS_V4_1
 		struct nfs_inode *nfsi = NFS_I(state->inode);
 
-		if (layoutcommit_needed(nfsi))
-			pnfs_layoutcommit_inode(state->inode, wait);
 		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
 			struct nfs4_pnfs_layout_segment range;
 
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 0fd33cb..875d07f 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1489,13 +1489,6 @@ static int nfs_commit_inode(struct inode *inode, int how)
 			wait_on_bit(&NFS_I(inode)->flags, NFS_INO_COMMIT,
 					nfs_wait_bit_killable,
 					TASK_KILLABLE);
-#ifdef CONFIG_NFS_V4_1
-		if (may_wait && layoutcommit_needed(NFS_I(inode))) {
-			error = pnfs_layoutcommit_inode(inode, 1);
-			if (error < 0)
-				return error;
-		}
-#endif /* CONFIG_NFS_V4_1 */
 	} else
 		nfs_commit_clear_lock(NFS_I(inode));
 out:
@@ -1545,7 +1538,16 @@ static int nfs_commit_unstable_pages(struct inode *inode, struct writeback_contr
 
 int nfs_write_inode(struct inode *inode, struct writeback_control *wbc)
 {
-	return nfs_commit_unstable_pages(inode, wbc);
+	int ret;
+	ret = nfs_commit_unstable_pages(inode, wbc);
+#ifdef CONFIG_NFS_V4_1
+	if (ret >= 0 && layoutcommit_needed(NFS_I(inode))) {
+		int err = pnfs_layoutcommit_inode(inode, 1);
+		if (err < 0)
+			ret = err;
+	}
+#endif /* CONFIG_NFS_V4_1 */
+	return ret;
 }
 
 /*
@@ -1562,10 +1564,6 @@ int nfs_wb_all(struct inode *inode)
 	};
 
 	ret = sync_inode(inode, &wbc);
-#ifdef CONFIG_NFS_V4_1
-	if (!ret && layoutcommit_needed(NFS_I(inode)))
-		ret = pnfs_layoutcommit_inode(inode, 1);
-#endif
 	return ret;
 }
 
-- 
1.6.6

Re: [PATCH 6/6] SQUASHME pnfs-submit: move layoutcommit to nfs_write_inode

[Boaz Harrosh]

On 06/02/2010 06:54 PM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> The LAYOUTCOMMIT call indicates an update to the file meta data is needed,
> and should be called when clearing the I_DIRTY_SYNC state.
> 
> A call to LAYOUTCOMMIT in nfs_write_inode replaces the calls in nfs_wb_all,
> nfs_commit_inode, and __nfs4_close.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/nfs4state.c |    2 --
>  fs/nfs/write.c     |   22 ++++++++++------------
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index d0dbdd4..e1207fa 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -589,8 +589,6 @@ static void __nfs4_close(struct path *path, struct nfs4_state *state, fmode_t fm
>  #ifdef CONFIG_NFS_V4_1
>  		struct nfs_inode *nfsi = NFS_I(state->inode);
>  
> -		if (layoutcommit_needed(nfsi))
> -			pnfs_layoutcommit_inode(state->inode, wait);
>  		if (has_layout(nfsi) && nfsi->layout.roc_iomode) {
>  			struct nfs4_pnfs_layout_segment range;
>  
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 0fd33cb..875d07f 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1489,13 +1489,6 @@ static int nfs_commit_inode(struct inode *inode, int how)
>  			wait_on_bit(&NFS_I(inode)->flags, NFS_INO_COMMIT,
>  					nfs_wait_bit_killable,
>  					TASK_KILLABLE);
> -#ifdef CONFIG_NFS_V4_1
> -		if (may_wait && layoutcommit_needed(NFS_I(inode))) {
> -			error = pnfs_layoutcommit_inode(inode, 1);
> -			if (error < 0)
> -				return error;
> -		}
> -#endif /* CONFIG_NFS_V4_1 */
>  	} else
>  		nfs_commit_clear_lock(NFS_I(inode));
>  out:
> @@ -1545,7 +1538,16 @@ static int nfs_commit_unstable_pages(struct inode *inode, struct writeback_contr
>  
>  int nfs_write_inode(struct inode *inode, struct writeback_control *wbc)
>  {
> -	return nfs_commit_unstable_pages(inode, wbc);
> +	int ret;
> +	ret = nfs_commit_unstable_pages(inode, wbc);
> +#ifdef CONFIG_NFS_V4_1
> +	if (ret >= 0 && layoutcommit_needed(NFS_I(inode))) {
> +		int err = pnfs_layoutcommit_inode(inode, 1);
> +		if (err < 0)
> +			ret = err;
> +	}
> +#endif /* CONFIG_NFS_V4_1 */

Here you should do better. Now that it's so easy for sure.

define an inline called pnfs_layoutcommit_inode that does
the if() and calls a _pnfs_layoutcommit_inode(). The later
is today's original function.

In header define that inline for the pnfs case and define
an empty one returning 0 for the not CONFIG_NFS_V4_1 case.

I had a patch like that but after your patch it is much
easier and makes more sense.

I would fold it into this patch.

Boaz
> +	return ret;
>  }
>  
>  /*
> @@ -1562,10 +1564,6 @@ int nfs_wb_all(struct inode *inode)
>  	};
>  
>  	ret = sync_inode(inode, &wbc);
> -#ifdef CONFIG_NFS_V4_1
> -	if (!ret && layoutcommit_needed(NFS_I(inode)))
> -		ret = pnfs_layoutcommit_inode(inode, 1);
> -#endif
>  	return ret;
>  }
>  

Re: [PATCH 4/6] SQUASHME pnfs-submit: cleanup layoutcommit call

[Boaz Harrosh]

On 06/02/2010 06:54 PM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> pnfs_layoutcommit_inode prints its status upon exit.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/pnfs.c |   11 ++---------
>  1 files changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index def27f8..91e944a 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -766,15 +766,8 @@ _pnfs_return_layout(struct inode *ino, struct nfs4_pnfs_layout_segment *range,
>  				!pnfs_return_layout_barrier(nfsi, &arg));
>  		}
>  
> -		if (layoutcommit_needed(nfsi)) {
> -			status = pnfs_layoutcommit_inode(ino, wait);
> -			if (status) {
> -				dprintk("%s: layoutcommit failed, status=%d. "
> -					"Returning layout anyway\n",
> -					__func__, status);
> -				status = 0;
> -			}
> -		}
> +		if (layoutcommit_needed(nfsi))
> +			pnfs_layoutcommit_inode(ino, wait);

The print was not for the fail but for the status ignored by
layout_return. But ye we can do with out.

But please leave a big comment why we ignore the status from
pnfs_layoutcommit_inode.

Boaz
>  	}
>  send_return:
>  	status = return_layout(ino, &arg, stateid, type, lo, wait);

[PATCH 0/6] pnfs-submit cleanup layoutcommit for file layout

[Andy Adamson]

This time sent with the subject....

On Jun 2, 2010, at 11:54 AM, andros@netapp.com wrote:

>
> This is against the pnfs-submit branch of the 2.6.34 tree. They will  
> need to be
> applied against the 2.6.35-rc1 tree which I can do after comments.
>
> RFC: I would like comments, especially on
> 0006-SQUASHME-pnfs-submit-move-layoutcommit-to-nfs_write_.patch.
>
> Remove unused layoutcommit layoutdriver_io_operations. Will be  
> restored
> in post-submit patches
> 0001-SQUASHME-pnfs-submit-remove-setup_layoutcommit.patch
> 0002-SQUASHNE-pnfs-submit-remove-cleanup_layoutcommit.patch
> 0003-SQUASHME-pnfs-submit-remove-encode_layoutcommit.patch
>
> A cleanup, and call the async error handler.
> 0004-SQUASHME-pnfs-submit-cleanup-layoutcommit-call.patch
> 0005-SQUASHME-pnfs-submit-handle-async-layoutcommit-error.patch
>
> This next  patch moves the pnfs_layoutcommit_inode call to  
> nfs_write_inode,
> and it is the only call other than in layoutreturn. (removed calls in
> __nfs4_close, nfs_commit_inode, nfs_wb_sync).
>
> This is fine for the file layout, and I think it's OK for the object  
> and
> block layouts as well.
>
> I left the LAYOUTCOMMIT call in nfs_write_inode a synchronous call,  
> because
> nfs_commit_unstable_pages sets the FLUSH_SYNC flag. Should this
> be an asyc LAYOUTCOMMIT call?
>
> pnfs_layoutcommit_inode is called after nfs_commit_unstable_pages()  
> so that
> if LAYOUTCOMMIT fails, the unstable pages have been processed..
>
> The error handlers (sync and async) call nfs4_map_errors, so unhandled
> errors (such as NFS4ERR_BADLAYOUT) get returned to nfs_write_ioode  
> as -EIO.
>
> Examining the write_inode call paths, I could not see where the -EIO  
> would
> be passed back to the application.  Testing with pynfs which I
> had return NFS4ERR_BADLAYOUT to the layout commit call, shows the - 
> EIO return
> not stopping the client nor is the error reported back to the  
> application.
>
> We will add code to the error handlers for errors such as  
> NFS4ERR_BADLAYOUT
> that require us to stop using and free the layout, and redo the I/O  
> through
> the MDS.
>
> Anyway, review is much appreciated.
>
> 0006-SQUASHME-pnfs-submit-move-layoutcommit-to-nfs_write_.patch
>
> Testing:
> With CONFIG_NFS_V4_1 set
> NFSv4.1/pnfs passed Connectathon against write enabled GFS2/pNFS.  
> Note: there
> were exactly the same number of LAYOUTCOMMITS sent as were sent with
> pnfs_layoutcommit_inode being called from __nfs4_close (never  
> happened),
> nfs_commit_inode and nfs_wb_sync.
>
> Passed Connectathon general test against pynfs file layout server with
> the NFS4ERR_BADLAYOUT being returned on every third LAYOUTCOMMIT.
>
>
> -->Andy
>
> --
> 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  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] pnfs-submit cleanup layoutcommit for file layout

[Boaz Harrosh]

On 06/02/2010 06:54 PM, andros@netapp.com wrote:
> This is against the pnfs-submit branch of the 2.6.34 tree. They will need to be
> applied against the 2.6.35-rc1 tree which I can do after comments.
> 
> RFC: I would like comments, especially on
> 0006-SQUASHME-pnfs-submit-move-layoutcommit-to-nfs_write_.patch.
> 
> Remove unused layoutcommit layoutdriver_io_operations. Will be restored
> in post-submit patches
> 0001-SQUASHME-pnfs-submit-remove-setup_layoutcommit.patch
> 0002-SQUASHNE-pnfs-submit-remove-cleanup_layoutcommit.patch

These two should be combined. The cleanup_ is to clean after
what's done in setup_.

> 0003-SQUASHME-pnfs-submit-remove-encode_layoutcommit.patch
> 

For example objects can do with this one only

> A cleanup, and call the async error handler.
> 0004-SQUASHME-pnfs-submit-cleanup-layoutcommit-call.patch
> 0005-SQUASHME-pnfs-submit-handle-async-layoutcommit-error.patch
> 
> This next  patch moves the pnfs_layoutcommit_inode call to nfs_write_inode,
> and it is the only call other than in layoutreturn. (removed calls in
> __nfs4_close, nfs_commit_inode, nfs_wb_sync).
> 
> This is fine for the file layout, and I think it's OK for the object and
> block layouts as well.
> 

It sounds very nice. It might have problems though. On the NFS_STABLE path
again. Because of this stupid thing I found that when returning NFS_STABLE
from writes, and no commits are called, then the internal i_size does not
get updated until after the layout commit has returned and the client detects
a change_attr on server. (Even if it was this client that caused the update)

But this should be fixed regardless. And currently I'm running with
commits on in objlayout. (Which reminds me to send the patch to Benny)

So yes I like this change a lot. It makes tons of sense to me as well.

> I left the LAYOUTCOMMIT call in nfs_write_inode a synchronous call, because
> nfs_commit_unstable_pages sets the FLUSH_SYNC flag. Should this
> be an asyc LAYOUTCOMMIT call?
> 

look at the struct writeback_control *wbc received, it has a flag which states
if this is sync or async do according to that flag. (Tell me if you don't find it)

> pnfs_layoutcommit_inode is called after nfs_commit_unstable_pages() so that
> if LAYOUTCOMMIT fails, the unstable pages have been processed..
> 
> The error handlers (sync and async) call nfs4_map_errors, so unhandled
> errors (such as NFS4ERR_BADLAYOUT) get returned to nfs_write_ioode as -EIO.
> 
> Examining the write_inode call paths, I could not see where the -EIO would
> be passed back to the application.  Testing with pynfs which I
> had return NFS4ERR_BADLAYOUT to the layout commit call, shows the -EIO return
> not stopping the client nor is the error reported back to the application.
> 
> We will add code to the error handlers for errors such as NFS4ERR_BADLAYOUT 
> that require us to stop using and free the layout, and redo the I/O through
> the MDS.
> 
> Anyway, review is much appreciated.
> 
> 0006-SQUASHME-pnfs-submit-move-layoutcommit-to-nfs_write_.patch
> 
> Testing:
> With CONFIG_NFS_V4_1 set
> NFSv4.1/pnfs passed Connectathon against write enabled GFS2/pNFS. Note: there
> were exactly the same number of LAYOUTCOMMITS sent as were sent with
> pnfs_layoutcommit_inode being called from __nfs4_close (never happened),
> nfs_commit_inode and nfs_wb_sync.
> 
> Passed Connectathon general test against pynfs file layout server with
> the NFS4ERR_BADLAYOUT being returned on every third LAYOUTCOMMIT.
> 

Andy you got this patchset all backwards. And they are not a set.

4,5,6 are to go in first and are intended for the full tree
and the .34 and .33 backport tree's as well. If I want to test
with them I'll need them stand alone un-conflicting.

Then 1+2,3 are something else and should be done on top of these above.
If they are self sustained and could be re applied on the to of the tree
as patch -R, then grate. If not then a "bring them back patch" could be
nice. without them we can't test any of this.

> 
> -->Andy
> 

Boaz

Re: [PATCH 0/6] pnfs-submit cleanup layoutcommit for file layout

[William A. (Andy) Adamson]

On Thu, Jun 3, 2010 at 4:10 AM, Boaz Harrosh <bharrosh@panasas.com> wro=
te:
> On 06/02/2010 06:54 PM, andros@netapp.com wrote:
>> This is against the pnfs-submit branch of the 2.6.34 tree. They will=
 need to be
>> applied against the 2.6.35-rc1 tree which I can do after comments.
>>
>> RFC: I would like comments, especially on
>> 0006-SQUASHME-pnfs-submit-move-layoutcommit-to-nfs_write_.patch.
>>
>> Remove unused layoutcommit layoutdriver_io_operations. Will be resto=
red
>> in post-submit patches
>> 0001-SQUASHME-pnfs-submit-remove-setup_layoutcommit.patch
>> 0002-SQUASHNE-pnfs-submit-remove-cleanup_layoutcommit.patch
>
> These two should be combined. The cleanup_ is to clean after
> what's done in setup_.
>
>> 0003-SQUASHME-pnfs-submit-remove-encode_layoutcommit.patch
>>
>
> For example objects can do with this one only

OK - makes sense as all three get squashed into the same patch.

>
>> A cleanup, and call the async error handler.
>> 0004-SQUASHME-pnfs-submit-cleanup-layoutcommit-call.patch
>> 0005-SQUASHME-pnfs-submit-handle-async-layoutcommit-error.patch
>>
>> This next =A0patch moves the pnfs_layoutcommit_inode call to nfs_wri=
te_inode,
>> and it is the only call other than in layoutreturn. (removed calls i=
n
>> __nfs4_close, nfs_commit_inode, nfs_wb_sync).
>>
>> This is fine for the file layout, and I think it's OK for the object=
 and
>> block layouts as well.
>>
>
> It sounds very nice. It might have problems though. On the NFS_STABLE=
 path
> again. Because of this stupid thing I found that when returning NFS_S=
TABLE
> from writes, and no commits are called, then the internal i_size does=
 not
> get updated until after the layout commit has returned and the client=
 detects
> a change_attr on server. (Even if it was this client that caused the =
update)
>
> But this should be fixed regardless. And currently I'm running with
> commits on in objlayout. (Which reminds me to send the patch to Benny=
)
>
> So yes I like this change a lot. It makes tons of sense to me as well=
=2E

Good.

>
>> I left the LAYOUTCOMMIT call in nfs_write_inode a synchronous call, =
because
>> nfs_commit_unstable_pages sets the FLUSH_SYNC flag. Should this
>> be an asyc LAYOUTCOMMIT call?
>>
>
> look at the struct writeback_control *wbc received, it has a flag whi=
ch states
> if this is sync or async do according to that flag. (Tell me if you d=
on't find it)

OK, thanks.

>
>> pnfs_layoutcommit_inode is called after nfs_commit_unstable_pages() =
so that
>> if LAYOUTCOMMIT fails, the unstable pages have been processed..
>>
>> The error handlers (sync and async) call nfs4_map_errors, so unhandl=
ed
>> errors (such as NFS4ERR_BADLAYOUT) get returned to nfs_write_ioode a=
s -EIO.
>>
>> Examining the write_inode call paths, I could not see where the -EIO=
 would
>> be passed back to the application. =A0Testing with pynfs which I
>> had return NFS4ERR_BADLAYOUT to the layout commit call, shows the -E=
IO return
>> not stopping the client nor is the error reported back to the applic=
ation.
>>
>> We will add code to the error handlers for errors such as NFS4ERR_BA=
DLAYOUT
>> that require us to stop using and free the layout, and redo the I/O =
through
>> the MDS.
>>
>> Anyway, review is much appreciated.
>>
>> 0006-SQUASHME-pnfs-submit-move-layoutcommit-to-nfs_write_.patch
>>
>> Testing:
>> With CONFIG_NFS_V4_1 set
>> NFSv4.1/pnfs passed Connectathon against write enabled GFS2/pNFS. No=
te: there
>> were exactly the same number of LAYOUTCOMMITS sent as were sent with
>> pnfs_layoutcommit_inode being called from __nfs4_close (never happen=
ed),
>> nfs_commit_inode and nfs_wb_sync.
>>
>> Passed Connectathon general test against pynfs file layout server wi=
th
>> the NFS4ERR_BADLAYOUT being returned on every third LAYOUTCOMMIT.
>>
>
> Andy you got this patchset all backwards. And they are not a set.
>
> 4,5,6 are to go in first and are intended for the full tree
> and the .34 and .33 backport tree's as well. If I want to test
> with them I'll need them stand alone un-conflicting.

Sure.

>
> Then 1+2,3 are something else and should be done on top of these abov=
e.
> If they are self sustained and could be re applied on the to of the t=
ree
> as patch -R, then grate. If not then a "bring them back patch" could =
be
> nice. without them we can't test any of this

Thanks for the review. I'll resend as requested so that you can test..

-->Andy

>
>>
>> -->Andy
>>
>
> Boaz
> --
> 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
>