Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: vWJBAWirJ4KF7BjXQAftYG0Hn8i6kWFPUj3NejJiQT+YdHlgTloX3IfvDNfeDWuliQDFrME0Y5
 qJCl/wouB/BQ==
Date:   Thu, 12 Mar 2020 20:51:39 +0000
From:   Frank van der Linden <fllinden@amazon.com>
To:     "Mkrtchyan, Tigran" <tigran.mkrtchyan@desy.de>
CC:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 03/13] NFSv4.2: query the server for extended attribute
 support
Message-ID: <20200312205139.GA32293@dev-dsk-fllinden-2c-c1893d73.us-west-2.amazon.com>
References: <20200311195613.26108-1-fllinden@amazon.com>
 <20200311195613.26108-4-fllinden@amazon.com>
 <530167624.4533477.1584029710746.JavaMail.zimbra@desy.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <530167624.4533477.1584029710746.JavaMail.zimbra@desy.de>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Thu, Mar 12, 2020 at 05:15:10PM +0100, Mkrtchyan, Tigran wrote:
> > +     fattr4_word2_nfs42_mask = FATTR4_WORD2_NFS42_MASK;
> > +
> > +     if (minorversion >= 2) {
> 
> I am not sure you need this extra check as by querying for  FATTR4_WORD0_SUPPORTED_ATTRS
> server already will return FATTR4_WORD2_XATTR_SUPPORT if supported.

There used to be a mount option check here, which I later removed. That
means that fattr4_word2_nfs42_mask is no longer needed, so I'll remove
that.

As for the attribute itself: I suppose the question here is what the client
should use to assume the server has RFC 8276 support:

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.

Frank