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

[Sheng Yong <shengyong2021@gmail.com>]

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