Date:   Tue, 17 May 2022 17:15:02 +0800
From:   Gao Xiang <hsiangkao@...ux.alibaba.com>
To:     Christian Brauner <brauner@...nel.org>
Cc:     Chao Yu <chao@...nel.org>, xiang@...nel.org,
        linux-erofs@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        Chao Yu <chao.yu@...o.com>, fsdevel@...r.kernel.org
Subject: Re: [PATCH] erofs: support idmapped mounts

Hi Christian,

On Tue, May 17, 2022 at 11:06:22AM +0200, Christian Brauner wrote:
> On Tue, May 17, 2022 at 03:32:10PM +0800, Chao Yu wrote:
> > This patch enables idmapped mounts for erofs, since all dedicated helpers
> > for this functionality existsm, so, in this patch we just pass down the
> > user_namespace argument from the VFS methods to the relevant helpers.
> > 
> > Simple idmap example on erofs image:
> > 
> > 1. mkdir dir
> > 2. touch dir/file
> > 3. mkfs.erofs erofs.img dir
> > 4. mount -t erofs -o loop erofs.img  /mnt/erofs/
> > 
> > 5. ls -ln /mnt/erofs/
> > total 0
> > -rw-rw-r-- 1 1000 1000 0 May 17 15:26 file
> > 
> > 6. mount-idmapped --map-mount b:0:1001:1 /mnt/erofs/ /mnt/scratch_erofs/
> > 
> > 7. ls -ln /mnt/scratch_erofs/
> > total 0
> > -rw-rw-r-- 1 65534 65534 0 May 17 15:26 file
> 
> Your current example maps id 0 in the filesystem to id 1001 in the
> mount. But since no files with id 0 exist in the filesystem you're
> illustrating that unmapped ids are correctly reported as overflow{g,u}id.
> 
> I think what you'd rather want to show is something like this:
> 
> 5. ls -ln /mnt/erofs/
> total 0
> -rw-rw-r-- 1 1000 1000 0 May 17 15:26 file
> 
> 6. mount-idmapped --map-mount b:1000:1001:1 /mnt/erofs/ /mnt/scratch_erofs/
> 
> 7. ls -ln /mnt/scratch_erofs/
> total 0
> -rw-rw-r-- 1 1001 1001 0 May 17 15:26 file
> 
> where id 1000 in the filesystem maps to id 1001 in the mount.
> 
> > 
> > Signed-off-by: Chao Yu <chao.yu@...o.com>
> > ---
> 
> Overall this is currently the smallest patch to support idmapped mounts.
> 
> Is erofs integrated with xfstests in any way?
> For read-only filesystems we probably only need to verify that {g,u}id
> are correctly reported. All the writable aspects are irrelevant.

Currently most generic xfstests test cases are unsuitable for erofs.

Instead we have regression testcases for EROFS specific since it needs
to generate images with care,
 https://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs-utils.git/log/?h=experimental-tests

Also we have an erofsstress to do long time random stress workloads,
https://github.com/erofs/erofsstress

But yeah, it's some awkward that fstests idmapped mount testcases may
be unsuitable for EROFS for now. I will add some new testcases to build
images and test for this behavior.

> 
> Looks good,
> Reviewed-by: Christian Brauner (Microsoft) <brauner@...nel.org>

Thanks for your review!

Thanks,
Gao Xiang

> 
> >  fs/erofs/inode.c | 2 +-
> >  fs/erofs/super.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> > index e8b37ba5e9ad..5320bf52c1ce 100644
> > --- a/fs/erofs/inode.c
> > +++ b/fs/erofs/inode.c
> > @@ -370,7 +370,7 @@ int erofs_getattr(struct user_namespace *mnt_userns, const struct path *path,
> >  	stat->attributes_mask |= (STATX_ATTR_COMPRESSED |
> >  				  STATX_ATTR_IMMUTABLE);
> >  
> > -	generic_fillattr(&init_user_ns, inode, stat);
> > +	generic_fillattr(mnt_userns, inode, stat);
> >  	return 0;
> >  }
> >  
> > diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> > index 0c4b41130c2f..7dc5f2e8ddee 100644
> > --- a/fs/erofs/super.c
> > +++ b/fs/erofs/super.c
> > @@ -781,7 +781,7 @@ static struct file_system_type erofs_fs_type = {
> >  	.name           = "erofs",
> >  	.init_fs_context = erofs_init_fs_context,
> >  	.kill_sb        = erofs_kill_sb,
> > -	.fs_flags       = FS_REQUIRES_DEV,
> > +	.fs_flags       = FS_REQUIRES_DEV | FS_ALLOW_IDMAP,
> >  };
> >  MODULE_ALIAS_FS("erofs");
> >  
> > -- 
> > 2.25.1
> > 

Powered by blists - more mailing lists