Re: [PATCH v1] NFSD: Decode NFSv4 birth time attribute

[Chuck Lever III]

> On Jul 11, 2022, at 1:14 PM, Igor Mammedov <imammedo@xxxxxxxxxx> wrote:
> 
> On Sun, 10 Jul 2022 14:46:04 -0400
> Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> 
>> NFSD has advertised support for the NFSv4 time_create attribute
>> since commit e377a3e698fb ("nfsd: Add support for the birth time
>> attribute").
>> 
>> Igor Mammedov reports that Mac OS clients attempt to set the NFSv4
>> birth time attribute via OPEN(CREATE) and SETATTR if the server
>> indicates that it supports it, but since the above commit was
>> merged, those attempts now fail.
>> 
>> Table 5 in RFC 8881 lists the time_create attribute as one that can
>> be both set and retrieved, but the above commit did not add server
>> support for clients to provide a time_create attribute. IMO that's
>> a bug in our implementation of the NFSv4 protocol, which this commit
>> addresses.
>> 
>> Whether NFSD silently ignores the new birth time or actually sets it
>> is another matter. I haven't found another filesystem service in the
>> Linux kernel that enables users or clients to modify a file's birth
>> time attribute.
>> 
>> This commit reflects my (perhaps incorrect) understanding of whether
>> Linux users can set a file's birth time. NFSD will now recognize a
>> time_create attribute but it ignores its value. It clears the
>> time_create bit in the returned attribute bitmask to indicate that
>> the value was not used.
>> 
>> Reported-by: Igor Mammedov <imammedo@xxxxxxxxxx>
>> Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> 
> Thanks for fixing it,
> tested 'touch', 'cp', 'tar' within CLI and copying file with Finder
> 
> Tested-by: Igor Mammedov <imammedo@xxxxxxxxxx>

Thanks!


> on tangent:
> when copying file from Mac (used 'cp') there is a delay ~4sec/file
> 'cp' does first triggers create then extra open and then setattr
> which returns
> SETATTR Status: NFS4ERR_DELAY
> after which the client stalls for a few seconds before repeating setattr.
> So question is what makes server unhappy to trigger this error
> and if it could be fixed on server side.
> 
> it seems to affect other methods of copying. So if one extracted
> an archive with multiple files or copied multiple files, that
> would be a pain.
> 
> With vers=3 copying is 'instant'
> with linux client and vers=4.0 copying is 'instant' as well but it
> doesn't use the same call sequence.
> 
> PS:
> it is not regression (I think slowness was there for a long time)

A network capture would help diagnose this further, but it
sounds like it's delegation-related.


>> ---
>> fs/nfsd/nfs4xdr.c | 9 +++++++++
>> fs/nfsd/nfsd.h | 3 ++-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index 61b2aae81abb..2acea7792bb2 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -470,6 +470,15 @@ nfsd4_decode_fattr4(struct nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
>> 			return nfserr_bad_xdr;
>> 		}
>> 	}
>> +	if (bmval[1] & FATTR4_WORD1_TIME_CREATE) {
>> +		struct timespec64 ts;
>> +
>> +		/* No Linux filesystem supports setting this attribute. */
>> +		bmval[1] &= ~FATTR4_WORD1_TIME_CREATE;
>> +		status = nfsd4_decode_nfstime4(argp, &ts);
>> +		if (status)
>> +			return status;
>> +	}
>> 	if (bmval[1] & FATTR4_WORD1_TIME_MODIFY_SET) {
>> 		u32 set_it;
>> 
>> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
>> index 847b482155ae..9a8b09afc173 100644
>> --- a/fs/nfsd/nfsd.h
>> +++ b/fs/nfsd/nfsd.h
>> @@ -465,7 +465,8 @@ static inline bool nfsd_attrs_supported(u32 minorversion, const u32 *bmval)
>> 	(FATTR4_WORD0_SIZE | FATTR4_WORD0_ACL)
>> #define NFSD_WRITEABLE_ATTRS_WORD1 \
>> 	(FATTR4_WORD1_MODE | FATTR4_WORD1_OWNER | FATTR4_WORD1_OWNER_GROUP \
>> -	| FATTR4_WORD1_TIME_ACCESS_SET | FATTR4_WORD1_TIME_MODIFY_SET)
>> +	| FATTR4_WORD1_TIME_ACCESS_SET | FATTR4_WORD1_TIME_CREATE \
>> +	| FATTR4_WORD1_TIME_MODIFY_SET)
>> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
>> #define MAYBE_FATTR4_WORD2_SECURITY_LABEL \
>> 	FATTR4_WORD2_SECURITY_LABEL

--
Chuck Lever