[03/13] NFSv4.2: query the server for extended attribute support
diff mbox series

Message ID 20200311195613.26108-4-fllinden@amazon.com
State New
Headers show
Series
  • client side user xattr (RFC8276) support
Related show

Commit Message

Frank van der Linden March 11, 2020, 7:56 p.m. UTC
Query the server for extended attribute support, and record it
as the NFS_CAP_XATTR flag in the server capabilities.

Signed-off-by: Frank van der Linden <fllinden@amazon.com>
---
 fs/nfs/nfs4proc.c         | 14 ++++++++++++--
 fs/nfs/nfs4xdr.c          | 23 +++++++++++++++++++++++
 include/linux/nfs_fs_sb.h |  1 +
 include/linux/nfs_xdr.h   |  1 +
 4 files changed, 37 insertions(+), 2 deletions(-)

Comments

Mkrtchyan, Tigran March 12, 2020, 4:15 p.m. UTC | #1
Hi Frank,

----- Original Message -----
> From: "Frank van der Linden" <fllinden@amazon.com>
> To: "Trond Myklebust" <trond.myklebust@hammerspace.com>, "Anna Schumaker" <anna.schumaker@netapp.com>, "linux-nfs"
> <linux-nfs@vger.kernel.org>
> Cc: "Frank van der Linden" <fllinden@amazon.com>
> Sent: Wednesday, March 11, 2020 8:56:03 PM
> Subject: [PATCH 03/13] NFSv4.2: query the server for extended attribute support

> Query the server for extended attribute support, and record it
> as the NFS_CAP_XATTR flag in the server capabilities.
> 
> Signed-off-by: Frank van der Linden <fllinden@amazon.com>
> ---
> fs/nfs/nfs4proc.c         | 14 ++++++++++++--
> fs/nfs/nfs4xdr.c          | 23 +++++++++++++++++++++++
> include/linux/nfs_fs_sb.h |  1 +
> include/linux/nfs_xdr.h   |  1 +
> 4 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 69b7ab7a5815..47bbd7db9d18 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3742,6 +3742,7 @@ static void nfs4_close_context(struct nfs_open_context
> *ctx, int is_sync)
> static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh
> *fhandle)
> {
> 	u32 bitmask[3] = {}, minorversion = server->nfs_client->cl_minorversion;
> +	u32 fattr4_word2_nfs42_mask;
> 	struct nfs4_server_caps_arg args = {
> 		.fhandle = fhandle,
> 		.bitmask = bitmask,
> @@ -3763,6 +3764,13 @@ static int _nfs4_server_capabilities(struct nfs_server
> *server, struct nfs_fh *f
> 	if (minorversion)
> 		bitmask[2] = FATTR4_WORD2_SUPPATTR_EXCLCREAT;
> 
> +	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.

Tigran.

> +		bitmask[2] |= FATTR4_WORD2_XATTR_SUPPORT;
> +		fattr4_word2_nfs42_mask |= FATTR4_WORD2_XATTR_SUPPORT;
> +	}
> +
> 	status = nfs4_call_sync(server->client, server, &msg, &args.seq_args,
> 	&res.seq_res, 0);
> 	if (status == 0) {
> 		/* Sanity check the server answers */
> @@ -3775,7 +3783,7 @@ static int _nfs4_server_capabilities(struct nfs_server
> *server, struct nfs_fh *f
> 			res.attr_bitmask[2] &= FATTR4_WORD2_NFS41_MASK;
> 			break;
> 		case 2:
> -			res.attr_bitmask[2] &= FATTR4_WORD2_NFS42_MASK;
> +			res.attr_bitmask[2] &= fattr4_word2_nfs42_mask;
> 		}
> 		memcpy(server->attr_bitmask, res.attr_bitmask, sizeof(server->attr_bitmask));
> 		server->caps &= ~(NFS_CAP_ACLS|NFS_CAP_HARDLINKS|
> @@ -3783,7 +3791,7 @@ static int _nfs4_server_capabilities(struct nfs_server
> *server, struct nfs_fh *f
> 				NFS_CAP_MODE|NFS_CAP_NLINK|NFS_CAP_OWNER|
> 				NFS_CAP_OWNER_GROUP|NFS_CAP_ATIME|
> 				NFS_CAP_CTIME|NFS_CAP_MTIME|
> -				NFS_CAP_SECURITY_LABEL);
> +				NFS_CAP_SECURITY_LABEL|NFS_CAP_XATTR);
> 		if (res.attr_bitmask[0] & FATTR4_WORD0_ACL &&
> 				res.acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
> 			server->caps |= NFS_CAP_ACLS;
> @@ -3811,6 +3819,8 @@ static int _nfs4_server_capabilities(struct nfs_server
> *server, struct nfs_fh *f
> 		if (res.attr_bitmask[2] & FATTR4_WORD2_SECURITY_LABEL)
> 			server->caps |= NFS_CAP_SECURITY_LABEL;
> #endif
> +		if (res.has_xattr)
> +			server->caps |= NFS_CAP_XATTR;
> 		memcpy(server->attr_bitmask_nl, res.attr_bitmask,
> 				sizeof(server->attr_bitmask));
> 		server->attr_bitmask_nl[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 47817ef0aadb..bebc087a1433 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -4201,6 +4201,26 @@ static int decode_attr_time_modify(struct xdr_stream
> *xdr, uint32_t *bitmap, str
> 	return status;
> }
> 
> +static int decode_attr_xattrsupport(struct xdr_stream *xdr, uint32_t *bitmap,
> +				    uint32_t *res)
> +{
> +	__be32 *p;
> +
> +	*res = 0;
> +	if (unlikely(bitmap[2] & (FATTR4_WORD2_XATTR_SUPPORT - 1U)))
> +		return -EIO;
> +	if (likely(bitmap[2] & FATTR4_WORD2_XATTR_SUPPORT)) {
> +		p = xdr_inline_decode(xdr, 4);
> +		if (unlikely(!p))
> +			return -EIO;
> +		*res = be32_to_cpup(p);
> +		bitmap[2] &= ~FATTR4_WORD2_XATTR_SUPPORT;
> +	}
> +	dprintk("%s: XATTR support=%s\n", __func__,
> +		*res == 0 ? "false" : "true");
> +	return 0;
> +}
> +
> static int verify_attr_len(struct xdr_stream *xdr, unsigned int savep, uint32_t
> attrlen)
> {
> 	unsigned int attrwords = XDR_QUADLEN(attrlen);
> @@ -4371,6 +4391,9 @@ static int decode_server_caps(struct xdr_stream *xdr,
> struct nfs4_server_caps_re
> 	if ((status = decode_attr_exclcreat_supported(xdr, bitmap,
> 				res->exclcreat_bitmask)) != 0)
> 		goto xdr_error;
> +	status = decode_attr_xattrsupport(xdr, bitmap, &res->has_xattr);
> +	if (status != 0)
> +		goto xdr_error;
> 	status = verify_attr_len(xdr, savep, attrlen);
> xdr_error:
> 	dprintk("%s: xdr returned %d!\n", __func__, -status);
> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> index 465fa98258a3..d881f7a38bc9 100644
> --- a/include/linux/nfs_fs_sb.h
> +++ b/include/linux/nfs_fs_sb.h
> @@ -281,5 +281,6 @@ struct nfs_server {
> #define NFS_CAP_OFFLOAD_CANCEL	(1U << 25)
> #define NFS_CAP_LAYOUTERROR	(1U << 26)
> #define NFS_CAP_COPY_NOTIFY	(1U << 27)
> +#define NFS_CAP_XATTR		(1U << 28)
> 
> #endif
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 94c77ed55ce1..5076fe42c693 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1178,6 +1178,7 @@ struct nfs4_server_caps_res {
> 	u32				has_links;
> 	u32				has_symlinks;
> 	u32				fh_expire_type;
> +	u32				has_xattr;
> };
> 
> #define NFS4_PATHNAME_MAXCOMPONENTS 512
> --
> 2.16.6
Frank van der Linden March 12, 2020, 8:51 p.m. UTC | #2
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
Frank van der Linden March 12, 2020, 9:15 p.m. UTC | #3
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
Mkrtchyan, Tigran March 13, 2020, 11:11 a.m. UTC | #4
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?

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 March 13, 2020, 1:50 p.m. UTC | #5
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.

> 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
Mkrtchyan, Tigran March 13, 2020, 2:19 p.m. UTC | #6
----- 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?

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 March 13, 2020, 5:10 p.m. UTC | #7
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
Frank van der Linden March 13, 2020, 5:55 p.m. UTC | #8
On Fri, Mar 13, 2020 at 01:50:38PM +0000, Trond Myklebust wrote:
> '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.

Right, that's the annoying part. I mean, theoretically, a server can
legally reject a GETATTR because you're asking for an unknown
attribute (e.g. xattr_support in this case). So then what I have
in my current patch (asking for both the supported_attrs and
xattr_support at once) might fail, and the mount would fail.

Which means I should split it up and have nfs4_do_fsinfo do
the 2nd part, just in case.

- Frank

Patch
diff mbox series

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 69b7ab7a5815..47bbd7db9d18 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3742,6 +3742,7 @@  static void nfs4_close_context(struct nfs_open_context *ctx, int is_sync)
 static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *fhandle)
 {
 	u32 bitmask[3] = {}, minorversion = server->nfs_client->cl_minorversion;
+	u32 fattr4_word2_nfs42_mask;
 	struct nfs4_server_caps_arg args = {
 		.fhandle = fhandle,
 		.bitmask = bitmask,
@@ -3763,6 +3764,13 @@  static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 	if (minorversion)
 		bitmask[2] = FATTR4_WORD2_SUPPATTR_EXCLCREAT;
 
+	fattr4_word2_nfs42_mask = FATTR4_WORD2_NFS42_MASK;
+
+	if (minorversion >= 2) {
+		bitmask[2] |= FATTR4_WORD2_XATTR_SUPPORT;
+		fattr4_word2_nfs42_mask |= FATTR4_WORD2_XATTR_SUPPORT;
+	}
+
 	status = nfs4_call_sync(server->client, server, &msg, &args.seq_args, &res.seq_res, 0);
 	if (status == 0) {
 		/* Sanity check the server answers */
@@ -3775,7 +3783,7 @@  static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 			res.attr_bitmask[2] &= FATTR4_WORD2_NFS41_MASK;
 			break;
 		case 2:
-			res.attr_bitmask[2] &= FATTR4_WORD2_NFS42_MASK;
+			res.attr_bitmask[2] &= fattr4_word2_nfs42_mask;
 		}
 		memcpy(server->attr_bitmask, res.attr_bitmask, sizeof(server->attr_bitmask));
 		server->caps &= ~(NFS_CAP_ACLS|NFS_CAP_HARDLINKS|
@@ -3783,7 +3791,7 @@  static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 				NFS_CAP_MODE|NFS_CAP_NLINK|NFS_CAP_OWNER|
 				NFS_CAP_OWNER_GROUP|NFS_CAP_ATIME|
 				NFS_CAP_CTIME|NFS_CAP_MTIME|
-				NFS_CAP_SECURITY_LABEL);
+				NFS_CAP_SECURITY_LABEL|NFS_CAP_XATTR);
 		if (res.attr_bitmask[0] & FATTR4_WORD0_ACL &&
 				res.acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
 			server->caps |= NFS_CAP_ACLS;
@@ -3811,6 +3819,8 @@  static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 		if (res.attr_bitmask[2] & FATTR4_WORD2_SECURITY_LABEL)
 			server->caps |= NFS_CAP_SECURITY_LABEL;
 #endif
+		if (res.has_xattr)
+			server->caps |= NFS_CAP_XATTR;
 		memcpy(server->attr_bitmask_nl, res.attr_bitmask,
 				sizeof(server->attr_bitmask));
 		server->attr_bitmask_nl[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 47817ef0aadb..bebc087a1433 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -4201,6 +4201,26 @@  static int decode_attr_time_modify(struct xdr_stream *xdr, uint32_t *bitmap, str
 	return status;
 }
 
+static int decode_attr_xattrsupport(struct xdr_stream *xdr, uint32_t *bitmap,
+				    uint32_t *res)
+{
+	__be32 *p;
+
+	*res = 0;
+	if (unlikely(bitmap[2] & (FATTR4_WORD2_XATTR_SUPPORT - 1U)))
+		return -EIO;
+	if (likely(bitmap[2] & FATTR4_WORD2_XATTR_SUPPORT)) {
+		p = xdr_inline_decode(xdr, 4);
+		if (unlikely(!p))
+			return -EIO;
+		*res = be32_to_cpup(p);
+		bitmap[2] &= ~FATTR4_WORD2_XATTR_SUPPORT;
+	}
+	dprintk("%s: XATTR support=%s\n", __func__,
+		*res == 0 ? "false" : "true");
+	return 0;
+}
+
 static int verify_attr_len(struct xdr_stream *xdr, unsigned int savep, uint32_t attrlen)
 {
 	unsigned int attrwords = XDR_QUADLEN(attrlen);
@@ -4371,6 +4391,9 @@  static int decode_server_caps(struct xdr_stream *xdr, struct nfs4_server_caps_re
 	if ((status = decode_attr_exclcreat_supported(xdr, bitmap,
 				res->exclcreat_bitmask)) != 0)
 		goto xdr_error;
+	status = decode_attr_xattrsupport(xdr, bitmap, &res->has_xattr);
+	if (status != 0)
+		goto xdr_error;
 	status = verify_attr_len(xdr, savep, attrlen);
 xdr_error:
 	dprintk("%s: xdr returned %d!\n", __func__, -status);
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index 465fa98258a3..d881f7a38bc9 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -281,5 +281,6 @@  struct nfs_server {
 #define NFS_CAP_OFFLOAD_CANCEL	(1U << 25)
 #define NFS_CAP_LAYOUTERROR	(1U << 26)
 #define NFS_CAP_COPY_NOTIFY	(1U << 27)
+#define NFS_CAP_XATTR		(1U << 28)
 
 #endif
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 94c77ed55ce1..5076fe42c693 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1178,6 +1178,7 @@  struct nfs4_server_caps_res {
 	u32				has_links;
 	u32				has_symlinks;
 	u32				fh_expire_type;
+	u32				has_xattr;
 };
 
 #define NFS4_PATHNAME_MAXCOMPONENTS 512