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.
So, I think just clearing the bit and returning NFS4_OK should be fine.
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.
> > The protocol
> > allows the server to indicate it ignored the time_create value
> > by clearing the FATTR4_WORD1_TIME_CREATE bit in the attribute
> > bitmask it returns in the reply.
>
> Yes, I think you also return an error in that case, though.
>
> --b.
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
