[PATCH 0/5] pnfs DLM cluster only use read iomode layouts

[PATCH 0/5] pnfs DLM cluster only use read iomode layouts

[andros]

Applies against 2.6.33-rc6 pnfs-all branch.

In a DLM cluster, writing to a node other than the node where the open call
occurred and where meta data is cached will have performance implications when
the write causes meta data changes that need to be propagated to the open call
node.

Return NFS4ERR_BADIOMODE for LAYOUTGET requests with LAYOUTIOMODE4_RW iomode.

Plus a couple of error code fixes.

0001-pnfsd-fix-NFS4ERR_BADIOMODE-in-layoutget.patch
0002-pnfsd-fix-pnfs_export_operations-layoutget-valid-err.patch
0003-pnfsd-DLM-file-layout-only-support-read-iomode-layou.patch
0004-pnfsd-fix-DLM-file-layout-no-device-return.patch

Allow the client to fail a RW iomode layout and still grab a RO iomode layout.
0005-pnfs-set-failed-layout-bit-per-iomode.patch


Testing: Connectathon tests pass. Basic test shows RW iomode layout for
'bigfile' fails with NFS4ERR_BADIOMODE with writes going through the MDS,
and RO layout for 'bigfile' is obtained with reads going to the DS.


-->Andy

[PATCH 1/5] pnfsd: fix NFS4ERR_BADIOMODE in layoutget

[andros]

From: Andy Adamson <andros@netapp.com>

If an invalid iomode, or an iomode of LAYOUTIOMODE4_ANY is specified, the
metadata server MUST return NFS4ERR_BADIOMODE.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 769628f..575e1b6 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1114,21 +1114,14 @@ nfsd4_layoutget(struct svc_rqst *rqstp,
 	if (status)
 		goto out;
 
-	status = nfserr_inval;
+	status = nfserr_badiomode;
 	if (lgp->lg_seg.iomode != IOMODE_READ &&
-	    lgp->lg_seg.iomode != IOMODE_RW &&
-	    lgp->lg_seg.iomode != IOMODE_ANY) {
+	    lgp->lg_seg.iomode != IOMODE_RW) {
 		dprintk("pNFS %s: invalid iomode %d\n", __func__,
 			lgp->lg_seg.iomode);
 		goto out;
 	}
 
-	status = nfserr_badiomode;
-	if (lgp->lg_seg.iomode == IOMODE_ANY) {
-		dprintk("pNFS %s: IOMODE_ANY is not allowed\n", __func__);
-		goto out;
-	}
-
 	/* Set up arguments so layout can be retrieved at encode time */
 	lgp->lg_fhp = current_fh;
 	copy_clientid((clientid_t *)&lgp->lg_seg.clientid, cstate->session);
-- 
1.6.6

[PATCH 2/5] pnfsd: fix pnfs_export_operations layoutget valid errors

[andros]

From: Andy Adamson <andros@netapp.com>

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfsd/nfs4pnfsd.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
index 816e2f0..3951e02 100644
--- a/fs/nfsd/nfs4pnfsd.c
+++ b/fs/nfsd/nfs4pnfsd.c
@@ -870,6 +870,7 @@ nfs4_pnfs_get_layout(struct nfsd4_pnfs_layoutget *lgp,
 	if (status) {
 		switch (status) {
 		case -ETOOSMALL:
+		case -E2BIG:
 			status = nfserr_toosmall;
 			break;
 		case -ENOMEM:
@@ -878,10 +879,7 @@ nfs4_pnfs_get_layout(struct nfsd4_pnfs_layoutget *lgp,
 			status = nfserr_layouttrylater;
 			break;
 		case -ENOENT:
-			status = nfserr_badlayout;
-			break;
-		case -E2BIG:
-			status = nfserr_toosmall;
+			status = nfserr_stale;
 			break;
 		default:
 			status = nfserr_layoutunavailable;
-- 
1.6.6

[PATCH 3/5] pnfsd: DLM file layout only support read iomode layouts

[andros]

From: Andy Adamson <andros@netapp.com>

In a DLM cluster, writing to a node other than the node where the open call
occurred and where meta data is cached will have performance implications when
the write causes meta data changes that need to be propagated to the open call
node.

DlM clusters support only LAYOUTIOMODE4_READ layouts. Writes will go through
the MDS.

Return NFS4ERR_BADIOMODE for LAYOUTGET requests with LAYOUTIOMODE4_RW iomode.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfsd/nfs4pnfsd.c   |    2 ++
 fs/nfsd/nfs4pnfsdlm.c |    5 +++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
index 3951e02..e1ecf81 100644
--- a/fs/nfsd/nfs4pnfsd.c
+++ b/fs/nfsd/nfs4pnfsd.c
@@ -881,6 +881,8 @@ nfs4_pnfs_get_layout(struct nfsd4_pnfs_layoutget *lgp,
 		case -ENOENT:
 			status = nfserr_stale;
 			break;
+		case nfserr_badiomode:
+			break;
 		default:
 			status = nfserr_layoutunavailable;
 		}
diff --git a/fs/nfsd/nfs4pnfsdlm.c b/fs/nfsd/nfs4pnfsdlm.c
index 293bf95..8cc7c84 100644
--- a/fs/nfsd/nfs4pnfsdlm.c
+++ b/fs/nfsd/nfs4pnfsdlm.c
@@ -28,6 +28,7 @@
 #include <linux/nfsd/nfs4layoutxdr.h>
 
 #include "nfsfh.h"
+#include "nfsd.h"
 
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
@@ -331,6 +332,10 @@ static int nfsd4_pnfs_dlm_layoutget(struct inode *inode,
 
 	dprintk("%s: LAYOUT_GET\n", __func__);
 
+	/* DLM exported file systems only support layouts for READ */
+	if (res->lg_seg.iomode == IOMODE_RW)
+		return nfserr_badiomode;
+
 	index = dlm_ino_hash(inode);
 	dprintk("%s first stripe index %d i_ino %lu\n", __func__, index,
 		inode->i_ino);
-- 
1.6.6

[PATCH 4/5] pnfsd: fix DLM file layout no device return

[andros]

From: Andy Adamson <andros@netapp.com>

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

diff --git a/fs/nfsd/nfs4pnfsdlm.c b/fs/nfsd/nfs4pnfsdlm.c
index 8cc7c84..83c4698 100644
--- a/fs/nfsd/nfs4pnfsdlm.c
+++ b/fs/nfsd/nfs4pnfsdlm.c
@@ -316,7 +316,7 @@ static int dlm_ino_hash(struct inode *ino)
 	 */
 	de = nfsd4_find_pnfs_dlm_device(ino->i_sb->s_bdev->bd_disk->disk_name);
 	if (!de)
-		return -EINVAL;
+		return -1;
 	hash_mask = de->num_ds - 1;
 	return ino->i_ino & hash_mask;
 }
@@ -340,7 +340,7 @@ static int nfsd4_pnfs_dlm_layoutget(struct inode *inode,
 	dprintk("%s first stripe index %d i_ino %lu\n", __func__, index,
 		inode->i_ino);
 	if (index < 0)
-		return index;
+		return nfserr_layoutunavailable;
 
 	res->lg_seg.layout_type = LAYOUT_NFSV4_FILES;
 	/* Always give out whole file layouts */
-- 
1.6.6

[PATCH 5/5] pnfs: set failed layout bit per iomode

[andros]

From: Andy Adamson <andros@netapp.com>

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

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 3a05422..596cfce 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1119,11 +1119,11 @@ pnfs_update_layout(struct inode *ino,
 	}
 
 	/* if get layout already failed once goto out */
-	if (test_bit(NFS_INO_LAYOUT_FAILED, &nfsi->pnfs_layout_state)) {
+	if (test_bit(lo_fail_bit(iomode), &nfsi->pnfs_layout_state)) {
 		if (unlikely(nfsi->pnfs_layout_suspend &&
 		    get_seconds() >= nfsi->pnfs_layout_suspend)) {
 			dprintk("%s: layout_get resumed\n", __func__);
-			clear_bit(NFS_INO_LAYOUT_FAILED,
+			clear_bit(lo_fail_bit(iomode),
 				  &nfsi->pnfs_layout_state);
 			nfsi->pnfs_layout_suspend = 0;
 		} else {
@@ -1247,7 +1247,7 @@ pnfs_get_layout_done(struct nfs4_pnfs_layoutget *lgp, int rpc_status)
 get_out:
 	/* remember that get layout failed and suspend trying */
 	nfsi->pnfs_layout_suspend = suspend;
-	set_bit(NFS_INO_LAYOUT_FAILED, &nfsi->pnfs_layout_state);
+	set_bit(lo_fail_bit(lgp->args.lseg.iomode), &nfsi->pnfs_layout_state);
 	dprintk("%s: layout_get suspended until %ld\n",
 		__func__, suspend);
 out:
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 379e67d..312c4ef 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -94,6 +94,12 @@ void pnfs_set_layout_stateid(struct pnfs_layout_type *lo,
 				     (srv)->pnfs_curr_ld->ld_policy_ops && \
 				     (srv)->pnfs_curr_ld->ld_policy_ops->opname)
 
+static inline int lo_fail_bit(u32 iomode)
+{
+	return iomode == IOMODE_RW ?
+			 NFS_INO_RW_LAYOUT_FAILED : NFS_INO_RO_LAYOUT_FAILED;
+}
+
 /* Return true if a layout driver is being used for this mountpoint */
 static inline int pnfs_enabled_sb(struct nfs_server *nfss)
 {
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index fc0990a..dd57a6a 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -201,8 +201,9 @@ struct nfs_inode {
 	struct list_head	lo_inodes;
 
 	unsigned long pnfs_layout_state;
-#define NFS_INO_LAYOUT_FAILED	0	/* get layout failed, stop trying */
-#define NFS_INO_LAYOUT_ALLOC	1	/* bit lock for layout allocation */
+#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 */
 	time_t pnfs_layout_suspend;
 	wait_queue_head_t lo_waitq;
 	spinlock_t lo_lock;
-- 
1.6.6

Re: [pnfs] [PATCH 4/5] pnfsd: fix DLM file layout no device return

[Boaz Harrosh]

On 02/05/2010 07:10 PM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfsd/nfs4pnfsdlm.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4pnfsdlm.c b/fs/nfsd/nfs4pnfsdlm.c
> index 8cc7c84..83c4698 100644
> --- a/fs/nfsd/nfs4pnfsdlm.c
> +++ b/fs/nfsd/nfs4pnfsdlm.c
> @@ -316,7 +316,7 @@ static int dlm_ino_hash(struct inode *ino)
>  	 */
>  	de = nfsd4_find_pnfs_dlm_device(ino->i_sb->s_bdev->bd_disk->disk_name);
>  	if (!de)
> -		return -EINVAL;
> +		return -1;
>  	hash_mask = de->num_ds - 1;
>  	return ino->i_ino & hash_mask;
>  }
> @@ -340,7 +340,7 @@ static int nfsd4_pnfs_dlm_layoutget(struct inode *inode,
>  	dprintk("%s first stripe index %d i_ino %lu\n", __func__, index,
>  		inode->i_ino);
>  	if (index < 0)
> -		return index;
> +		return nfserr_layoutunavailable;

You are relaying on a side effect that undefined errors
are nfserr_layoutunavailable. That's not so noble ;)

I wish we would only allow nfserr_* and your first patch
defines the allowed returns from layoutget

(I can fix exofs, easily, if we just agree on protocol)

Thanks
Boaz
>  
>  	res->lg_seg.layout_type = LAYOUT_NFSV4_FILES;
>  	/* Always give out whole file layouts */

Re: [pnfs] [PATCH 3/5] pnfsd: DLM file layout only support read iomode layouts

[Boaz Harrosh]

On 02/05/2010 07:10 PM, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> In a DLM cluster, writing to a node other than the node where the open call
> occurred and where meta data is cached will have performance implications when
> the write causes meta data changes that need to be propagated to the open call
> node.
> 
> DlM clusters support only LAYOUTIOMODE4_READ layouts. Writes will go through
> the MDS.
> 
> Return NFS4ERR_BADIOMODE for LAYOUTGET requests with LAYOUTIOMODE4_RW iomode.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfsd/nfs4pnfsd.c   |    2 ++
>  fs/nfsd/nfs4pnfsdlm.c |    5 +++++
>  2 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
> index 3951e02..e1ecf81 100644
> --- a/fs/nfsd/nfs4pnfsd.c
> +++ b/fs/nfsd/nfs4pnfsd.c
> @@ -881,6 +881,8 @@ nfs4_pnfs_get_layout(struct nfsd4_pnfs_layoutget *lgp,
>  		case -ENOENT:
>  			status = nfserr_stale;
>  			break;
> +		case nfserr_badiomode:
> +			break;

2 questions:

- Why In this patch? - The first patch already fixes up what is allowed
  and what is not. It is more clear to establish protocol in one place
  so we can review.

- Why only this one? Why not all the nfserr_xxx allowed/make-sense, in
  a layoutget operation?

In my opinion we should only allow nfserr_xxx constants and be done with it
It should not be hard to fix all users.

Boaz
>  		default:
>  			status = nfserr_layoutunavailable;
>  		}
> diff --git a/fs/nfsd/nfs4pnfsdlm.c b/fs/nfsd/nfs4pnfsdlm.c
> index 293bf95..8cc7c84 100644
> --- a/fs/nfsd/nfs4pnfsdlm.c
> +++ b/fs/nfsd/nfs4pnfsdlm.c
> @@ -28,6 +28,7 @@
>  #include <linux/nfsd/nfs4layoutxdr.h>
>  
>  #include "nfsfh.h"
> +#include "nfsd.h"
>  
>  #define NFSDDBG_FACILITY                NFSDDBG_PROC
>  
> @@ -331,6 +332,10 @@ static int nfsd4_pnfs_dlm_layoutget(struct inode *inode,
>  
>  	dprintk("%s: LAYOUT_GET\n", __func__);
>  
> +	/* DLM exported file systems only support layouts for READ */
> +	if (res->lg_seg.iomode == IOMODE_RW)
> +		return nfserr_badiomode;
> +
>  	index = dlm_ino_hash(inode);
>  	dprintk("%s first stripe index %d i_ino %lu\n", __func__, index,
>  		inode->i_ino);

Re: [pnfs] [PATCH 2/5] pnfsd: fix pnfs_export_operations layoutget valid errors

[Benny Halevy]

On Feb. 05, 2010, 19:10 +0200, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfsd/nfs4pnfsd.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
> index 816e2f0..3951e02 100644
> --- a/fs/nfsd/nfs4pnfsd.c
> +++ b/fs/nfsd/nfs4pnfsd.c
> @@ -870,6 +870,7 @@ nfs4_pnfs_get_layout(struct nfsd4_pnfs_layoutget *lgp,
>  	if (status) {
>  		switch (status) {
>  		case -ETOOSMALL:
> +		case -E2BIG:

Should we allow the filesystem to return nfs errors?
Or even require it to do so?

This can be done by adding cases for the
valid error values for LAYOUTGET in this switch statement.

Benny

>  			status = nfserr_toosmall;
>  			break;
>  		case -ENOMEM:
> @@ -878,10 +879,7 @@ nfs4_pnfs_get_layout(struct nfsd4_pnfs_layoutget *lgp,

		case nfserr_layouttrylater:

>  			status = nfserr_layouttrylater;
>  			break;
>  		case -ENOENT:
> -			status = nfserr_badlayout;
> -			break;
> -		case -E2BIG:
> -			status = nfserr_toosmall;
> +			status = nfserr_stale;
>  			break;
>  		default:
>  			status = nfserr_layoutunavailable;

Re: [pnfs] [PATCH 2/5] pnfsd: fix pnfs_export_operations layoutget valid errors

[Andy Adamson]

On Feb 7, 2010, at 4:05 AM, Benny Halevy wrote:

> On Feb. 05, 2010, 19:10 +0200, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>> fs/nfsd/nfs4pnfsd.c |    6 ++----
>> 1 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
>> index 816e2f0..3951e02 100644
>> --- a/fs/nfsd/nfs4pnfsd.c
>> +++ b/fs/nfsd/nfs4pnfsd.c
>> @@ -870,6 +870,7 @@ nfs4_pnfs_get_layout(struct  
>> nfsd4_pnfs_layoutget *lgp,
>> 	if (status) {
>> 		switch (status) {
>> 		case -ETOOSMALL:
>> +		case -E2BIG:
>
> Should we allow the filesystem to return nfs errors?
> Or even require it to do so?
>
> This can be done by adding cases for the
> valid error values for LAYOUTGET in this switch statement.

OK. From re-reading all the past mail and the comments on this latest  
patch set:

1) We want a limited number of well documented error returns -  
documented as part of the filesystem API not just the spec.

2) Start with just the errors that we know we need for gfs2 and exofs,  
and expand the list later as necessary.

3) Allow only nfserr_xxxx errors.


Does this address the comments? Do you want more documentation, if so,  
where?

         if (status) {
+               /*
+                * The allowable error codes for the layout_get pNFS  
export
+                * operations vector function can be expanded as needed
+                * to include other errors defined for the LAYOUTGET  
pNFS
+                * operation.
+                */
+               case nfserr_badiomode:
+               case nfserr_badlayout:
+               case nfserr_layouttrylater:
+               case nfserr_layoutunavailable:
+               case nfserr_toosmall:
                         break;
                 default:
+                       BUG();
                 }
                 goto out_freelayout;

-->Andy

>
> Benny
>
>> 			status = nfserr_toosmall;
>> 			break;
>> 		case -ENOMEM:
>> @@ -878,10 +879,7 @@ nfs4_pnfs_get_layout(struct  
>> nfsd4_pnfs_layoutget *lgp,
>
> 		case nfserr_layouttrylater:
>
>> 			status = nfserr_layouttrylater;
>> 			break;
>> 		case -ENOENT:
>> -			status = nfserr_badlayout;
>> -			break;
>> -		case -E2BIG:
>> -			status = nfserr_toosmall;
>> +			status = nfserr_stale;
>> 			break;
>> 		default:
>> 			status = nfserr_layoutunavailable;
> --
> 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: [pnfs] [PATCH 2/5] pnfsd: fix pnfs_export_operations layoutget valid errors

[J. Bruce Fields]

On Mon, Feb 08, 2010 at 03:41:44PM -0500, Andy Adamson wrote:
>
> On Feb 7, 2010, at 4:05 AM, Benny Halevy wrote:
>
>> On Feb. 05, 2010, 19:10 +0200, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> fs/nfsd/nfs4pnfsd.c |    6 ++----
>>> 1 files changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
>>> index 816e2f0..3951e02 100644
>>> --- a/fs/nfsd/nfs4pnfsd.c
>>> +++ b/fs/nfsd/nfs4pnfsd.c
>>> @@ -870,6 +870,7 @@ nfs4_pnfs_get_layout(struct nfsd4_pnfs_layoutget 
>>> *lgp,
>>> 	if (status) {
>>> 		switch (status) {
>>> 		case -ETOOSMALL:
>>> +		case -E2BIG:
>>
>> Should we allow the filesystem to return nfs errors?
>> Or even require it to do so?
>>
>> This can be done by adding cases for the
>> valid error values for LAYOUTGET in this switch statement.
>
> OK. From re-reading all the past mail and the comments on this latest  
> patch set:
>
> 1) We want a limited number of well documented error returns -  
> documented as part of the filesystem API not just the spec.
>
> 2) Start with just the errors that we know we need for gfs2 and exofs,  
> and expand the list later as necessary.
>
> 3) Allow only nfserr_xxxx errors.
>
>
> Does this address the comments? Do you want more documentation, if so,  
> where?
>
>         if (status) {
> +               /*
> +                * The allowable error codes for the layout_get pNFS  
> export
> +                * operations vector function can be expanded as needed
> +                * to include other errors defined for the LAYOUTGET  
> pNFS
> +                * operation.
> +                */

Could you dump some description of the cases into the error numbers, if
it's not obvious?  E.g.:

> +               case nfserr_badiomode:
			/*
			 * filesystem doesn't want to give out a
			 * writeable layout in this case.
			 */
> +               case nfserr_badlayout:
> +               case nfserr_layouttrylater:
> +               case nfserr_layoutunavailable:
> +               case nfserr_toosmall:

just to summarize what the various filesystems are doing.

And don't worry about it being a little messy or verbose; we can clean
it up later.

--b.

Re: [pnfs] [PATCH 2/5] pnfsd: fix pnfs_export_operations layoutget valid errors

[Boaz Harrosh]

On 02/08/2010 10:41 PM, Andy Adamson wrote:
> 
> On Feb 7, 2010, at 4:05 AM, Benny Halevy wrote:
> 
>> On Feb. 05, 2010, 19:10 +0200, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> fs/nfsd/nfs4pnfsd.c |    6 ++----
>>> 1 files changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
>>> index 816e2f0..3951e02 100644
>>> --- a/fs/nfsd/nfs4pnfsd.c
>>> +++ b/fs/nfsd/nfs4pnfsd.c
>>> @@ -870,6 +870,7 @@ nfs4_pnfs_get_layout(struct  
>>> nfsd4_pnfs_layoutget *lgp,
>>> 	if (status) {
>>> 		switch (status) {
>>> 		case -ETOOSMALL:
>>> +		case -E2BIG:
>>
>> Should we allow the filesystem to return nfs errors?
>> Or even require it to do so?
>>
>> This can be done by adding cases for the
>> valid error values for LAYOUTGET in this switch statement.
> 
> OK. From re-reading all the past mail and the comments on this latest  
> patch set:
> 
> 1) We want a limited number of well documented error returns -  
> documented as part of the filesystem API not just the spec.
> 
> 2) Start with just the errors that we know we need for gfs2 and exofs,  
> and expand the list later as necessary.
> 
> 3) Allow only nfserr_xxxx errors.
> 
> 
> Does this address the comments? Do you want more documentation, if so,  
> where?
> 
>          if (status) {
> +               /*
> +                * The allowable error codes for the layout_get pNFS  
> export
> +                * operations vector function can be expanded as needed
> +                * to include other errors defined for the LAYOUTGET  
> pNFS
> +                * operation.
> +                */
> +               case nfserr_badiomode:
> +               case nfserr_badlayout:
> +               case nfserr_layouttrylater:
> +               case nfserr_layoutunavailable:
> +               case nfserr_toosmall:
>                          break;
>                  default:
> +                       BUG();
>                  }
>                  goto out_freelayout;
> 
> -->Andy
> 

Perfect. it's all exofs needs.

I've prepared a SQUASHME for pnfsd-exofs to return these.
I'll send it as soon as you send the renewed patches.

Thanks Andy
Boaz

>>
>> Benny
>>
>>> 			status = nfserr_toosmall;
>>> 			break;
>>> 		case -ENOMEM:
>>> @@ -878,10 +879,7 @@ nfs4_pnfs_get_layout(struct  
>>> nfsd4_pnfs_layoutget *lgp,
>>
>> 		case nfserr_layouttrylater:
>>
>>> 			status = nfserr_layouttrylater;
>>> 			break;
>>> 		case -ENOENT:
>>> -			status = nfserr_badlayout;
>>> -			break;
>>> -		case -E2BIG:
>>> -			status = nfserr_toosmall;
>>> +			status = nfserr_stale;
>>> 			break;
>>> 		default:
>>> 			status = nfserr_layoutunavailable;
>> --
>> 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
> 
> _______________________________________________
> pNFS mailing list
> pNFS@linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs

Re: [pnfs] [PATCH 2/5] pnfsd: fix pnfs_export_operations layoutget valid errors

[Benny Halevy]

On Feb. 08, 2010, 22:41 +0200, Andy Adamson <andros@netapp.com> wrote:
> On Feb 7, 2010, at 4:05 AM, Benny Halevy wrote:
> 
>> On Feb. 05, 2010, 19:10 +0200, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>> fs/nfsd/nfs4pnfsd.c |    6 ++----
>>> 1 files changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
>>> index 816e2f0..3951e02 100644
>>> --- a/fs/nfsd/nfs4pnfsd.c
>>> +++ b/fs/nfsd/nfs4pnfsd.c
>>> @@ -870,6 +870,7 @@ nfs4_pnfs_get_layout(struct  
>>> nfsd4_pnfs_layoutget *lgp,
>>> 	if (status) {
>>> 		switch (status) {
>>> 		case -ETOOSMALL:
>>> +		case -E2BIG:
>> Should we allow the filesystem to return nfs errors?
>> Or even require it to do so?
>>
>> This can be done by adding cases for the
>> valid error values for LAYOUTGET in this switch statement.
> 
> OK. From re-reading all the past mail and the comments on this latest  
> patch set:
> 
> 1) We want a limited number of well documented error returns -  
> documented as part of the filesystem API not just the spec.
> 
> 2) Start with just the errors that we know we need for gfs2 and exofs,  
> and expand the list later as necessary.
> 
> 3) Allow only nfserr_xxxx errors.
> 
> 
> Does this address the comments? Do you want more documentation, if so,  
> where?
> 
>          if (status) {
> +               /*
> +                * The allowable error codes for the layout_get pNFS  
> export
> +                * operations vector function can be expanded as needed
> +                * to include other errors defined for the LAYOUTGET  
> pNFS
> +                * operation.
> +                */
> +               case nfserr_badiomode:
> +               case nfserr_badlayout:
> +               case nfserr_layouttrylater:
> +               case nfserr_layoutunavailable:
> +               case nfserr_toosmall:

Yup, I think that's the way to go.

>                          break;
>                  default:
> +                       BUG();

There are some generic return values we should allow:
NFS4ERR_ACCESS, NFS4ERR_INVAL, NFS4ERR_IO, NFS4ERR_LOCKED, NFS4ERR_NOSPC,
NFS4ERR_RECALLCONFLICT, NFS4ERR_SERVERFAULT

Benny

>                  }
>                  goto out_freelayout;
> 
> -->Andy
> 
>> Benny
>>
>>> 			status = nfserr_toosmall;
>>> 			break;
>>> 		case -ENOMEM:
>>> @@ -878,10 +879,7 @@ nfs4_pnfs_get_layout(struct  
>>> nfsd4_pnfs_layoutget *lgp,
>> 		case nfserr_layouttrylater:
>>
>>> 			status = nfserr_layouttrylater;
>>> 			break;
>>> 		case -ENOENT:
>>> -			status = nfserr_badlayout;
>>> -			break;
>>> -		case -E2BIG:
>>> -			status = nfserr_toosmall;
>>> +			status = nfserr_stale;
>>> 			break;
>>> 		default:
>>> 			status = nfserr_layoutunavailable;
>> --
>> 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
>