[PATCH] ceph: don't set st_dev to 0 for stat(2)

[PATCH] ceph: don't set st_dev to 0 for stat(2)

[Yan, Zheng]

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

Re: [PATCH] ceph: don't set st_dev to 0 for stat(2)

[Jeff Layton]

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.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] ceph: don't set st_dev to 0 for stat(2)

[Yan, Zheng]

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

Re: [PATCH] ceph: don't set st_dev to 0 for stat(2)

[Jeff Layton]

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.

-- 
Jeff Layton <jlayton@redhat.com>