From: Zhan Xusheng <zhanxusheng1024@gmail.com>

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

erofs_ishare_fops routes everything that touches the page cache to the
backing file in ->private_data: read_iter clones the iocb onto it, mmap
does vma_set_file(), fadvise calls vfs_fadvise() on it.  splice_read was
left as filemap_splice_read(), which works on the user file's own mapping.

filemap_splice_read() does init_sync_kiocb(&iocb, in), and
filemap_get_pages() then takes iocb->ki_filp->f_mapping, so splice() and
sendfile() populate the per-file page cache from disk instead of using the
shared one.  The data is correct, since erofs_fill_inode() sets that
mapping's a_ops either way, but the same content ends up cached twice,
which is what inode_share is there to avoid.

Pass the backing file, as read_iter already does.

Fixes: 5ef3208e3be5 ("erofs: introduce the page cache share feature")
Link: https://lore.kernel.org/all/b7dc7192-d586-45a2-bc4a-b41dc681c9bb@linux.alibaba.com/
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 fs/erofs/ishare.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/erofs/ishare.c b/fs/erofs/ishare.c
index fa7d4112dec5..01dc53e9e3ad 100644
--- a/fs/erofs/ishare.c
+++ b/fs/erofs/ishare.c
@@ -148,6 +148,13 @@ static int erofs_ishare_mmap(struct file *file, struct vm_area_struct *vma)
 	return generic_file_readonly_mmap(file, vma);
 }
 
+static ssize_t erofs_ishare_splice_read(struct file *in, loff_t *ppos,
+					struct pipe_inode_info *pipe,
+					size_t len, unsigned int flags)
+{
+	return filemap_splice_read(in->private_data, ppos, pipe, len, flags);
+}
+
 static int erofs_ishare_fadvise(struct file *file, loff_t offset,
 				loff_t len, int advice)
 {
@@ -161,7 +168,7 @@ const struct file_operations erofs_ishare_fops = {
 	.mmap		= erofs_ishare_mmap,
 	.release	= erofs_ishare_file_release,
 	.get_unmapped_area = thp_get_unmapped_area,
-	.splice_read	= filemap_splice_read,
+	.splice_read	= erofs_ishare_splice_read,
 	.fadvise	= erofs_ishare_fadvise,
 };
 
-- 
2.43.0

Hi Xusheng,

On Thu, Aug 20, 2026 at 02:44:41PM +0800, Zhan Xusheng wrote:
> From: Zhan Xusheng <zhanxusheng1024@gmail.com>
> 
> From: Zhan Xusheng <zhanxusheng@xiaomi.com>
> 

the subject can be improved as "erofs: support splice() in inode_share mode"

> erofs_ishare_fops routes everything that touches the page cache to the
> backing file in ->private_data: read_iter clones the iocb onto it, mmap
> does vma_set_file(), fadvise calls vfs_fadvise() on it.  splice_read was
> left as filemap_splice_read(), which works on the user file's own mapping.
> 
> filemap_splice_read() does init_sync_kiocb(&iocb, in), and
> filemap_get_pages() then takes iocb->ki_filp->f_mapping, so splice() and
> sendfile() populate the per-file page cache from disk instead of using the
> shared one.  The data is correct, since erofs_fill_inode() sets that
> mapping's a_ops either way, but the same content ends up cached twice,
> which is what inode_share is there to avoid.
> 
> Pass the backing file, as read_iter already does.
> 
> Fixes: 5ef3208e3be5 ("erofs: introduce the page cache share feature")
> Link: https://lore.kernel.org/all/b7dc7192-d586-45a2-bc4a-b41dc681c9bb@linux.alibaba.com/
> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>

I observed the sashiko's report too:
https://sashiko.dev/#/patchset/20260818113713.116849-1-jefflexu%40linux.alibaba.com

Regardless of the implementation details, I don't think it is a fix
since this feature should be considered as a best-effort approach.

If it's an enhancement, please also write an erofs-utils test and
drop the fixes tag.

Thanks,
Gao Xiang

On 8/20/26 2:44 PM, Zhan Xusheng wrote:
> From: Zhan Xusheng <zhanxusheng1024@gmail.com>
> 
> From: Zhan Xusheng <zhanxusheng@xiaomi.com>
> 
> erofs_ishare_fops routes everything that touches the page cache to the
> backing file in ->private_data: read_iter clones the iocb onto it, mmap
> does vma_set_file(), fadvise calls vfs_fadvise() on it.  splice_read was
> left as filemap_splice_read(), which works on the user file's own mapping.
> 
> filemap_splice_read() does init_sync_kiocb(&iocb, in), and
> filemap_get_pages() then takes iocb->ki_filp->f_mapping, so splice() and
> sendfile() populate the per-file page cache from disk instead of using the
> shared one.  The data is correct, since erofs_fill_inode() sets that
> mapping's a_ops either way, but the same content ends up cached twice,
> which is what inode_share is there to avoid.
> 
> Pass the backing file, as read_iter already does.
> 
> Fixes: 5ef3208e3be5 ("erofs: introduce the page cache share feature")
> Link: https://lore.kernel.org/all/b7dc7192-d586-45a2-bc4a-b41dc681c9bb@linux.alibaba.com/
> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
> ---
>  fs/erofs/ishare.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/erofs/ishare.c b/fs/erofs/ishare.c
> index fa7d4112dec5..01dc53e9e3ad 100644
> --- a/fs/erofs/ishare.c
> +++ b/fs/erofs/ishare.c
> @@ -148,6 +148,13 @@ static int erofs_ishare_mmap(struct file *file, struct vm_area_struct *vma)
>  	return generic_file_readonly_mmap(file, vma);
>  }
>  
> +static ssize_t erofs_ishare_splice_read(struct file *in, loff_t *ppos,
> +					struct pipe_inode_info *pipe,
> +					size_t len, unsigned int flags)
> +{
> +	return filemap_splice_read(in->private_data, ppos, pipe, len, flags);

Please refer to backing_file_splice_read() called from
ovl_splice_read(), file_accessed() needs to be called on the original
file (just as what .read_iter() i.e. filemap_read() does), and the input
@ppos needs to be updated accordingly.


-- 
Thanks,
Jingbo

On Thu, 20 Aug 2026 17:53:20 +0800, Jingbo Xu wrote:
> Please refer to backing_file_splice_read() called from
> ovl_splice_read(), file_accessed() needs to be called on the original
> file (just as what .read_iter() i.e. filemap_read() does), and the input
> @ppos needs to be updated accordingly.

Taking the file_accessed() one, thanks.  filemap_splice_read() calls it at
mm/filemap.c:3155 on whatever file it was handed, so on the backing file,
whereas backing_file_splice_read() ends in ctx->accessed(iocb->ki_filp),
which for ovl_splice_read() is the original.  v2 adds file_accessed(in).

@ppos looks already handled to me.  filemap_splice_read() takes a loff_t *
and advances it itself, at mm/filemap.c:3144; its internal kiocb is seeded
from *ppos at 3083 and 3098, not the other way round.  ovl_splice_read()
has to copy iocb.ki_pos back because backing_file_splice_read() takes a
struct kiocb and hands &iocb->ki_pos to vfs_splice_read().  Say if I have
that wrong.

One you may want for read_iter too: it clones the kiocb onto the backing
file, so filemap_read() marks that one accessed rather than the user's
file, which is the shape splice_read had.  Neither is observable today,
since erofs_fc_fill_super() sets SB_RDONLY | SB_NOATIME and the backing
file is opened O_NOATIME, so both reach a no-op.  That is why I left
read_iter alone here.

Thanks,
Zhan Xusheng

On 8/20/26 8:37 PM, Zhan Xusheng wrote:
> On Thu, 20 Aug 2026 17:53:20 +0800, Jingbo Xu wrote:
>> Please refer to backing_file_splice_read() called from
>> ovl_splice_read(), file_accessed() needs to be called on the original
>> file (just as what .read_iter() i.e. filemap_read() does), and the input
>> @ppos needs to be updated accordingly.
> 
> Taking the file_accessed() one, thanks.  filemap_splice_read() calls it at
> mm/filemap.c:3155 on whatever file it was handed, so on the backing file,
> whereas backing_file_splice_read() ends in ctx->accessed(iocb->ki_filp),
> which for ovl_splice_read() is the original.  v2 adds file_accessed(in).
> 
> @ppos looks already handled to me.  filemap_splice_read() takes a loff_t *
> and advances it itself, at mm/filemap.c:3144; its internal kiocb is seeded
> from *ppos at 3083 and 3098, not the other way round.  ovl_splice_read()
> has to copy iocb.ki_pos back because backing_file_splice_read() takes a
> struct kiocb and hands &iocb->ki_pos to vfs_splice_read().  Say if I have
> that wrong.

Make sense.


> 
> One you may want for read_iter too: it clones the kiocb onto the backing
> file, so filemap_read() marks that one accessed rather than the user's
> file, which is the shape splice_read had.  Neither is observable today,
> since erofs_fc_fill_super() sets SB_RDONLY | SB_NOATIME and the backing
> file is opened O_NOATIME, so both reach a no-op.  That is why I left
> read_iter alone here.

Okay, it seems that file_accessed() shall also be added to
erofs_ishare_file_read_iter()?


-- 
Thanks,
Jingbo

On Thu, Aug 20, 2026 at 09:34:49PM +0800, Jingbo Xu wrote:
> 
> 
> On 8/20/26 8:37 PM, Zhan Xusheng wrote:
> > On Thu, 20 Aug 2026 17:53:20 +0800, Jingbo Xu wrote:
> >> Please refer to backing_file_splice_read() called from
> >> ovl_splice_read(), file_accessed() needs to be called on the original
> >> file (just as what .read_iter() i.e. filemap_read() does), and the input
> >> @ppos needs to be updated accordingly.
> > 
> > Taking the file_accessed() one, thanks.  filemap_splice_read() calls it at
> > mm/filemap.c:3155 on whatever file it was handed, so on the backing file,
> > whereas backing_file_splice_read() ends in ctx->accessed(iocb->ki_filp),
> > which for ovl_splice_read() is the original.  v2 adds file_accessed(in).
> > 
> > @ppos looks already handled to me.  filemap_splice_read() takes a loff_t *
> > and advances it itself, at mm/filemap.c:3144; its internal kiocb is seeded
> > from *ppos at 3083 and 3098, not the other way round.  ovl_splice_read()
> > has to copy iocb.ki_pos back because backing_file_splice_read() takes a
> > struct kiocb and hands &iocb->ki_pos to vfs_splice_read().  Say if I have
> > that wrong.
> 
> Make sense.
> 
> 
> > 
> > One you may want for read_iter too: it clones the kiocb onto the backing
> > file, so filemap_read() marks that one accessed rather than the user's
> > file, which is the shape splice_read had.  Neither is observable today,
> > since erofs_fc_fill_super() sets SB_RDONLY | SB_NOATIME and the backing
> > file is opened O_NOATIME, so both reach a no-op.  That is why I left
> > read_iter alone here.
> 
> Okay, it seems that file_accessed() shall also be added to
> erofs_ishare_file_read_iter()?

I think file_accessed() is a no-op for erofs?

Thanks,
Gao Xiang

> 
> 
> -- 
> Thanks,
> Jingbo
> 
>

On 8/20/26 9:49 PM, Gao Xiang wrote:
> On Thu, Aug 20, 2026 at 09:34:49PM +0800, Jingbo Xu wrote:
>>
>>
>> On 8/20/26 8:37 PM, Zhan Xusheng wrote:
>>> On Thu, 20 Aug 2026 17:53:20 +0800, Jingbo Xu wrote:
>>>> Please refer to backing_file_splice_read() called from
>>>> ovl_splice_read(), file_accessed() needs to be called on the original
>>>> file (just as what .read_iter() i.e. filemap_read() does), and the input
>>>> @ppos needs to be updated accordingly.
>>>
>>> Taking the file_accessed() one, thanks.  filemap_splice_read() calls it at
>>> mm/filemap.c:3155 on whatever file it was handed, so on the backing file,
>>> whereas backing_file_splice_read() ends in ctx->accessed(iocb->ki_filp),
>>> which for ovl_splice_read() is the original.  v2 adds file_accessed(in).
>>>
>>> @ppos looks already handled to me.  filemap_splice_read() takes a loff_t *
>>> and advances it itself, at mm/filemap.c:3144; its internal kiocb is seeded
>>> from *ppos at 3083 and 3098, not the other way round.  ovl_splice_read()
>>> has to copy iocb.ki_pos back because backing_file_splice_read() takes a
>>> struct kiocb and hands &iocb->ki_pos to vfs_splice_read().  Say if I have
>>> that wrong.
>>
>> Make sense.
>>
>>
>>>
>>> One you may want for read_iter too: it clones the kiocb onto the backing
>>> file, so filemap_read() marks that one accessed rather than the user's
>>> file, which is the shape splice_read had.  Neither is observable today,
>>> since erofs_fc_fill_super() sets SB_RDONLY | SB_NOATIME and the backing
>>> file is opened O_NOATIME, so both reach a no-op.  That is why I left
>>> read_iter alone here.
>>
>> Okay, it seems that file_accessed() shall also be added to
>> erofs_ishare_file_read_iter()?
> 
> I think file_accessed() is a no-op for erofs?

Right. erofs unconditionally sets SB_NOATIME (sb->s_flags |= SB_RDONLY |
SB_NOATIME).  Please ignore the noise..

-- 
Thanks,
Jingbo