Hi Frank- > On Oct 24, 2019, at 7:15 PM, Frank van der Linden <fllinden@xxxxxxxxxx> wrote: > > 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? Yes. utils/mount/nfs.man for the mount option. >> - 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. I have similar patches adding support for access to a couple of security xattrs. I initially wrapped the new code with CONFIG but after some discussion it was decided there was really no need to be so cautious. The user_xattr export option is a separate matter, but again, if we don't know of a use case for it, I would leave it out for the moment. >> - 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. After the patches are cleaner (checkpatch and squashing) I think you will get more direct review of the caching heuristics. I'll send some suggestions via private e-mail. >> - 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). That's exactly what's of concern. RDMA has similar logic as TCP here to allocate pages on demand for this case. The problem arises when the server needs to return a bigger reply than will fit in this buffer. Rare. >> 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. fstests would be the one. >> - 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. OK, if v4.2 is not supported in the test suite, then there is a pre-requisite discussion to be had. > 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)? For generic defines, include the same patches in both the client and server series. When git merges the two separate branches, it should recognize that the incoming files are identical and do nothing. -- Chuck Lever chucklever@xxxxxxxxx
