Re: [PATCH 03/13] NFSv4.2: query the server for extended attribute support

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2020-03-13 at 15:19 +0100, Mkrtchyan, Tigran wrote:
> 
> ----- Original Message -----
> > From: "trondmy" <trondmy@hammerspace.com>
> > To: "Frank van der Linden" <fllinden@amazon.com>, "Tigran
> > Mkrtchyan" <tigran.mkrtchyan@desy.de>
> > Cc: "linux-nfs" <linux-nfs@vger.kernel.org>, "Anna Schumaker" <
> > anna.schumaker@netapp.com>
> > Sent: Friday, March 13, 2020 2:50:38 PM
> > Subject: Re: [PATCH 03/13] NFSv4.2: query the server for extended
> > attribute support
> > On Fri, 2020-03-13 at 12:11 +0100, Mkrtchyan, Tigran wrote:
> > > Hi Frank,
> > > 
> > > I think the way how you have implemented is almost correct. You
> > > query
> > > server for supported attributes. As result client will get all
> > > attributes
> > > supported bu the server and if FATTR4_XATTR_SUPPORT is returned,
> > > then
> > > client
> > > adds xattr capability. This the way how I read rfc8276. Do you
> > > have a
> > > different
> > > opinion?
> > > 
> > 
> > 'xattr_support' seems like a protocol hack to allow the client to
> > determine whether or not the xattr operations are supported.
> > 
> > The reason why it is a hack is that 'supported_attrs' is also a
> > per-
> > filesystem attribute, and there is no value in advertising
> > 'xattr_support' there unless your filesystem also supports xattrs.
> > 
> > IOW: the protocol forces you to do 2 round trips to the server in
> > order
> > to figure out something that really should be obvious with 1 round
> > trip.
> > 
> 
> So you say  that client have to query for xattr_support every time
> the
> fsid is changing?
> 

According to the spec it is a per-filesystem attribute, just like
supported_attrs...

Cheers
  Trond

> Tigran.
> 
> > > Regards,
> > >    Tigran.
> > > 
> > > ----- Original Message -----
> > > > From: "Frank van der Linden" <fllinden@amazon.com>
> > > > To: "Tigran Mkrtchyan" <tigran.mkrtchyan@desy.de>
> > > > Cc: "Trond Myklebust" <trond.myklebust@hammerspace.com>, "Anna
> > > > Schumaker" <anna.schumaker@netapp.com>, "linux-nfs"
> > > > <linux-nfs@vger.kernel.org>
> > > > Sent: Thursday, March 12, 2020 10:15:55 PM
> > > > Subject: Re: [PATCH 03/13] NFSv4.2: query the server for
> > > > extended
> > > > attribute support
> > > > On Thu, Mar 12, 2020 at 08:51:39PM +0000, Frank van der Linden
> > > > wrote:
> > > > > 1) The xattr_support attribute exists
> > > > > 2) The xattr support attribute exists *and* it's true for the
> > > > > root fh
> > > > > 
> > > > > Currently the code does 2) in one operation. That might not
> > > > > be
> > > > > 100%
> > > > > correct - the RFC does mention that (section 8.2):
> > > > > 
> > > > > "Before interrogating this attribute using GETATTR, a client
> > > > > should
> > > > >  determine whether it is a supported attribute by
> > > > > interrogating
> > > > > the
> > > > >  supported_attrs attribute."
> > > > > 
> > > > > That's a "should", not a "MUST", but it's still waving its
> > > > > finger
> > > > > at you not to do this.
> > > > > 
> > > > > Since 8.2.1 says:
> > > > > 
> > > > > "However, a client may reasonably assume that a server
> > > > >  (or file system) that does not support the xattr_support
> > > > > attribute
> > > > >  does not provide xattr support, and it acts on that basis."
> > > > > 
> > > > > ..I think you're right, and the code should just use the
> > > > > existence
> > > > > of the attribute as a signal that the server knows about
> > > > > xattrs -
> > > > > operations should still error out correctly if it doesn't.
> > > > > 
> > > > > I'll make that change, thanks.
> > > > 
> > > > ..or, alternatively, only query xattr_support in
> > > > nfs4_server_capabilities,
> > > > and then its actual value, if it exists, in nfs4_fs_info.
> > > > 
> > > > Any opinions on this?
> > > > 
> > > > - Frank
> > --
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@hammerspace.com
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com