is the renewal in dentry_lease_is_valid necessary?

is the renewal in dentry_lease_is_valid necessary?

[Jeff Layton]

I'm eyeballing how to make ceph dentry revalidation occur in rcu-walk
mode in the kernel cephfs client. It looks fairly straightforward, but
for one thing:

dentry_lease_is_valid queues an asynchronous lease renewal if we're
halfway to the next lease renewal time:

        if (session) {
                ceph_mdsc_lease_send_msg(session, dir, dentry,
                                         CEPH_MDS_LEASE_RENEW, seq);
                ceph_put_mds_session(session);
        }

That can sleep, so we have to drop out of rcu walk mode in that case.

Is this lease renewal strictly necessary? I can make the code drop out
of rcuwalk mode and try again in that case, but if this is just an
opportunistic thing, then it may be better for performance to take a
pass on renewing the lease early if we're still in RCU walk mode.

-- 
Jeff Layton <jlayton@redhat.com>

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: is the renewal in dentry_lease_is_valid necessary?

[Gregory Farnum]

On Mon, Jun 27, 2016 at 12:07 PM, Jeff Layton <jlayton@redhat.com> wrote:
> I'm eyeballing how to make ceph dentry revalidation occur in rcu-walk
> mode in the kernel cephfs client. It looks fairly straightforward, but
> for one thing:
>
> dentry_lease_is_valid queues an asynchronous lease renewal if we're
> halfway to the next lease renewal time:
>
>         if (session) {
>                 ceph_mdsc_lease_send_msg(session, dir, dentry,
>                                          CEPH_MDS_LEASE_RENEW, seq);
>                 ceph_put_mds_session(session);
>         }
>
> That can sleep, so we have to drop out of rcu walk mode in that case.
>
> Is this lease renewal strictly necessary? I can make the code drop out
> of rcuwalk mode and try again in that case, but if this is just an
> opportunistic thing, then it may be better for performance to take a
> pass on renewing the lease early if we're still in RCU walk mode.

I'm just guessing based on the function names (and what I know of the
algorithms), but as long as something *else* will do the renewal, it
shouldn't need to be done here. There's nothing magic about half the
lease time.
-Greg

Re: is the renewal in dentry_lease_is_valid necessary?

[Yan, Zheng]

On Tue, Jun 28, 2016 at 3:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
> I'm eyeballing how to make ceph dentry revalidation occur in rcu-walk
> mode in the kernel cephfs client. It looks fairly straightforward, but
> for one thing:
>
> dentry_lease_is_valid queues an asynchronous lease renewal if we're
> halfway to the next lease renewal time:
>
>         if (session) {
>                 ceph_mdsc_lease_send_msg(session, dir, dentry,
>                                          CEPH_MDS_LEASE_RENEW, seq);
>                 ceph_put_mds_session(session);
>         }
>
> That can sleep, so we have to drop out of rcu walk mode in that case.
>
> Is this lease renewal strictly necessary? I can make the code drop out
> of rcuwalk mode and try again in that case, but if this is just an
> opportunistic thing, then it may be better for performance to take a
> pass on renewing the lease early if we're still in RCU walk mode.
>

dentry_lease_is_valid is the only place that renew lease. I think it's
better to drop out of rcuwalk and try again

Regards
Yan, Zheng


> --
> Jeff Layton <jlayton@redhat.com>
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: is the renewal in dentry_lease_is_valid necessary?

[Jeff Layton]

On Tue, 2016-06-28 at 21:03 +0800, Yan, Zheng wrote:
> On Tue, Jun 28, 2016 at 3:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > I'm eyeballing how to make ceph dentry revalidation occur in rcu-walk
> > mode in the kernel cephfs client. It looks fairly straightforward, but
> > for one thing:
> > 
> > dentry_lease_is_valid queues an asynchronous lease renewal if we're
> > halfway to the next lease renewal time:
> > 
> >         if (session) {
> >                 ceph_mdsc_lease_send_msg(session, dir, dentry,
> >                                          CEPH_MDS_LEASE_RENEW, seq);
> >                 ceph_put_mds_session(session);
> >         }
> > 
> > That can sleep, so we have to drop out of rcu walk mode in that case.
> > 
> > Is this lease renewal strictly necessary? I can make the code drop out
> > of rcuwalk mode and try again in that case, but if this is just an
> > opportunistic thing, then it may be better for performance to take a
> > pass on renewing the lease early if we're still in RCU walk mode.
> > 
> dentry_lease_is_valid is the only place that renew lease. I think it's
> better to drop out of rcuwalk and try again
> 
> Regards
> Yan, Zheng
> 

That seems really odd to me.

That call is only ever done in the d_revalidate codepath, so you'll
only get a lease renewal when you have repeated path-based access to an
existing dentry.

Maybe this means that I don't really understand ceph leases. More
questions...

What exactly does the lease cover here?

Shouldn't we be renewing it even if we're _not_ doing any path-based
access? For instance, if I open a file and issue reads and writes
against it, that doesn't involve pathnames and the lease doesn't get an
explicit renewal. Doesn't it still need to be renewed though?

-- 
Jeff Layton <jlayton@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: is the renewal in dentry_lease_is_valid necessary?

[Gregory Farnum]

On Tue, Jun 28, 2016 at 7:00 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Tue, 2016-06-28 at 21:03 +0800, Yan, Zheng wrote:
>> On Tue, Jun 28, 2016 at 3:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> >
>> > I'm eyeballing how to make ceph dentry revalidation occur in rcu-walk
>> > mode in the kernel cephfs client. It looks fairly straightforward, but
>> > for one thing:
>> >
>> > dentry_lease_is_valid queues an asynchronous lease renewal if we're
>> > halfway to the next lease renewal time:
>> >
>> >         if (session) {
>> >                 ceph_mdsc_lease_send_msg(session, dir, dentry,
>> >                                          CEPH_MDS_LEASE_RENEW, seq);
>> >                 ceph_put_mds_session(session);
>> >         }
>> >
>> > That can sleep, so we have to drop out of rcu walk mode in that case.
>> >
>> > Is this lease renewal strictly necessary? I can make the code drop out
>> > of rcuwalk mode and try again in that case, but if this is just an
>> > opportunistic thing, then it may be better for performance to take a
>> > pass on renewing the lease early if we're still in RCU walk mode.
>> >
>> dentry_lease_is_valid is the only place that renew lease. I think it's
>> better to drop out of rcuwalk and try again
>>
>> Regards
>> Yan, Zheng
>>
>
> That seems really odd to me.
>
> That call is only ever done in the d_revalidate codepath, so you'll
> only get a lease renewal when you have repeated path-based access to an
> existing dentry.
>
> Maybe this means that I don't really understand ceph leases. More
> questions...
>
> What exactly does the lease cover here?
>
> Shouldn't we be renewing it even if we're _not_ doing any path-based
> access? For instance, if I open a file and issue reads and writes
> against it, that doesn't involve pathnames and the lease doesn't get an
> explicit renewal. Doesn't it still need to be renewed though?

Ah. Leases cover the dentries, and are separate from the capabilities
covering file IO and the inode itself. So actually, no, if nobody is
doing dentry operations (directory listing, name resolution, etc) we
don't care.

This isn't a part of the protocol I'm very familiar with (it doesn't
really come up in userspace code), but that might mean it's easier to
understand if you look at the userspace version of it...?
-Greg

Re: is the renewal in dentry_lease_is_valid necessary?

[Yan, Zheng]

> On Jun 28, 2016, at 22:00, Jeff Layton <jlayton@redhat.com> wrote:
> 
> On Tue, 2016-06-28 at 21:03 +0800, Yan, Zheng wrote:
>> On Tue, Jun 28, 2016 at 3:07 AM, Jeff Layton <jlayton@redhat.com> wrote:
>>> 
>>> I'm eyeballing how to make ceph dentry revalidation occur in rcu-walk
>>> mode in the kernel cephfs client. It looks fairly straightforward, but
>>> for one thing:
>>> 
>>> dentry_lease_is_valid queues an asynchronous lease renewal if we're
>>> halfway to the next lease renewal time:
>>> 
>>>         if (session) {
>>>                 ceph_mdsc_lease_send_msg(session, dir, dentry,
>>>                                          CEPH_MDS_LEASE_RENEW, seq);
>>>                 ceph_put_mds_session(session);
>>>         }
>>> 
>>> That can sleep, so we have to drop out of rcu walk mode in that case.
>>> 
>>> Is this lease renewal strictly necessary? I can make the code drop out
>>> of rcuwalk mode and try again in that case, but if this is just an
>>> opportunistic thing, then it may be better for performance to take a
>>> pass on renewing the lease early if we're still in RCU walk mode.
>>> 
>> dentry_lease_is_valid is the only place that renew lease. I think it's
>> better to drop out of rcuwalk and try again
>> 
>> Regards
>> Yan, Zheng
>> 
> 
> That seems really odd to me.
> 
> That call is only ever done in the d_revalidate codepath, so you'll
> only get a lease renewal when you have repeated path-based access to an
> existing dentry.
> 
> Maybe this means that I don't really understand ceph leases. More
> questions...
> 
> What exactly does the lease cover here?

Lease covers dentry. MDS revoke release before changing dentry’s linkage. If client access a dentry frequently, it should renew the dentry lease when necessary. If dentry lease expires, client need to send a lookup request if it want to access the dentry.

Yan, Zheng 

> 
> Shouldn't we be renewing it even if we're _not_ doing any path-based
> access? For instance, if I open a file and issue reads and writes
> against it, that doesn't involve pathnames and the lease doesn't get an
> explicit renewal. Doesn't it still need to be renewed though?
> 
> -- 
> Jeff Layton <jlayton@redhat.com>

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: is the renewal in dentry_lease_is_valid necessary?

[Jeff Layton]

On Wed, 2016-06-29 at 14:53 +0800, Yan, Zheng wrote:
> > 
> > On Jun 28, 2016, at 22:00, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > On Tue, 2016-06-28 at 21:03 +0800, Yan, Zheng wrote:
> > > 
> > > On Tue, Jun 28, 2016 at 3:07 AM, Jeff Layton <jlayton@redhat.com>
> > > wrote:
> > > > 
> > > > 
> > > > I'm eyeballing how to make ceph dentry revalidation occur in
> > > > rcu-walk
> > > > mode in the kernel cephfs client. It looks fairly
> > > > straightforward, but
> > > > for one thing:
> > > > 
> > > > dentry_lease_is_valid queues an asynchronous lease renewal if
> > > > we're
> > > > halfway to the next lease renewal time:
> > > > 
> > > >         if (session) {
> > > >                 ceph_mdsc_lease_send_msg(session, dir, dentry,
> > > >                                          CEPH_MDS_LEASE_RENEW,
> > > > seq);
> > > >                 ceph_put_mds_session(session);
> > > >         }
> > > > 
> > > > That can sleep, so we have to drop out of rcu walk mode in that
> > > > case.
> > > > 
> > > > Is this lease renewal strictly necessary? I can make the code
> > > > drop out
> > > > of rcuwalk mode and try again in that case, but if this is just
> > > > an
> > > > opportunistic thing, then it may be better for performance to
> > > > take a
> > > > pass on renewing the lease early if we're still in RCU walk
> > > > mode.
> > > > 
> > > dentry_lease_is_valid is the only place that renew lease. I think
> > > it's
> > > better to drop out of rcuwalk and try again
> > > 
> > > Regards
> > > Yan, Zheng
> > > 
> > That seems really odd to me.
> > 
> > That call is only ever done in the d_revalidate codepath, so you'll
> > only get a lease renewal when you have repeated path-based access
> > to an
> > existing dentry.
> > 
> > Maybe this means that I don't really understand ceph leases. More
> > questions...
> > 
> > What exactly does the lease cover here?
> Lease covers dentry. MDS revoke release before changing dentry’s
> linkage. If client access a dentry frequently, it should renew the
> dentry lease when necessary. If dentry lease expires, client need to
> send a lookup request if it want to access the dentry.
> 

Ok, got it, thanks. In that case, what might be better is to queue the
lease renewal to a workqueue and stay in RCU lookup mode. It's not like
we're planning to wait around on the reply so dropping out of RCU mode
is probably not what we want. I'll take a look at whether it makes
sense to embed a work_struct in the ceph_dentry_info.

-- 
Jeff Layton <jlayton@redhat.com>

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html