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

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

[Jeff Layton]

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>
---
 src/client/Client.cc  | 13 +++----------
 src/include/ceph_fs.h |  1 -
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 2df632fc0e08..77f464ad76ae 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -7244,9 +7244,9 @@ int Client::readdir_r_cb(dir_result_t *d, add_dirent_cb_t cb, void *p)
       fill_stat(in, &st);
       fill_dirent(&de, "..", S_IFDIR, st.st_ino, next_off);
     } else {
-      /* must be at the root (no parent),
-       * so we add the dotdot with a special inode (3) */
-      fill_dirent(&de, "..", S_IFDIR, CEPH_INO_DOTDOT, next_off);
+      /* must be at the root (no parent), so fill out with same info as "." */
+      fill_stat(diri, &st);
+      fill_dirent(&de, "..", S_IFDIR, st.st_ino, next_off);
     }
 
 
@@ -9684,13 +9684,6 @@ int Client::ll_getattr(Inode *in, struct stat *attr, int uid, int gid)
   tout(cct) << "ll_getattr" << std::endl;
   tout(cct) << vino.ino.val << std::endl;
 
-  /* special case for dotdot (..) */
-  if (vino.ino.val == CEPH_INO_DOTDOT) {
-    attr->st_mode = S_IFDIR | 0755;
-    attr->st_nlink = 2;
-    return 0;
-  }
-
   int res;
   if (vino.snapid < CEPH_NOSNAP)
     res = 0;
diff --git a/src/include/ceph_fs.h b/src/include/ceph_fs.h
index 772e475ccc91..1610aaf17cc2 100644
--- a/src/include/ceph_fs.h
+++ b/src/include/ceph_fs.h
@@ -28,7 +28,6 @@
 
 #define CEPH_INO_ROOT   1
 #define CEPH_INO_CEPH   2       /* hidden .ceph dir */
-#define CEPH_INO_DOTDOT 3	/* used by ceph fuse for parent (..) */
 #define CEPH_INO_LOST_AND_FOUND 4	/* reserved ino for use in recovery */
 
 /* arbitrary limit on max # of monitors (cluster of 3 is typical) */
-- 
2.7.4

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

[Patrick Donnelly]

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.

-- 
Patrick Donnelly

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

[Jeff Layton]

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;


-- 
Jeff Layton <jlayton@redhat.com>

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

[Patrick Donnelly]

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.

> Note that this patch is not strictly necessary, but it does simplify
> some other changes that I have queued up:

I think the patch is a good change but there may be some other code
paths that need fixed. This change needs some simple tests.

-- 
Patrick Donnelly

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

[Jeff Layton]

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 ?

> > 
> > Note that this patch is not strictly necessary, but it does simplify
> > some other changes that I have queued up:
> 
> I think the patch is a good change but there may be some other code
> paths that need fixed. This change needs some simple tests.
> 

Yeah, agreed. I'll plan to add some if this patch is reasonable. I just
wanted to float the patch out here as an RFC first, in case I was
missing some reason that we needed to keep CEPH_INO_DOTDOT.

Thanks for having a look!
-- 
Jeff Layton <jlayton@redhat.com>

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

[Patrick Donnelly]

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.)

-- 
Patrick Donnelly

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

[Sage Weil]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2032 bytes --]

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

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

[Jeff Layton]

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>

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

[Jeff Layton]

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>

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

[Jeff Layton]

On Wed, 2016-08-10 at 18:21 -0400, Jeff Layton wrote:
> 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 <jlayto
> > > > > > > > > n@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 <j
> > > > > > > > > > > > > > > > > > > > > layton@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" ?
> 

Thinking out loud...

I think we'd want that to return the info for "baz" since anything else
would mean escaping from cmount. So, looking at the code...maybe we
should alter Inode->is_root() to something like:

  bool is_root() { return ino == client->get_root_ino; }

I don't think there are any callers of Inode->is_root currently, so I
wouldn't think this would break anything. Then we could call that to
see if we're at the root when doing a lookup or readdir of "..".

-- 
Jeff Layton <jlayton@redhat.com>