Re: [PATCH 2/3] fs: hide another detail of delegation logic

From: NeilBrown <neilb@suse.com>
To: "J. Bruce Fields" <bfields@redhat.com>
Cc: linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Trond Myklebust <trond.myklebust@primarydata.com>
Subject: Re: [PATCH 2/3] fs: hide another detail of delegation logic
Date: Fri, 01 Sep 2017 09:27:54 +1000	[thread overview]
Message-ID: <873787fhc5.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <20170831190531.GA8223@parsley.fieldses.org>

[-- Attachment #1: Type: text/plain, Size: 3432 bytes --]

On Thu, Aug 31 2017, J. Bruce Fields wrote:

> On Thu, Aug 31, 2017 at 09:26:28AM +1000, NeilBrown wrote:
>> On Wed, Aug 30 2017, J. Bruce Fields wrote:
>> 
>> > On Wed, Aug 30, 2017 at 10:43:52AM +1000, NeilBrown wrote:
>> >> I'm suggesting that nfsd have a local "struct deleg_break_ctl" (or
>> >> whatever name you like) which contains a 'struct inode *delegated_inode'
>> >> plus whatever else is useful to nfsd.
>> >> Then nfsd/vfs.c, when it calls things like vfs_unlink(), passes
>> >>  &dbc.delegated_inode
>> >> (where 'struct deleg_break_ctl dbc').
>> >> So the vfs codes doesn't know about 'struct deleg_break_ctl', it just
>> >> knows about the 'struct inode ** inodep' like it does now, though with the
>> >> understanding that "DELEG_NO_WAIT" in the **inodep means that same as
>> >> inodep==NULL.
>> >> 
>> >> The vfs passes this same 'struct **inode' to lm_breaker_owns_lease() and
>> >> the nfsd code uses
>> >>    dbc = container_of(inodep, struct deleg_break_ctl, delegated_inode)
>> >> to get the dbc, and it can use the other fields however it likes.
>> >
>> > Oh, now I understand.  That's an interesting idea.  I don't *think* it
>> > works on its own, because I don't think we've got a way in that case to
>> > know whether the passed-down delegated inode came from nfsd (and thus is
>> > contained in a deleg_break_ctl structure).  We get the
>> > lm_breaker_owns_lease operation from the lease that's already set on the
>> > inode, but we don't know who that breaking operation is coming from.
>> 
>> That is a perfectly valid criticism and one that, I think, applies
>> equally to your original code.
>> 
>>  +static bool nfsd_breaker_owns_lease(void *who, struct file_lock *fl)
>>  +{
>>  +	struct svc_rqst *rqst = who;
>> 
>> How does nfsd know that 'who' is an svc_rqst??
>
> Only nfsd fills in the "who" field of deleg_break_ctl.  But non-nfsd
> users do need to pass a non-NULL delegated_inode.

Yes, of course...

So having been wrong about this code twice, I'm starting to get a feel
for what it does and why.  I still wonder if there might be a better
approach though.

You are changing the interface to pass a magic cookie with the meaning
"Don't bother breaking a delegation which matches this magic cookie".
Would it not be better to pass a delegation, and say "Don't bother
breaking this delegation".  And if it were a WRITE delegation, that
could be optimised as "don't bother breaking any delegation, I have a
write delegation so I have exclusive access".

Whenever we call a vfs_* function that will need to break delegations we
have already done the lookup and have the dentry and inode, so finding a
delegation shouldn't be prohibitive.

nfsd would need to find that delegation, prevent further delegations
being handed out, and check that there aren't already conflicting
delegations.  If there are conflicts, recall them.  Once there are no
conflicting delegations, make the vfs_ request.
One downside of this is that nfsd delegations would be recalled before
any others, rather than doing them all in parallel.  This could be
addressed by calling try_break_deleg() when recalling the nfsd
delegations.

This approach seems to be half-way between your original attempt that
you described, which is racy, and the attempt you posted which adds the
callback that I don't particularly like.

???

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]