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

[Gao Xiang <hsiangkao@linux.alibaba.com>]

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