linux-nfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Frank van der Linden <fllinden@amazon.com>
To: Chuck Lever <chucklever@gmail.com>
Cc: Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>
Subject: Re: [RFC PATCH 00/35] user xattr support (RFC8276)
Date: Thu, 24 Oct 2019 23:15:48 +0000	[thread overview]
Message-ID: <20191024231547.GA16466@dev-dsk-fllinden-2c-c1893d73.us-west-2.amazon.com> (raw)
In-Reply-To: <9CAEB69A-A92C-47D8-9871-BA6EA83E1881@gmail.com>

Hi Chuck,

Thanks for your comments.

On Thu, Oct 24, 2019 at 04:16:33PM -0400, Chuck Lever wrote:
> - IMO you can post future updates just to linux-nfs. Note that the
> kernel NFS client and server are maintained separately, so when it
> comes time to submit final patches, you will send the client work
> to Trond and Anna, and the server work to Bruce (and maybe me).

Sure, I'll do that.

> 
> - We like patches that are as small as possible but no smaller.
> Some of these might be too small. For example, you don't need to add
> the XDR encoders, decoders, and reply size macros in separate patches.

True, I might have gone overboard there :-) If you can send further
suggestions offline, that'd be great!

> - Please run scripts/checkpatch.pl on each patch before you post
> again. This will help identify coding convention issues that should
> be addressed before merge. sparse is also a good idea too.
> clang-format is also nice but is entirely optional.

No problem. I think there shouldn't be many issues, but I'm sure
I mixed up some of the coding styles I've had to adhere to over
the decades..

> 
> - I was not able to get 34/35 to apply. The series might be missing
> a patch that adds nfsd_getxattr and friends.

Hm, odd. I'll check on that - I might have messed up there.

> 
> - Do you have man page updates for the new mount and export options?

I don't, but I can easily write them. They go in nfs-utils, right?

> - I'm not clear why new CONFIG options are necessary. These days we
> try to avoid adding new CONFIG options if possible. I can't think of
> a reason someone would need to compile user xattr support out if
> NFSv4.2 is enabled.
> 
> - Can you explain why an NFS server administrator might want to
> disable user xattr support on a share?

I think both of these are cases of being careful. E.g. don't enable
something by default and allow it to be disabled at runtime in
case something goes terribly wrong.

I didn't have any other reasons, really. I'm happy do to away with
the CONFIG options if that's the consensus, as well as the
nouser_xattr export option.

> - Probably you are correct that the design choices you made regarding
> multi-message LISTXATTR are the best that can be done. Hopefully that
> is not a frequent operation, but for something like "tar" it might be.
> Do you have any feeling for how to assess performance?

So far, my performance testing has been based on synthetic workloads,
which I'm also using to test some boundary conditions. E.g. create
as many xattrs as the Linux limit allows, list them all, now do
this for many files, etc. I'll definitely add testing with code
that uses xattrs. tar is on the list, but I'm happy to test anything
that exercises the code.

I don't think a multi-message LISTXATTR will happen a lot in practice,
if at all.

> 
> - Regarding client caching... the RFC is notably vague about what
> is needed there. You might be able to get away with no caching, just
> as a start. Do you (and others) think that this series would be
> acceptable and mergeable without any client caching support?

The performance is, obviously, not great without client side caching.
But then again, that's on my synthetic workloads. In cases like GNU
tar, it won't matter a whole lot because of the way that code is
structured.

I would prefer to have client side caching enabled from the start.
I have an implementation that works, but, like I mentioned, I
have some misgivings about it. But I should just include it when
I re-post - I might simply be worrying too much.

> 
> - Do you have access to an RDMA-capable platform? RPC/RDMA needs to
> be able to predict how large each reply will be, in order to reserve
> appropriate memory resources to land the whole RPC reply on the client.
> I'm wondering if you've found any particular areas where that might be
> a challenge.

Hm. I might be able to set something up. If not, I'd be relying
on someone you might know to test it for me :-)

I am not too familiar with the RDMA RPC code. From what I posted, is 
there any specific part of how the RPC layer is used that would
be of concern with RDMA?

I don't do anything other parts of the code don't do. The only special
case is using on-demand page allocation on receive, which the ACL code
also does (XDRBUF_SPARSE_PAGES - used for LISTXATTR and GETXATTR).

> 
> 
> Testing:
> 
> - Does fstests already have user xattr functional tests? If not, how
> do you envision testing this new code?

https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/ has some xattr
tests. I've, so far, been using my own set of tests that I'm happy
to contribute to any testsuite.

> 
> - How should we test the logic that deals with delegation recall?

I believe pyNFS has some logic do test this. What I have been doing
is manual testing, either using 2 clients, or, simpler, setting
xattrs on a file on the server itself, and verifying that client
delegations were recalled.

> 
> - Do you have plans to submit patches to pyNFS?

It wasn't in my plans, but I certainly could. One issue I've noticed,
with pyNFS and some other tests, is that they go no further than 4.1.
They'll need some more work to do 4.2 - although that shouldn't be
a lot of work, as most (or was it all?) features in 4.2 are optional.

I've not had much time to work on this in the past few weeks, but
next week is looking much better. Here's my plan:

* address any issues flagged by checkpatch
* merge some patches, with your input
* clean up my nfs-ganesha patches and test some more against that
* test against Rick's FreeBSD prototype
* repost the series, split in to client and server

In general, what do people do with code changes that affect both
client and server (e.g. generic defines)?

Thanks,

- Frank

  reply	other threads:[~2019-10-24 23:15 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12 17:25 [RFC PATCH 00/35] user xattr support (RFC8276) Frank van der Linden
2019-07-01 17:56 ` [RFC PATCH 02/35] nfs/nfsd: basic NFS4 extended attribute definitions Frank van der Linden
2019-08-26 21:38 ` [RFC PATCH 03/35] NFSv4.2: query the server for extended attribute support Frank van der Linden
2019-08-26 21:53 ` [RFC PATCH 04/35] nfs: parse the {no}user_xattr option Frank van der Linden
2019-08-26 22:06 ` [RFC PATCH 05/35] NFSv4.2: define a function to compute the maximum XDR size for listxattr Frank van der Linden
2019-08-26 22:23 ` [RFC PATCH 06/35] NFSv4.2: define and set initial limits for extended attributes Frank van der Linden
2019-08-26 22:32 ` [RFC PATCH 07/35] NFSv4.2: define argument and response structures for xattr operations Frank van der Linden
2019-08-26 22:44 ` [RFC PATCH 08/35] NFSv4.2: define the encode/decode sizes for the XATTR operations Frank van der Linden
2019-08-26 23:09 ` [RFC PATCH 09/35] NFSv4.2: define and use extended attribute overhead sizes Frank van der Linden
2019-08-27 15:34 ` [RFC PATCH 10/35] NFSv4.2: add client side XDR handling for extended attributes Frank van der Linden
2019-08-27 15:46 ` [RFC PATCH 11/35] nfs: define nfs_access_get_cached function Frank van der Linden
2019-08-27 16:01 ` [RFC PATCH 12/35] NFSv4.2: query the extended attribute access bits Frank van der Linden
2019-08-27 22:51 ` [RFC PATCH 13/35] nfs: modify update_changeattr to deal with regular files Frank van der Linden
2019-08-30 22:48 ` [RFC PATCH 14/35] nfs: define and use the NFS_INO_INVALID_XATTR flag Frank van der Linden
2019-08-30 22:56 ` [RFC PATCH 15/35] nfs: make the buf_to_pages_noslab function available to the nfs code Frank van der Linden
2019-08-30 23:15 ` [RFC PATCH 16/35] NFSv4.2: add the extended attribute proc functions Frank van der Linden
2019-08-30 23:31 ` [RFC PATCH 17/35] NFSv4.2: hook in the user extended attribute handlers Frank van der Linden
2019-08-30 23:38 ` [RFC PATCH 18/35] NFSv4.2: add client side xattr caching functions Frank van der Linden
2019-08-30 23:45 ` [RFC PATCH 19/35] NFSv4.2: call the xattr cache functions Frank van der Linden
2019-08-30 23:59 ` [RFC PATCH 20/35] nfs: add the NFS_V4_XATTR config option Frank van der Linden
2019-08-31  2:12 ` [RFC PATCH 21/35] xattr: modify vfs_{set,remove}xattr for NFS server use Frank van der Linden
2019-08-31 19:00 ` [RFC PATCH 22/35] nfsd: split off the write decode code in to a seperate function Frank van der Linden
2019-08-31 19:19 ` [RFC PATCH 23/35] nfsd: add defines for NFSv4.2 extended attribute support Frank van der Linden
2019-08-31 21:35 ` [RFC PATCH 26/35] nfsd: add structure definitions for xattr requests / responses Frank van der Linden
2019-08-31 23:53 ` [RFC PATCH 24/35] nfsd: define xattr functions to call in to their vfs counterparts Frank van der Linden
2019-09-01  0:13 ` [RFC PATCH 25/35] nfsd: take xattr access bits in to account when checking Frank van der Linden
2019-09-01  1:19 ` [RFC PATCH 27/35] nfsd: implement the xattr procedure functions Frank van der Linden
2019-09-01  2:46 ` [RFC PATCH 01/35] nfsd: make sure the nfsd4_ops array has the right size Frank van der Linden
2019-09-02 19:40 ` [RFC PATCH 28/35] nfsd: define xattr reply size functions Frank van der Linden
2019-09-02 19:58 ` [RFC PATCH 29/35] nfsd: add xattr XDR decode functions Frank van der Linden
2019-09-02 20:09 ` [RFC PATCH 30/35] nfsd: add xattr XDR encode functions Frank van der Linden
2019-09-02 20:19 ` [RFC PATCH 31/35] nfsd: add xattr operations to ops array Frank van der Linden
2019-09-02 20:34 ` [RFC PATCH 32/35] xattr: add a function to check if a namespace is supported Frank van der Linden
2019-09-02 21:30 ` [RFC PATCH 33/35] nfsd: add fattr support for user extended attributes Frank van der Linden
2019-09-02 23:06 ` [RFC PATCH 34/35] nfsd: add export flag to disable " Frank van der Linden
2019-09-02 23:17 ` [RFC PATCH 35/35] nfsd: add NFSD_V4_XATTR config option Frank van der Linden
2019-10-24 20:16 ` [RFC PATCH 00/35] user xattr support (RFC8276) Chuck Lever
2019-10-24 23:15   ` Frank van der Linden [this message]
2019-10-25 19:55     ` Chuck Lever
2019-11-04  3:01       ` J. Bruce Fields
2019-11-04 15:36         ` Chuck Lever
2019-11-04 16:21           ` Frank van der Linden
2019-11-04 22:58             ` Bruce Fields
2019-11-05  0:06               ` Frank van der Linden
2019-11-05 15:44               ` Chuck Lever

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191024231547.GA16466@dev-dsk-fllinden-2c-c1893d73.us-west-2.amazon.com \
    --to=fllinden@amazon.com \
    --cc=chucklever@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).