| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6b456e0d-04cf-4ecd-a23a-e91c7d58b592@linux.alibaba.com>
Date: Fri, 16 May 2025 22:04:51 +0800
From: Gao Xiang <hsiangkao@...ux.alibaba.com>
To: Sheng Yong <shengyong2021@...il.com>, xiang@...nel.org, chao@...nel.org,
zbestahu@...il.com, jefflexu@...ux.alibaba.com, dhavale@...gle.com,
lihongbo22@...wei.com
Cc: linux-erofs@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
Sheng Yong <shengyong1@...omi.com>, Wang Shuai <wangshuai12@...omi.com>
Subject: Re: [PATCH v6] erofs: add 'fsoffset' mount option for file-backed &
bdev-based mounts
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
Powered by blists - more mailing lists