Re: is the renewal in dentry_lease_is_valid necessary?

[Jeff Layton <jlayton@redhat.com>]

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