Re: [GIT PULL] nfsd changes for 5.18

[Bruce Fields]

On Mon, Jul 11, 2022 at 02:56:40PM -0400, Jeff Layton wrote:
> On Mon, 2022-07-11 at 14:36 -0400, Bruce Fields wrote:
> > On Mon, Jul 11, 2022 at 06:24:01PM +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Jul 11, 2022, at 2:19 PM, Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> > > > 
> > > > On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> > > > > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:
> > > > > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > > > > Starting with this patch client might think that server supports
> > > > > > > TIME_CREATE and start sending this attribute in its requests.
> > > > > > 
> > > > > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > > > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > > > > that decodes the birth time attribute.
> > > > > > 
> > > > > > I don't immediately see another storage protocol stack in our
> > > > > > kernel that supports a client setting the birth time, so NFSD
> > > > > > might have to ignore the client-provided value.
> > > > > > 
> > > > > 
> > > > > Cephfs allows this. My thinking at the time that I implemented it was
> > > > > that it should be settable for backup purposes, but this was possibly a
> > > > > mistake. On most filesystems, the btime seems to be equivalent to inode
> > > > > creation time and is read-only.
> > > > 
> > > > So supporting it as read-only seems reasonable.
> > > > 
> > > > Clearly, failing to decode the setattr attempt isn't the right way to do
> > > > that.  I'm not sure what exactly it should be doing--some kind of
> > > > permission error on any setattr containing TIME_CREATE?
> > > 
> > > I don't think that will work.
> > > 
> > > NFSD now asserts FATTR4_WORD1_TIME_CREATE when clients ask for
> > > the mask of attributes it supports. That means the server has
> > > to process GETATTR and SETATTR (and OPEN) operations that
> > > contain FATTR4_WORD1_TIME_CREATE as not an error.
> > 
> > Well, permissions or bad attribute values or other stuff may prevent
> > setting one of the attributes.
> > 
> > And setattr isn't guaranteed to be atomic, so I don't think you can
> > eliminate the possibility that part of it might succeed and part might
> > not.
> > 
> > But it might be more helpful to fail the whole thing up front if you
> > know part of it's going to fail?
> > 
> 
> RFC5661 says:
> 
>    On either success or failure of the operation, the server will return
>    the attrsset bitmask to represent what (if any) attributes were
>    successfully set.  The attrsset in the response is a subset of the
>    attrmask field of the obj_attributes field in the argument.
> 
> ...and then later:
> 
>    A mask of the attributes actually set is returned by SETATTR in all
>    cases.  That mask MUST NOT include attribute bits not requested to be
>    set by the client.  If the attribute masks in the request and reply
>    are equal, the status field in the reply MUST be NFS4_OK.

For some reason I thought the converse was true too (if the masks
differ, then the server should return an error).  But you're right, I
don't see that in the spec.

> So, I think just clearing the bit and returning NFS4_OK should be fine.

I suppose.

Nevertheless, the spec gives the option of returning both an error and a
bitmap, and to me it seems more helpful to take advantage of the
opportunity to tell the client both which attribute(s) failed and (to
the extent possible) why.  ??

> If the mask ends up being 0 after clearing the bit though, it might be
> reasonable to return something like NFS4ERR_ATTRNOTSUPP. That would be a
> bit weird though since we do support it for GETATTR, hence my suggestion
> for a NFS4ERR_ATTR_RO.

That might be useful.

--b.