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.
