ceph: don't set st_dev to 0 for stat(2)
diff mbox

Message ID 20171211084758.3225-1-zyan@redhat.com
State New
Headers show

Commit Message

Yan, Zheng Dec. 11, 2017, 8:47 a.m. UTC
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(-)

Comments

Jeff Layton Dec. 11, 2017, 11:59 a.m. UTC | #1
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.
Yan, Zheng Dec. 11, 2017, 12:19 p.m. UTC | #2
> 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
Jeff Layton Dec. 11, 2017, 12:42 p.m. UTC | #3
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.

Patch
diff mbox

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