Re: [PATCH v4 2/2] erofs: add 'fsoffset' mount option for file-backed & bdev-based mounts

[Sheng Yong]

On 5/8/25 12:09, Gao Xiang wrote:
> Hi Yong,
[...]
> > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> > index 4ac188d5d894..10656bd986bd 100644
> > --- a/fs/erofs/internal.h
> > +++ b/fs/erofs/internal.h
> > @@ -43,6 +43,7 @@ struct erofs_device_info {
> >       char *path;
> >       struct erofs_fscache *fscache;
> >       struct file *file;
> > +    loff_t off;
> >       struct dax_device *dax_dev;
> >       u64 dax_part_off;
>
> I wonder if it's possible to combine off as dax_part_off since
> they are the same functionality...

Hi, Xiang,

Thanks for the review. Yes, they almost have the same functionality.
Will combine them.
> > @@ -199,6 +200,7 @@ enum {
> >   struct erofs_buf {
> >       struct address_space *mapping;
> >       struct file *file;
> > +    loff_t off;
>
> I wonder if there is some other way to check
> if it's a metabuf, so that we could just use sbi->dif0.off..
>
> But not sure.

It seems no specific fields could tell directly whether a buf is a
metabuf. But I'll have a try.
> >       struct page *page;
> >       void *base;
> >   };
>
> ..
>
> > +        if (sb->s_bdev)
> > +            devsz = bdev_nr_bytes(sb->s_bdev);
> > +        else if (erofs_is_fileio_mode(sbi))
> > +            devsz = i_size_read(file_inode(sbi->dif0.file));
> > +        else
> > +            return invalfc(fc, "fsoffset only supports file or bdev 
> > backing");
> > +        if (sbi->dif0.off + (1 << sbi->blkszbits) > devsz)
> > +            return invalfc(fc, "fsoffset exceeds device size");
>
> I wonder if those checks are really necessary? even it exceeds
> the device size, it won't find the valid on-disk superblock then.

Will remove this check.
> > +    }
> > +
[...]
> > diff --git a/fs/erofs/zdata.c b/fs/erofs/zdata.c
> > index 0671184d9cf1..671527b63c6d 100644
> > --- a/fs/erofs/zdata.c
> > +++ b/fs/erofs/zdata.c
> > @@ -1624,7 +1624,8 @@ static void z_erofs_submit_queue(struct 
> > z_erofs_frontend *f,
> >                    bool *force_fg, bool readahead)
> >   {
> >       struct super_block *sb = f->inode->i_sb;
> > -    struct address_space *mc = MNGD_MAPPING(EROFS_SB(sb));
> > +    struct erofs_sb_info *sbi = EROFS_SB(sb);
> > +    struct address_space *mc = MNGD_MAPPING(sbi);
> >       struct z_erofs_pcluster **qtail[NR_JOBQUEUES];
> >       struct z_erofs_decompressqueue *q[NR_JOBQUEUES];
> >       struct z_erofs_pcluster *pcl, *next;
> > @@ -1673,12 +1674,15 @@ static void z_erofs_submit_queue(struct 
> > z_erofs_frontend *f,
> >               if (bio && (cur != last_pa ||
> >                       bio->bi_bdev != mdev.m_bdev)) {
> >   drain_io:
> > -                if (erofs_is_fileio_mode(EROFS_SB(sb)))
> > +                if (erofs_is_fileio_mode(sbi)) {
> >                       erofs_fileio_submit_bio(bio);
> > -                else if (erofs_is_fscache_mode(sb))
> > +                } else if (erofs_is_fscache_mode(sb)) {
> >                       erofs_fscache_submit_bio(bio);
> > -                else
> > +                } else {
> > +                    bio->bi_iter.bi_sector +=
> > +                        sbi->dif0.off >> SECTOR_SHIFT;
>
> How about multi-device? I guess we should modify
> erofs_map_dev() directly rather than callers.
Will fix fsoffset on multi device in the next version.

thanks,
shengyong
> Thanks,
> Gao Xiang