On Wed, Mar 8, 2023 at 8:53 AM David Howells <dhowells@xxxxxxxxxx> wrote:
>
> The new filemap_splice_read() has an implicit expectation via
> filemap_get_pages() that ->read_folio() exists if ->readahead() doesn't
> fully populate the pagecache of the file it is reading from[1], potentially
> leading to a jump to NULL if this doesn't exist. shmem, however, (and by
> extension, tmpfs, ramfs and rootfs), doesn't have ->read_folio(),
This patch is the only one in your series that I went "Ugh, that's
really ugly" for.
Do we really want to basically duplicate all of filemap_splice_read()?
I get the feeling that the zeropage case just isn't so important that
we'd need to duplicate filemap_splice_read() just for that, and I
think that the code should either
(a) just make a silly "read_folio()" for shmfs that just clears the page.
Ugly but maybe simple and not horrid?
or
(b) teach filemap_splice_read() that a NULL 'read_folio' function
means "use the zero page"
That might not be splice() itself, but maybe in
filemap_get_pages() or something.
or
(c) go even further, and teach read_folio() in general about file
holes, and allow *any* filesystem to read zeroes that way in general
without creating a folio for it.
in a perfect world, if done well I think shmem_file_read_iter() should
go away, and it could use generic_file_read_iter too.
I dunno. Maybe shm really is *so* special that this is the right way
to do things, but I did react quite negatively to this patch. So not a
complete NAK, but definitely a "do we _really_ have to do this?"
Linus
