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 <[email protected]>
Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
Signed-off-by: Chuck Lever <[email protected]>
---
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
On Sun, 2022-07-10 at 14:46 -0400, Chuck Lever 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 <[email protected]>
> Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
> Signed-off-by: Chuck Lever <[email protected]>
> ---
> 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
>
>
RFC5661 lists time_create as being writeable, so silently ignoring it
seems wrong. It seems like we ought to have nfsd attempt to set the
btime and then just return an error if it doesn't work...but, I don't
see a mechanism in the kernel for setting it. ATTR_BTIME doesn't exist,
for instance.
Still, since we can't set it, returning an error there seems more
correct. NFS4ERR_INVAL is probably the wrong one -- maybe
NFS4ERR_NOTSUPP ? It's a bit weird since we do support querying it, but
not setting it. Maybe we need to propose a new NFS4ERR_ATTR_RO ?
--
Jeff Layton <[email protected]>
> On Jul 11, 2022, at 7:36 AM, Jeff Layton <[email protected]> wrote:
>
> On Sun, 2022-07-10 at 14:46 -0400, Chuck Lever 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 <[email protected]>
>> Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>> 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
>>
>>
>
> RFC5661 lists time_create as being writeable, so silently ignoring it
> seems wrong.
Open for debate. The protocol does allow a SETATTR. But the
specification doesn't have much else to say about the semantics
of time_create; contrast that with mtime or ctime.
> It seems like we ought to have nfsd attempt to set the
> btime and then just return an error if it doesn't work...
The usual way the NFSv4 protocol handles this is that the
attribute's bit in the returned bitmask is cleared, so that
the rest of the COMPOUND is able to succeed. There's no
NFS4ERR status code in this case.
> but, I don't
> see a mechanism in the kernel for setting it. ATTR_BTIME doesn't exist,
> for instance.
That's what I observed: there doesn't seem to be a mechanism in
Linux for setting it. Perhaps I should have copied fsdevel.
> Still, since we can't set it, returning an error there seems more
> correct. NFS4ERR_INVAL is probably the wrong one -- maybe
> NFS4ERR_NOTSUPP ? It's a bit weird since we do support querying it, but
> not setting it. Maybe we need to propose a new NFS4ERR_ATTR_RO ?
As I said above, the protocol's way of dealing with it is to
clear the attribute's bit in the returned attribute bitmask.
"You asked me to set this attribute, but I didn't". Clients,
IMO, will be more prepared to deal with that than having
all of their OPENs fail with NFS4ERR_NOTSUPP.
IMO explicitly setting a file's birth time doesn't seem quite
kosher, and it's not a POSIX attribute anyway, so we don't
have a standard to cleave to here (at least one that I'm aware
of). I'm fine with the patch as it stands, but I'm open to
hear more opinions about this.
--
Chuck Lever
On Mon, 2022-07-11 at 14:29 +0000, Chuck Lever III wrote:
>
> > On Jul 11, 2022, at 7:36 AM, Jeff Layton <[email protected]> wrote:
> >
> > On Sun, 2022-07-10 at 14:46 -0400, Chuck Lever 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 <[email protected]>
> > > Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
> > > Signed-off-by: Chuck Lever <[email protected]>
> > > ---
> > > 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
> > >
> > >
> >
> > RFC5661 lists time_create as being writeable, so silently ignoring it
> > seems wrong.
>
> Open for debate. The protocol does allow a SETATTR. But the
> specification doesn't have much else to say about the semantics
> of time_create; contrast that with mtime or ctime.
>
>
> > It seems like we ought to have nfsd attempt to set the
> > btime and then just return an error if it doesn't work...
>
> The usual way the NFSv4 protocol handles this is that the
> attribute's bit in the returned bitmask is cleared, so that
> the rest of the COMPOUND is able to succeed. There's no
> NFS4ERR status code in this case.
>
>
> > but, I don't
> > see a mechanism in the kernel for setting it. ATTR_BTIME doesn't exist,
> > for instance.
>
> That's what I observed: there doesn't seem to be a mechanism in
> Linux for setting it. Perhaps I should have copied fsdevel.
>
>
> > Still, since we can't set it, returning an error there seems more
> > correct. NFS4ERR_INVAL is probably the wrong one -- maybe
> > NFS4ERR_NOTSUPP ? It's a bit weird since we do support querying it, but
> > not setting it. Maybe we need to propose a new NFS4ERR_ATTR_RO ?
>
> As I said above, the protocol's way of dealing with it is to
> clear the attribute's bit in the returned attribute bitmask.
> "You asked me to set this attribute, but I didn't". Clients,
> IMO, will be more prepared to deal with that than having
> all of their OPENs fail with NFS4ERR_NOTSUPP.
>
> IMO explicitly setting a file's birth time doesn't seem quite
> kosher, and it's not a POSIX attribute anyway, so we don't
> have a standard to cleave to here (at least one that I'm aware
> of). I'm fine with the patch as it stands, but I'm open to
> hear more opinions about this.
>
>
Ok, now that I looked over the SETATTR part of the spec, I agree. Just
clearing the bit in the "attrsset" mask should do the right thing, and
that's probably better than returning an error.
Reviewed-by: Jeff Layton <[email protected]>
> On Jul 11, 2022, at 10:46 AM, Jeff Layton <[email protected]> wrote:
>
> On Mon, 2022-07-11 at 14:29 +0000, Chuck Lever III wrote:
>>
>>> On Jul 11, 2022, at 7:36 AM, Jeff Layton <[email protected]> wrote:
>>>
>>> On Sun, 2022-07-10 at 14:46 -0400, Chuck Lever 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 <[email protected]>
>>>> Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
>>>> Signed-off-by: Chuck Lever <[email protected]>
>>>> ---
>>>> 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
>>>>
>>>>
>>>
>>> RFC5661 lists time_create as being writeable, so silently ignoring it
>>> seems wrong.
>>
>> Open for debate. The protocol does allow a SETATTR. But the
>> specification doesn't have much else to say about the semantics
>> of time_create; contrast that with mtime or ctime.
>>
>>
>>> It seems like we ought to have nfsd attempt to set the
>>> btime and then just return an error if it doesn't work...
>>
>> The usual way the NFSv4 protocol handles this is that the
>> attribute's bit in the returned bitmask is cleared, so that
>> the rest of the COMPOUND is able to succeed. There's no
>> NFS4ERR status code in this case.
>>
>>
>>> but, I don't
>>> see a mechanism in the kernel for setting it. ATTR_BTIME doesn't exist,
>>> for instance.
>>
>> That's what I observed: there doesn't seem to be a mechanism in
>> Linux for setting it. Perhaps I should have copied fsdevel.
>>
>>
>>> Still, since we can't set it, returning an error there seems more
>>> correct. NFS4ERR_INVAL is probably the wrong one -- maybe
>>> NFS4ERR_NOTSUPP ? It's a bit weird since we do support querying it, but
>>> not setting it. Maybe we need to propose a new NFS4ERR_ATTR_RO ?
>>
>> As I said above, the protocol's way of dealing with it is to
>> clear the attribute's bit in the returned attribute bitmask.
>> "You asked me to set this attribute, but I didn't". Clients,
>> IMO, will be more prepared to deal with that than having
>> all of their OPENs fail with NFS4ERR_NOTSUPP.
>>
>> IMO explicitly setting a file's birth time doesn't seem quite
>> kosher, and it's not a POSIX attribute anyway, so we don't
>> have a standard to cleave to here (at least one that I'm aware
>> of). I'm fine with the patch as it stands, but I'm open to
>> hear more opinions about this.
>>
>>
>
> Ok, now that I looked over the SETATTR part of the spec, I agree. Just
> clearing the bit in the "attrsset" mask should do the right thing, and
> that's probably better than returning an error.
> Reviewed-by: Jeff Layton <[email protected]>
Thanks for your review!
Just because I'm not 100% certain of this choice, I will hang
onto it for a few more days. Also, Igor, please feel free to try
this out and reply with Tested-by. I do have Monterey here to
do some simple testing, but the more the merrier.
--
Chuck Lever
On Sun, 10 Jul 2022 14:46:04 -0400
Chuck Lever <[email protected]> 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 <[email protected]>
> Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
> Signed-off-by: Chuck Lever <[email protected]>
Thanks for fixing it,
tested 'touch', 'cp', 'tar' within CLI and copying file with Finder
Tested-by: Igor Mammedov <[email protected]>
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)
> ---
> 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
>
>
> On Jul 11, 2022, at 1:14 PM, Igor Mammedov <[email protected]> wrote:
>
> On Sun, 10 Jul 2022 14:46:04 -0400
> Chuck Lever <[email protected]> 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 <[email protected]>
>> Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
>> Signed-off-by: Chuck Lever <[email protected]>
>
> Thanks for fixing it,
> tested 'touch', 'cp', 'tar' within CLI and copying file with Finder
>
> Tested-by: Igor Mammedov <[email protected]>
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
On Mon, 11 Jul 2022 17:18:38 +0000
Chuck Lever III <[email protected]> wrote:
> > On Jul 11, 2022, at 1:14 PM, Igor Mammedov <[email protected]> wrote:
> >
> > On Sun, 10 Jul 2022 14:46:04 -0400
> > Chuck Lever <[email protected]> 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 <[email protected]>
> >> Fixes: e377a3e698fb ("nfsd: Add support for the birth time attribute")
> >> Signed-off-by: Chuck Lever <[email protected]>
> >
> > Thanks for fixing it,
> > tested 'touch', 'cp', 'tar' within CLI and copying file with Finder
> >
> > Tested-by: Igor Mammedov <[email protected]>
>
> 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
>
>
>
> On Jul 12, 2022, at 4:09 AM, Igor Mammedov <[email protected]> wrote:
>
> On Mon, 11 Jul 2022 17:18:38 +0000
> Chuck Lever III <[email protected]> wrote:
>
>>> On Jul 11, 2022, at 1:14 PM, Igor Mammedov <[email protected]> wrote:
>>>
>>> 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?
Yes, you can send it directly to me.
--
Chuck Lever