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

[Benny Halevy <[e-mail]>]

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" <[e-mail]> wrote:
> On Tue, Dec 15, 2009 at 03:03:54PM -0500, [e-mail] wrote:
>> From: Andy Adamson <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>  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
>>