| Message ID | 20171211084758.3225-1-zyan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2017-12-11 at 16:47 +0800, Yan, Zheng wrote: > Zero st_dev confuses ls(1) code. it make 'ls -l' not print '+' sign > for file/directory with ACL. > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > --- > fs/ceph/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index c6ec5aa46100..b5529b0b77a6 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -2244,7 +2244,7 @@ int ceph_getattr(const struct path *path, struct kstat *stat, > if (ceph_snap(inode) != CEPH_NOSNAP) > stat->dev = ceph_snap(inode); > else > - stat->dev = 0; > + stat->dev = 1; /* the smallest snapid is 2 */ > if (S_ISDIR(inode->i_mode)) { > if (ceph_test_mount_opt(ceph_sb_to_client(inode->i_sb), > RBYTES)) This looks wrong to me. This should be getting filled out with the value of the sb->s_dev field in generic_fillattr. The right fix would be to ensure that that field ends up with something valid in it, and that should be happening when we call set_anon_super. I think we need to understand why that isn't happening here.
> On 11 Dec 2017, at 19:59, Jeff Layton <jlayton@redhat.com> wrote: > > On Mon, 2017-12-11 at 16:47 +0800, Yan, Zheng wrote: >> Zero st_dev confuses ls(1) code. it make 'ls -l' not print '+' sign >> for file/directory with ACL. >> >> Signed-off-by: "Yan, Zheng" <zyan@redhat.com> >> --- >> fs/ceph/inode.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c >> index c6ec5aa46100..b5529b0b77a6 100644 >> --- a/fs/ceph/inode.c >> +++ b/fs/ceph/inode.c >> @@ -2244,7 +2244,7 @@ int ceph_getattr(const struct path *path, struct kstat *stat, >> if (ceph_snap(inode) != CEPH_NOSNAP) >> stat->dev = ceph_snap(inode); >> else >> - stat->dev = 0; >> + stat->dev = 1; /* the smallest snapid is 2 */ >> if (S_ISDIR(inode->i_mode)) { >> if (ceph_test_mount_opt(ceph_sb_to_client(inode->i_sb), >> RBYTES)) > > This looks wrong to me. > > This should be getting filled out with the value of the sb->s_dev field > in generic_fillattr. The right fix would be to ensure that that field > ends up with something valid in it, and that should be happening when we > call set_anon_super. > > I think we need to understand why that isn't happening here. I think the reason we set st_dev to snapid is to distinguish head inode from snapped inode. It avoids user programs (eg backup program) from recognizing snapped inode as the same as head inode. (We probably can allocate an anon super for each snapid we encountered) 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
On Mon, 2017-12-11 at 20:19 +0800, Yan, Zheng wrote: > > On 11 Dec 2017, at 19:59, Jeff Layton <jlayton@redhat.com> wrote: > > > > On Mon, 2017-12-11 at 16:47 +0800, Yan, Zheng wrote: > > > Zero st_dev confuses ls(1) code. it make 'ls -l' not print '+' sign > > > for file/directory with ACL. > > > > > > Signed-off-by: "Yan, Zheng" <zyan@redhat.com> > > > --- > > > fs/ceph/inode.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > > index c6ec5aa46100..b5529b0b77a6 100644 > > > --- a/fs/ceph/inode.c > > > +++ b/fs/ceph/inode.c > > > @@ -2244,7 +2244,7 @@ int ceph_getattr(const struct path *path, struct kstat *stat, > > > if (ceph_snap(inode) != CEPH_NOSNAP) > > > stat->dev = ceph_snap(inode); > > > else > > > - stat->dev = 0; > > > + stat->dev = 1; /* the smallest snapid is 2 */ > > > if (S_ISDIR(inode->i_mode)) { > > > if (ceph_test_mount_opt(ceph_sb_to_client(inode->i_sb), > > > RBYTES)) > > > > This looks wrong to me. > > > > This should be getting filled out with the value of the sb->s_dev field > > in generic_fillattr. The right fix would be to ensure that that field > > ends up with something valid in it, and that should be happening when we > > call set_anon_super. > > > > I think we need to understand why that isn't happening here. > > I think the reason we set st_dev to snapid is to distinguish head inode from snapped inode. It avoids user programs (eg backup program) from recognizing snapped inode as the same as head inode. (We probably can allocate an anon super for each snapid we encountered) > Yes, that would make more sense. I'm not sure you need a separate sb for each one (though it would make a lot of sense to do what NFS does here and create a shrinkable mount when the fsid changes). It may be sufficient to just call get_anon_bdev / free_anon_bdev to get a unique device number, and then track them somehow.
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index c6ec5aa46100..b5529b0b77a6 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -2244,7 +2244,7 @@ int ceph_getattr(const struct path *path, struct kstat *stat, if (ceph_snap(inode) != CEPH_NOSNAP) stat->dev = ceph_snap(inode); else - stat->dev = 0; + stat->dev = 1; /* the smallest snapid is 2 */ if (S_ISDIR(inode->i_mode)) { if (ceph_test_mount_opt(ceph_sb_to_client(inode->i_sb), RBYTES))
Zero st_dev confuses ls(1) code. it make 'ls -l' not print '+' sign for file/directory with ACL. Signed-off-by: "Yan, Zheng" <zyan@redhat.com> --- fs/ceph/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)