Re: [RFC PATCH] client: don't use special inode for /..

[Jeff Layton <jlayton@redhat.com>]

On Wed, 2016-08-10 at 21:23 +0000, Sage Weil wrote:
> On Wed, 10 Aug 2016, Jeff Layton wrote:
> > On Wed, 2016-08-10 at 16:08 -0400, Patrick Donnelly wrote:
> > > > On Wed, Aug 10, 2016 at 12:30 PM, Jeff Layton <jlayton@redhat.com>
> > > wrote:
> > > > 
> > > > The CEPH_INO_DOTDOT thing is quite strange. Under most OS (Linux
> > > > included), the parent of the root is itself. IOW, at the root, '.'
> > > > and
> > > > '..' refer to the same inode.
> > > > 
> > > > Change the ceph client to do the same, as this allows users to get
> > > > valid stat info for '..', as well as elimnating some special-
> > > > casing.
> > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > 
> > > Don't forget Client::_lookup:
> > > 
> > >   if (dname == "..") {
> > >     if (dir->dn_set.empty())
> > >       r = -ENOENT;
> > >     else
> > >       *target = dir->get_first_parent()->dir->parent_inode; //dirs
> > > can't be hard-linked
> > >     goto done;
> > >   }
> > > 
> > > Otherwise LGTM.
> > > 
> > 
> > 
> > Ahh, thanks. So will dir->dn_set.empty() be true at the root? If so,
> > then something like the patch below?
> > 
> > Note that this patch is not strictly necessary, but it does simplify
> > some other changes that I have queued up:
> > 
> > diff --git a/src/client/Client.cc b/src/client/Client.cc
> > index 5ab0ace4d3df..287baaf20536 100644
> > --- a/src/client/Client.cc
> > +++ b/src/client/Client.cc
> > @@ -5924,7 +5924,7 @@ int Client::_lookup(Inode *dir, const string& dname, int mask,
> >  
> >    if (dname == "..") {
> >      if (dir->dn_set.empty())
> > -      r = -ENOENT;
> > +      *target = dir;
> >      else
> >        *target = dir->get_first_parent()->dir->parent_inode; //dirs can't be hard-linked
> >      goto done;
> 
> IIRC I did the dotdot thing originally because otherwise the '..' entry at 
> the mount point in ls -al didn't point to the parent directory.  Having 
> the fs explicitly do .. at all seems pretty weird to me... it seems like 
> the VFS should be doing this.  But in any case, I'd just verify that it 
> behaves the same way a real mount does after this change.
> 
> sage


The Linux VFS will definitely already handle ".." correctly (as you end
up doing a transition to a different vfsmount). So this shouldn't
affect ceph-fuse, AFAICT.

I think this change would primarily be noticed by those using libcephfs
directly...either in ceph_readdir/ceph_lookup (and related) calls, or
during a pathwalk.

That said, Patrick's suggestion to add some tests around this makes
sense. I'll plan to spin that up so we can be clear on how the behavior
changes.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>