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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Mar 2023 14:39:00 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     David Howells <dhowells@...hat.com>
Cc:     Jens Axboe <axboe@...nel.dk>, Al Viro <viro@...iv.linux.org.uk>,
        Christoph Hellwig <hch@...radead.org>,
        Matthew Wilcox <willy@...radead.org>, Jan Kara <jack@...e.cz>,
        Jeff Layton <jlayton@...nel.org>,
        David Hildenbrand <david@...hat.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Logan Gunthorpe <logang@...tatee.com>,
        Hillf Danton <hdanton@...a.com>, linux-fsdevel@...r.kernel.org,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Daniel Golle <daniel@...rotopia.org>,
        Guenter Roeck <groeck7@...il.com>,
        Christoph Hellwig <hch@....de>,
        John Hubbard <jhubbard@...dia.com>,
        Hugh Dickins <hughd@...gle.com>
Subject: Re: [PATCH v17 03/14] shmem: Implement splice-read

On Wed, Mar 8, 2023 at 8:53 AM David Howells <dhowells@...hat.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?

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