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

From: "J. Bruce Fields" <bfields@redhat.com>
To: NeilBrown <neilb@suse.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: Thu, 31 Aug 2017 15:05:32 -0400	[thread overview]
Message-ID: <20170831190531.GA8223@parsley.fieldses.org> (raw)
In-Reply-To: <87wp5kfxi3.fsf@notabene.neil.brown.name>

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.

> You can save your code by passing
>    nfsd4_client_from_rqst(rqst)
> 
> as the 'who', and then testing
>  +		if (dl->dl_stid.sc_client != who) {
> 
> in the loop in nfsd_breaker_owns_lease.  So the only action performed
> on the void* is an equality test.

Yes, that sounds more robust, thanks for the suggestion.

--b.