Re: [PATCH v17 03/14] shmem: Implement splice-read

[Matthew Wilcox <willy@infradead.org>]

On Wed, Mar 08, 2023 at 02:39:00PM -0800, Linus Torvalds wrote:
> On Wed, Mar 8, 2023 at 8:53 AM David Howells <dhowells@redhat.com> 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?

The problem is that we might have swapped out the shmem folio.  So we
don't want to clear the page, but ask swap to fill the page.  The way
that currently works (see shmem_get_folio_gfp()) is to fetch the swap
entry from the page cache, allocate a new folio inside the shmem code,
then replace the swap entry with the new folio.

What I'd like to see is the generic code say "Ah, this is a shmem
inode, so it's special and the xa_value entry is swap information,
not workingset information, so I'll allocate the folio and restore
the folio->private swap information to let the shmem_read_folio
function do its job correctly".

Either that or we completely overhaul the shmem code to store the
location of its swapped data somewhere that's not the page cache.

>  (b) teach filemap_splice_read() that a NULL 'read_folio' function
> means "use the zero page"

Same problem as (a).

>  (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.

I've had thoughts along those lines in the past.  It's pretty major
surgery, I think.  At the moment, we allocate the pages and add them
to the page cache in a locked state before asking the filesystem to
populate them.  So the fs doesn't even have the file layout (eg the
get_block or iomap info) that would tell it where the holes are until
the page has already been allocated and inserted.  We could of course
free the page and replace it with a special 'THIS_IS_A_HOLE' entry.
It's just never seemed important enuogh to me to do this surgery.

> 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?"

I'd really like to see shmem have a read_folio implementation.  I
don't know how much work it's going to be.