Subject: Re: [nfsv4] [PATCH 1/1] pnfsd: DLM file layout only support read iomode layouts

From: Benny Halevy <[email-masked]>
Date: Wed, 16 Dec 2009 14:18:14 +0200

Bruce, I agree that the spec isn't clear about what to return
so I'm adding the ietf list to this thread.

In summary, we have a server that supports read-only layouts but
not read-write.  What should it return to a LAYOUTGET with LAYOUTIOMODE4_RW?

We see two possibilities that make sense: NFS4ERR_BADIOMODE and
NFS4ERR_LAYOUTUNAVAILABLE.

15.1.10.1.  NFS4ERR_BADIOMODE (Error Code 10049)

   An invalid or inappropriate layout iomode was specified.

15.1.10.4.  NFS4ERR_LAYOUTUNAVAILABLE (Error Code 10059)

   Returned when layouts are not available for the current file system
   or the particular specified file.

NFS4ERR_BADIOMODE seems to make more sense since layouts are available,
just not for the requested iomode. However its definition indicates that
the iomode in the client request is "invalid or inappropriate".
It is not clear what "inappropriate" means in this context.
I guess that the original intent was for clients who e.g. send a LAYOUTGET
with LAYOUTIOMODE4_ANY which is a valid iomode but inappropriate for LAYOUTGET.

To make the spec clearer we could say:

15.1.10.1.  NFS4ERR_BADIOMODE (Error Code 10049)

   An invalid, inappropriate, or unsupported layout iomode was specified.

18.43.  Operation 50: LAYOUTGET - Get Layout Information
18.43.3.  DESCRIPTION
iomode
...
+     If the metadata server does not support read/write layouts and
+     the value of the lo_iomode is LAYOUTIOMODE4_RW it MUST return
+     NFS4ERR_BADIOMODE.

Benny

On Dec. 16, 2009, 0:41 +0200, "J. Bruce Fields" <[email-masked]> wrote:
> On Tue, Dec 15, 2009 at 03:03:54PM -0500, [email-masked] wrote:
>> From: Andy Adamson <[email-masked]>
>>
>> In a DLM cluster, writing to a node other than the node where the open call
>> occurred will have performance implications due to meta data updates.
>>
>> Return only LAYOUTIOMODE4_READ layouts so that writes go through the MDS
>> LAYOUTGET with LAYOUTIOMODE4_RW iomodes will receive the NFS4ERR_BADIOMODE
>> error.
> 
> Hm.  The 4.1 draft does refer (in 13.8) to using LAYOUTIOMODE4_READ
> layouts for the read-only data-server case, but doesn't really give a
> clear error recommendation for it.  BADIOMODE says "An invalid or
> inappropriate layout iomode was specified".  The client definitely isn't
> making an *invalid* request.  I don't know what inappropriate means.
> 
> LAYOUTUNAVAILABLE might be better, though it doesn't mention this
> particular case: "Returned when layouts are not available for the
> current file system or the particular specified file."  So a client
> might reasonably take that to mean that it shouldn't ever ask for that
> file again.
> 
> I guess BADIOMODE wins, but the spec could be clearer.
> 
> --b.
> 
>> Pass the NFS4ERR_BADIOMODE error to the client - Don't map
>> pnfs_export_operation:layout_get() NFS4 errors.
>>
>> Signed-off-by: Andy Adamson <[email-masked]>
>> ---
>>  fs/nfsd/nfs4pnfsd.c   |    2 +-
>>  fs/nfsd/nfs4pnfsdlm.c |    4 ++++
>>  2 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4pnfsd.c b/fs/nfsd/nfs4pnfsd.c
>> index aa7abad..658a343 100644
>> --- a/fs/nfsd/nfs4pnfsd.c
>> +++ b/fs/nfsd/nfs4pnfsd.c
>> @@ -541,7 +541,7 @@ nfs4_pnfs_get_layout(struct nfsd4_pnfs_layoutget *lgp,
>>  		__func__, status, res.lg_seg.iomode,
>>  		res.lg_seg.offset, res.lg_seg.length);
>>  
>> -	if (status) {
>> +	if (status && status >= -1000) {
>>  		switch (status) {
>>  		case -ETOOSMALL:
>>  			status = nfserr_toosmall;
>> diff --git a/fs/nfsd/nfs4pnfsdlm.c b/fs/nfsd/nfs4pnfsdlm.c
>> index ed2e940..94ddba2 100644
>> --- a/fs/nfsd/nfs4pnfsdlm.c
>> +++ b/fs/nfsd/nfs4pnfsdlm.c
>> @@ -329,6 +329,10 @@ static int nfsd4_pnfs_dlm_layoutget(struct inode *inode,
>>  
>>  	dprintk("%s: LAYOUT_GET\n", __func__);
>>  
>> +	/* Due to DLM internode locking, only support read layouts */
>> +	if res->lg_seg.iomode != IOMODE_READ)
>> +		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.2.5
>>

---

From: "William A. (Andy) Adamson" <[email-masked]>
Date: Wed, 16 Dec 2009 10:26:57 -0500

>   An invalid or inappropriate layout iomode was specified.

The spec is fine. Inappropriate means the server does not want to
return a layout for that file with the requested iomode at this time.
It doesn't necessarily mean that the server won't return a layout with
that same iomode for a different file, or return a layout for the file
with the requested iomode at a later date.  It's totally up to the
server what is appropriate.
For what ever reason, the client has no choice but to try a different
iomode and/or
shoulder on via the MDS,

> It is not clear what "inappropriate" means in this context.

What difference does 'what inappropriate means' make to the client?
None! It's up to the server.

> with LAYOUTIOMODE4_ANY which is a valid iomode but inappropriate for LAYOUTGET.

The client specifies the iomode so that the server can decide whether
it is appropriate to return a layout or not. This is why
LAYOUTIOMODE4_ANY is not allowed on LAYOUTGET.

>   An invalid, inappropriate, or unsupported layout iomode was specified.

This is redundant. One meaning of inappropriate is unsupported.
Another reason is 'sorry, I'm re-striping, and this file has read-only
layouts now'. Another reason is ' it's Tuesday, and I don't hand out
layouts with iomode blah for file bleep on Tuesday. It's the server's
choice so the client just has to get over it and move on!

-->Andy

> _______________________________________________
> pNFS mailing list
> [email-masked]
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>

---

From: "J. Bruce Fields" <[email-masked]>
Date: Wed, 16 Dec 2009 10:37:40 -0500

> None! It's up to the server.

We do have to worry how clients will react to the error:

	- If clients really assume that this error can only result from
	  a bug, then they may choose to return an IO error to the
	  application, turn off pnfs entirely for this server, or
	  something similarly drastic.

	- If they assume it means that the server doesn't feel like
	  giving out a layout of that iomode right now, then we're good.

You're probably right that the latter is a reasonable interpretation of
"inappropriate".

However, the spec isn't entirely explicit about this, and the only
specific example given of BADIOMODE use is an "invalid" case (client
supplying IOMODE4_ANY on getlayout), so I think it would be easy for a
client to get this wrong.

--b.

---

From: "William A. (Andy) Adamson" <[email-masked]>
Date: Wed, 16 Dec 2009 10:42:59 -0500

>          something similarly drastic.

Then that client implementation won't be used for very long. This is
Darwin in action!

> client to get this wrong.

Perhaps. Linux doesn't. What does the only other client (Solaris) do?

-->Andy

---

From: Benny Halevy <[email-masked]>
Date: Wed, 16 Dec 2009 17:51:24 +0200

> Perhaps. Linux doesn't. What does the only other client (Solaris) do?

I'm not sure that matters much.  The fact that we got it wrong
and it wasn't obvious for us what error the server should have returned
is a pretty good indication that the spec isn't clear about it.

Benny

---

From: "William A. (Andy) Adamson" <[email-masked]>
Date: Wed, 16 Dec 2009 11:17:59 -0500

> is a pretty good indication that the spec isn't clear about it.

ummm - we got it right!

-->Andy

---

From: "J. Bruce Fields" <[email-masked]>
Date: Wed, 16 Dec 2009 11:30:16 -0500

>    An invalid, inappropriate, or unsupported layout iomode was specified.

Sounds OK to me.

A mention of BADIOMODE in 13.8 might help too:

	For example, if the server implementation supports reading from
	read-only replicas or mirrors, it would be useful for the server
	to return a layout enabling the client to do so.  As such, the
	client SHOULD set the iomode based on its intent to read or
	write the data.  In such a case, the server will respond
	NFS4ERR_BADIOMODE to a client that requests an iomode which it
	does not support for the given file.

But I don't know if it's too late for that kind of change.

Probably "invalid" was actually meant for iomode's other than those
defined by the spec, the word "inappropriate" was added to cover the
layoutget IOMODE4_ANY case, and the read-only data server case wasn't
considered.  I dunno.

--b.