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

[NeilBrown <neilb@suse.com>]

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

On Tue, Aug 29 2017, J. Bruce Fields wrote:

> On Mon, Aug 28, 2017 at 02:43:05PM +1000, NeilBrown wrote:
>> On Fri, Aug 25 2017, J. Bruce Fields wrote:
>> 
>> > From: "J. Bruce Fields" <bfields@redhat.com>
>> >
>> > Pass around a new struct deleg_break_ctl instead of pointers to inode
>> > pointers; in a future patch I want to use this to pass a little more
>> > information from the nfs server to the lease code.
>> 
>> The information you are passing from the nfs server to the lease code is
>> largely ignored by the lease code and is passed back to the nfs server,
>> in the sm_breaker_owns_lease call back.
>> 
>> If try_break_deleg() passed the 'delegated_inode' pointer though to
>> __break_lease(), it could pass it through any_leases_conflict() and
>> leases_conflict() to the lm_breaker_owns_lease() callback.
>> Then container_of() could be used to access whatever other data nfsd had
>> stashed near the inode.  The common code wouldn't need to know any of
>> the details.
>
> The new information that we need is some notion of "who" (really, which
> NFSv4 client) is doing the operation (unlink, whatever) that breaks the
> lease.  We can't get that information from an inode pointer.
>
> I may just not understand your suggestion.

Probably I was too terse.

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.

Then instead of the rather task-specific name "lm_breaker_owns_lease" we
could have a more general name like "lm_lease_compatible" ... or
something.  "lm_break_doesn't_see_this_lease_as_being_in_conflict" is a
bit long, and contains "'".

Thanks,
NeilBrown

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