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

[Igor Mammedov]

On Mon, 11 Jul 2022 17:18:38 +0000
Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:

> > 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.
yep, there was delegation request/response right after SETATTR failure
possibly prompted by NFS4ERR_DELAY

shall I provide a network capture (I guess pcap file) from test env
I have?

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