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

From: Jeff Layton <jlayton@redhat.com>
To: Patrick Donnelly <pdonnell@redhat.com>, Sage Weil <sage@redhat.com>
Cc: Ceph Development <ceph-devel@vger.kernel.org>
Subject: Re: [RFC PATCH] client: don't use special inode for /..
Date: Wed, 10 Aug 2016 18:21:45 -0400	[thread overview]
Message-ID: <1470867705.2694.30.camel@redhat.com> (raw)
In-Reply-To: <CA+2bHPaDy-UiEvve3y0f6eCmxTvCx5UUV9yvjUP=_J0eqCWJJQ@mail.gmail.com>

On Wed, 2016-08-10 at 17:15 -0400, Patrick Donnelly wrote:
> > On Wed, Aug 10, 2016 at 5:06 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > 
> > On Wed, 2016-08-10 at 16:46 -0400, Patrick Donnelly wrote:
> > > 
> > > > 
> > > > On Wed, Aug 10, 2016 at 4:30 PM, Jeff Layton <jlayton@redhat.com> 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?
> > > 
> > > Well, that's tricky actually. My understanding is that if dn_set is
> > > empty then either the inode is unlinked or it is the root inode (from
> > > the client's perspective). So the below patch is probably not quite
> > > right? I think if the directory is unlinked but not the root, its ".."
> > > should still refer to its first parent? The ENOENT error is probably
> > > wrong.
> > > 
> > 
> > Ok, so is there some way to reliably tell whether it's the root? Should
> > we instead check whether it's inode number is CEPH_INO_ROOT ?
> 
> Inode::is_root should work. By the way, I see now that the readdir
> code is also wrong. It should not need to check dn_set at all (just
> is_root()). (It doesn't matter if the directory inode is unlinked, the
> parent is still visible.)
> 

Ahh thanks. I'll see about fixing that up while I'm in there too.

Your point about is_root is valid, but I think we should step back a
min and consider how we expect this to work when we mount a subtree of
the MDS root.

Suppose I do:

    ceph_mount(&cmount, "/foo/bar/baz");
    ceph_lookup(&cmount, "/..", &st);

...what should we ultimately end up stat'ing there? Should I get back
the info for "bar" or "baz" ?

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