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

[Gao Xiang]

On 2025/5/16 21:50, Sheng Yong wrote:
> Hi Xiang,
>
> On 5/16/25 17:15, Gao Xiang wrote:
> > Hi Yong,
> >
> > On 2025/5/16 17:00, Sheng Yong wrote:
> >
> > ...
> >
> > > diff --git a/Documentation/filesystems/erofs.rst b/Documentation/ filesystems/erofs.rst
> > > index c293f8e37468..b24cb0d5d4d6 100644
> > > --- a/Documentation/filesystems/erofs.rst
> > > +++ b/Documentation/filesystems/erofs.rst
> > > @@ -128,6 +128,7 @@ device=%s              Specify a path to an extra device to be used together.
> > >   fsid=%s                Specify a filesystem image ID for Fscache back-end.
> > >   domain_id=%s           Specify a domain ID in fscache mode so that different images
> > >                          with the same blobs under a given domain ID can share storage.
> > > +fsoffset=%lu           Specify image offset for file-backed or bdev- based mounts.
> >
> > Maybe document it as:
> > fsoffset=%lu              Specify filesystem offset for the primary device.
>
> OK, this makes sense. But if we need to handle offset for extra devices,
> we might need to use an array or a string to temporarily store the values.
> This is because devices are not initialized during parsing parameters.
> And set `off' for each extra device during scan devices.
> For now, I prefer to add fsoffset for the primary device only. I think
> the primary device which has an offset is the more generic case.

Yes, I prefer to handle the primary device only too, but leave
some further extension with the new expression above.

Also I suggest using a new subject as:
"erofs: add 'fsoffset' mount option to specify filesystem offset"

> > Since I'm not sure if we need later
> > fsoffset=%lu,[%lu,...]    Specify filesystem offset for all devices.
> >
> > >   =================== =========================================================
> > >   Sysfs Entries
> > > diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> > > index 2409d2ab0c28..599a44d5d782 100644
> > > --- a/fs/erofs/data.c
> > > +++ b/fs/erofs/data.c
> > > @@ -27,9 +27,12 @@ void erofs_put_metabuf(struct erofs_buf *buf)
> > >   void *erofs_bread(struct erofs_buf *buf, erofs_off_t offset, bool need_kmap)
> > >   {
> > > -    pgoff_t index = offset >> PAGE_SHIFT;
> > > +    pgoff_t index;
> >
> > How about just
> >      pgoff_t index = (offset + buf->off) >> PAGE_SHIFT;
> >
> > since it's not complex to break it into two statements..
> >
> > >       struct folio *folio = NULL;
> > > +    offset += buf->off;
> > > +    index = offset >> PAGE_SHIFT;
> > > +
> > >       if (buf->page) {
> > >           folio = page_folio(buf->page);
> > >           if (folio_file_page(folio, index) != buf->page)
> > > @@ -54,6 +57,7 @@ void erofs_init_metabuf(struct erofs_buf *buf, struct super_block *sb)
> > >       struct erofs_sb_info *sbi = EROFS_SB(sb);
> > >       buf->file = NULL;
> > > +    buf->off = sbi->dif0.off;
> > >       if (erofs_is_fileio_mode(sbi)) {
> > >           buf->file = sbi->dif0.file;    /* some fs like FUSE needs it */
> > >           buf->mapping = buf->file->f_mapping;
> > > @@ -299,7 +303,7 @@ static int erofs_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> > >           iomap->private = buf.base;
> > >       } else {
> > >           iomap->type = IOMAP_MAPPED;
> > > -        iomap->addr = mdev.m_pa;
> > > +        iomap->addr = mdev.m_dif->off + mdev.m_pa;
> >
> > I mean, could we update erofs_init_device() and then
> > `mdev.pa` is already an number added by `mdev.m_dif->off`...
> >
> > Is it possible? since mdev.pa is already a device-based
> > offset.
>
> I think in most cases we can add `off' to mdev.m_pa directly in
> erofs_map_dev(). But for readdir, erofs_read_metabuf(mdev.m_pa)
> is called after erofs_map_dev() in dir's erofs_iomap_begin().
> However, read metabuf needs `off'.

Ok, I see.  Yes, it's somewhat tricky for EROFS_MAP_META,
so okay, let's leave it as-is.

> > >           if (flags & IOMAP_DAX)
> > >               iomap->addr += mdev.m_dif->dax_part_off;
> > >       }
> > > diff --git a/fs/erofs/fileio.c b/fs/erofs/fileio.c
> > > index 60c7cc4c105c..a2c7001ff789 100644
> > > --- a/fs/erofs/fileio.c
> > > +++ b/fs/erofs/fileio.c
> > > @@ -147,7 +147,8 @@ static int erofs_fileio_scan_folio(struct erofs_fileio *io, struct folio *folio)
> > >                   if (err)
> > >                       break;
> > >                   io->rq = erofs_fileio_rq_alloc(&io->dev);
> > > -                io->rq->bio.bi_iter.bi_sector = io->dev.m_pa >> 9;
> > > +                io->rq->bio.bi_iter.bi_sector =
> > > +                    (io->dev.m_dif->off + io->dev.m_pa) >> 9;
> >
> > So we don't need here.
> >
> > >                   attached = 0;
> > >               }unambiguous
> > >               if (!bio_add_folio(&io->rq->bio, folio, len, cur))
> > > diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> > > index 4ac188d5d894..cd8c738f5eb8 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;
> > > +    u64 off;
> > >       struct dax_device *dax_dev;
> > >       u64 dax_part_off;
> >
> > Maybe `u64 off, dax_part_off;` here?
> >
> > Also I'm still not quite sure `off` is unambiguous...
> > Maybe `dataoff`? not quite sure.
>
> What about fsoff accroding to fsoffset?

`fsoff` is fine by me.

Thanks,
Gao Xiang