[v20,03/32] splice: Make direct_read_splice() limit to eof where appropriate

  Make direct_read_splice() limit the read to the end of the file for regular
files and block devices, thereby reducing the amount of allocation it will
do in such a case.

This means that the blockdev code doesn't require any special handling as
filemap_read_splice() also limits to i_size.

Signed-off-by: David Howells <dhowells@redhat.com>
cc: Christoph Hellwig <hch@lst.de>
cc: Al Viro <viro@zeniv.linux.org.uk>
cc: Jens Axboe <axboe@kernel.dk>
cc: linux-block@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
cc: linux-mm@kvack.org
---
 fs/splice.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
  

On Fri, May 19, 2023 at 08:40:18AM +0100, David Howells wrote:
> Make direct_read_splice() limit the read to the end of the file for regular
> files and block devices, thereby reducing the amount of allocation it will
> do in such a case.
> 
> This means that the blockdev code doesn't require any special handling as
> filemap_read_splice() also limits to i_size.

I'm really not sure if this is a good idea.  Right now
direct_read_splice (which also appears a little misnamed) really is
a splice by calling ->read_iter helper.  I we don't do any
of this validtion we can just call it directly from splice.c instead
of calling into ->splice_read for direct I/O and DAX and remove a ton
of boilerplate code.

How often do we even call into splice beyond i_size and for how much?

> +	if (S_ISREG(file_inode(in)->i_mode) ||
> +	    S_ISBLK(file_inode(in)->i_mode)) {

Btw, having these kinds of checks in supposedly generic code is always
a marked for a bit of a layering problem.
  

Christoph Hellwig <hch@infradead.org> wrote:

> I'm really not sure if this is a good idea.  Right now
> direct_read_splice (which also appears a little misnamed) really is
> a splice by calling ->read_iter helper.

It can be renamed if you want a different name.  copy_splice_read() or
copy_splice_read_iter()?

> I we don't do any of this validtion we can just call it directly from
> splice.c instead of calling into ->splice_read for direct I/O and DAX and
> remove a ton of boilerplate code.

There's a couple of places where we might not want to do that - at least for
non-DAX.  shmem and f2fs for example.  f2fs calls back to buffered reading
under some circumstances.  shmem ignores O_DIRECT and always splices from the
pagecache.

David
  

On Fri, May 19, 2023 at 09:43:34AM +0100, David Howells wrote:
> > direct_read_splice (which also appears a little misnamed) really is
> > a splice by calling ->read_iter helper.
> 
> It can be renamed if you want a different name.  copy_splice_read() or
> copy_splice_read_iter()?

Maybe something like that, yes.

> 
> > I we don't do any of this validtion we can just call it directly from
> > splice.c instead of calling into ->splice_read for direct I/O and DAX and
> > remove a ton of boilerplate code.
> 
> There's a couple of places where we might not want to do that - at least for
> non-DAX.  shmem and f2fs for example.  f2fs calls back to buffered reading
> under some circumstances.  shmem ignores O_DIRECT and always splices from the
> pagecache.

So?  even if ->read_iter does buffered I/O for O_DIRECT it will still
work.  This can in fact happen for many other file systems due when
they fall back to buffeed I/O due to various reasons.
  

On Fri, May 19, 2023 at 12:41 AM David Howells <dhowells@redhat.com> wrote:
>
> +
> +       if (S_ISREG(file_inode(in)->i_mode) ||
> +           S_ISBLK(file_inode(in)->i_mode)) {

This really feels fundamentally wrong to me.

If block and regular files have this limit, they should have their own
splice_read() function that implements that limit.

Not make everybody else check it.

IOW, this should be a separate function ("block_splice_read()" or
whatever), not inside a generic function that other users use.

The zero size checking looks fine, although I wondered about that too.
Some special files do traditionally have special meanings for
zero-sized reads (as in "packet boundary"). But I suspect that isn't
an issue for splice, and perhaps more importantly, I think the same
rule should be in place: special files that want special rules
shouldn't be using this generic function directly then.

                 Linus
  

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > +       if (S_ISREG(file_inode(in)->i_mode) ||
> > +           S_ISBLK(file_inode(in)->i_mode)) {
> 
> This really feels fundamentally wrong to me.
> 
> If block and regular files have this limit, they should have their own
> splice_read() function that implements that limit.
> 
> Not make everybody else check it.
> 
> IOW, this should be a separate function ("block_splice_read()" or
> whatever), not inside a generic function that other users use.

This is just an optimisation to cut down the amount of bufferage allocated, so
I could just drop it and leave it to userspace for now as the filesystem/block
layer will stop anyway if it hits the EOF.  Christoph would prefer that I call
direct_splice_read() from generic_file_splice_read() in all O_DIRECT cases, if
that's fine with you.

David
  

On Fri, May 19, 2023 at 9:48 AM David Howells <dhowells@redhat.com> wrote:
>
> This is just an optimisation to cut down the amount of bufferage allocated

So the thing is, it's actually very very wrong for some files.

Now, admittedly, those files have other issues too, and it's a design
mistake to begin with, but look at a number of files in /proc.

In particular, look at the regular files that have a size of '0'. It's
quite common indeed. Things like

    /proc/cpuinfo
    /proc/stat
    ...

you can find a ton of them with

    find /proc -type f -size 0

Is it horribly wrong and bad? Yes. I hate it. It means that some
really basic user space tools refuse to work on them, and the tools
are 100% right - this is a kernel misfeature. Trying to do things like

    less -S /proc/cpuinfo

may or may not work depending on your version of 'less', for example,
because it's entirely reasonable to do something like

    fd = open(..);
    if (!fstat(fd, &st))
         len = st.st_size;

and limit your reads to the size of the file - exactly like your patch does.

Except it fails horribly on those broken /proc files.

I hate it, and I blame myself for the above horror, but it's pretty
much unfixable. We could make them look like named pipes or something,
but that's really ugly and probably would break other things anyway.
And we simply don't know the size ahead of time.

Now, *most* things work, because they just do the whole "read until
EOF". In fact, my current version of 'less' has no problem at all
doing the above thing, and gives the "expected" output.

Also, honestly, I really don't think that it's necessarily a good idea
to splice /proc files, but we actually do have splice wired up to
these because people asked for it:

    fe33850ff798 ("proc: wire up generic_file_splice_read for iter ops")
    4bd6a7353ee1 ("sysctl: Convert to iter interfaces")

so I suspect those things do exist.

> I could just drop it and leave it to userspace for now as the filesystem/block
> layer will stop anyway if it hits the EOF.  Christoph would prefer that I call
> direct_splice_read() from generic_file_splice_read() in all O_DIRECT cases, if
> that's fine with you.

I guess that's fine, and for O_DIRECT itself it might even make sense
to do the size test. That said, I doubt it matters: if you use
O_DIRECT on a small file, you only have yourself to blame for doing
something stupid.

And if it isn't a small file, then who cares about some small EOF-time
optimization? Nobody.

So I would suggest not doing that optimization at all, because as-is,
it's either pointless or actively broken.

That said, I would *not* hate some kind of special FMODE_SIZELIMIT
flag that allows filesystems to opt in to "limit reads to size".

We already have flags like that: FMODE_UNSIGNED_OFFSET and
'sb->s_maxbytes' are both basically variations on that same theme, and
having another flag to say "limit reads to i_size" wouldn't be wrong.

It's only wrong when it is done mindlessly with S_ISREG().

             Linus
  

Okay.  Let's go with that.  So I have to put the handling in vfs_splice_read():

	long vfs_splice_read(struct file *in, loff_t *ppos,
			     struct pipe_inode_info *pipe, size_t len,
			     unsigned int flags)
	{
	...
		if (unlikely(!in->f_op->splice_read))
			return warn_unsupported(in, "read");
		/*
		 * O_DIRECT and DAX don't deal with the pagecache, so we
		 * allocate a buffer, copy into it and splice that into the pipe.
		 */
		if ((in->f_flags & O_DIRECT) || IS_DAX(in->f_mapping->host))
			return copy_splice_read(in, ppos, pipe, len, flags);
		return in->f_op->splice_read(in, ppos, pipe, len, flags);
	}

which leaves very little in generic_file_splice_read:

	ssize_t generic_file_splice_read(struct file *in, loff_t *ppos,
					 struct pipe_inode_info *pipe, size_t len,
					 unsigned int flags)
	{
		if (unlikely(*ppos >= in->f_mapping->host->i_sb->s_maxbytes))
			return 0;
		if (unlikely(!len))
			return 0;
		return filemap_splice_read(in, ppos, pipe, len, flags);
	}

so I wonder if the tests in generic_file_splice_read() can be folded into
vfs_splice_read(), pointers to generic_file_splice_read() be replaced with
pointers to filemap_splice_read() and generic_file_splice_read() just be
removed.

I suspect we can't quite do this because of the *ppos check - but I wonder if
that's actually necessary since filemap_splice_read() checks against
i_size... or if the check can be moved there if we definitely want to do it.

Certainly, the zero-length check can be done in vfs_splice_read().

David
  

On Fri, May 19, 2023 at 11:27:51PM +0100, David Howells wrote:
> 	ssize_t generic_file_splice_read(struct file *in, loff_t *ppos,
> 					 struct pipe_inode_info *pipe, size_t len,
> 					 unsigned int flags)
> 	{
> 		if (unlikely(*ppos >= in->f_mapping->host->i_sb->s_maxbytes))
> 			return 0;
> 		if (unlikely(!len))
> 			return 0;
> 		return filemap_splice_read(in, ppos, pipe, len, flags);
> 	}
> 
> so I wonder if the tests in generic_file_splice_read() can be folded into
> vfs_splice_read(), pointers to generic_file_splice_read() be replaced with
> pointers to filemap_splice_read() and generic_file_splice_read() just be
> removed.
> 
> I suspect we can't quite do this because of the *ppos check - but I wonder if
> that's actually necessary since filemap_splice_read() checks against
> i_size... or if the check can be moved there if we definitely want to do it.
> 
> Certainly, the zero-length check can be done in vfs_splice_read().

The zero length check makes sense in vfs_splice_read.  The ppos check
I think makes sense for filemap_splice_read - after all we're dealing
with the page cache here, and the page cache needs a working maxbytes
and i_size.  What callers of filemap_splice_read that don't have the
checks do you have in your tree right now and how did you end up with
them?