On Wed, Aug 30 2017, J. Bruce Fields wrote:
> On Wed, Aug 30, 2017 at 10:43:52AM +1000, NeilBrown wrote:
>> 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@xxxxxxxxxx>
>> >> >
>> >> > 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.
>
> 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??
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.
I cannot save my code quite so easily. :-(
Thanks,
NeilBrown
>
> Maybe something alon those lines could be made to work.
>
>> 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 "'".
>
> Hah, yes.--b.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
