Re: [PATCH 0/8] NFSD: clean up locking.

[Chuck Lever III <chuck.lever@oracle.com>]

> On Jul 14, 2022, at 10:36 PM, NeilBrown <neilb@suse.de> wrote:
> 
> On Thu, 14 Jul 2022, Chuck Lever III wrote:
>> 
>>> On Jul 13, 2022, at 12:32 AM, NeilBrown <neilb@suse.de> wrote:
>>> 
>>> On Wed, 13 Jul 2022, Chuck Lever III wrote:
>> 
>>>>>> Secondarily, the series adds more bells and whistles to the generic
>>>>>> NFSD VFS APIs on behalf of NFSv4-specific requirements. In particular:
>>>>>> 
>>>>>> - ("NFSD: change nfsd_create() to unlock directory before returning.")
>>>>>> makes some big changes to nfsd_create(). But that helper itself
>>>>>> is pretty small. Seems like cleaner code would result if NFSv4
>>>>>> had its own version of nfsd_create() to deal with the post-change
>>>>>> cases.
>>>>> 
>>>>> I would not like that approach. Duplicating code is rarely a good idea.
>>>> 
>>>> De-duplicating code /can/ be a good idea, but isn't always a good
>>>> idea. If the exceptional cases add a lot of logic, that can make the
>>>> de-duplicated code difficult to read and reason about, and it can
>>>> make it brittle, just as it does in this case. Modifications on
>>>> behalf of NFSv4 in this common piece of code is possibly hazardous
>>>> to NFSv3, and navigating around the exception logic makes it
>>>> difficult to understand and review.
>>> 
>>> Are we looking at the same code?
>>> The sum total of extra code needed for v4 is:
>>> - two extra parameters:
>>> 	 void (*post_create)(struct svc_fh *fh, void *data),
>>> 	 void *data)
>>> - two lines of code:
>>> 	if (!err && post_create)
>>> 		post_create(resfhp, data);
>>> 
>>> does that really make anything hard to follow?
>> 
>> The synopsis of nfsd_create() is already pretty cluttered.
> 
> Would it help to collect related details (name, type, attributes, acl,
> label) into a struct and pass a reference to that around?
> One awkwardness in my latest patch is that the acl and label are not set
> in nfsd_create_setattr().  If they were passed around together with the
> attributes, that would be a lot easier to achieve.

That kind of makes my point: using nfsd_create() for both classic
NFSv2/3 and the new NFSv4 hotness overloads the API.

But OK, you seem very set on this. So, let's construct a set
of parameters for a create operation and give the set a
sensible name. The "_args" suffix already has some meaning
and precedence in this code. How about "struct nfsd_create_info" ?


>> You're adding that extra code in nfsd_symlink() too IIRC? And,
>> this change adds a virtual function call (which has significant
>> overhead these days) for reasons of convenience rather than
>> necessity.
> 
> "significant"?  In context of creation a file, I don't think one more
> virtual function call is all that significant?

If you consider how fast underlying storage has become, and how
fast it will become soon (eg, NVRAM-backed filesystems) then the
cost of a virtual function call becomes significant.

And note there is also control flow integrity. A virtual function
can be used to branch into malicious code unless we are careful
to lock it down correctly.

But these are details, and kind of miss my main point.


> I started writing code to avoid the function pointer, but the function
> args list exploded.  If you could be happy with e.g.  a struct
> nfs_create_args which contains attrs, acl, label, and was passed into
> nfsd_create_setattr(), then I think that would cleanly avoid the desire
> for a function pointer.

Note that my complaint was also about adding more logic in these
functions. It isn't much now, but having an "info struct" means
we can pass all kinds of junk into nfsd_create() and add lots
more exceptions to the code path.

Do you see why I really don't like this approach?


--
Chuck Lever