Date:   Sat, 18 Feb 2023 10:41:49 +0800
From:   Ming Lei <ming.lei@redhat.com>
To:     David Howells <dhowells@redhat.com>
Cc:     Jens Axboe <axboe@kernel.dk>, Al Viro <viro@zeniv.linux.org.uk>,
        Christoph Hellwig <hch@infradead.org>,
        Matthew Wilcox <willy@infradead.org>, Jan Kara <jack@suse.cz>,
        Jeff Layton <jlayton@kernel.org>,
        David Hildenbrand <david@redhat.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        Logan Gunthorpe <logang@deltatee.com>,
        Hillf Danton <hdanton@sina.com>, linux-fsdevel@vger.kernel.org,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, Christoph Hellwig <hch@lst.de>,
        John Hubbard <jhubbard@nvidia.com>, ming.lei@redhat.com
Subject: Re: [PATCH v14 02/17] splice: Add a func to do a splice from a
 buffered file without ITER_PIPE
Message-ID: <Y/A67a6LovSYHhHz@T590>
References: <20230214171330.2722188-1-dhowells@redhat.com>
 <20230214171330.2722188-3-dhowells@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230214171330.2722188-3-dhowells@redhat.com>
Precedence: bulk

On Tue, Feb 14, 2023 at 05:13:15PM +0000, David Howells wrote:
> Provide a function to do splice read from a buffered file, pulling the
> folios out of the pagecache directly by calling filemap_get_pages() to do
> any required reading and then pasting the returned folios into the pipe.
> 
> A helper function is provided to do the actual folio pasting and will
> handle multipage folios by splicing as many of the relevant subpages as
> will fit into the pipe.
> 
> The code is loosely based on filemap_read() and might belong in
> mm/filemap.c with that as it needs to use filemap_get_pages().
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jens Axboe <axboe@kernel.dk>
> cc: Christoph Hellwig <hch@lst.de>
> cc: Al Viro <viro@zeniv.linux.org.uk>
> cc: David Hildenbrand <david@redhat.com>
> cc: John Hubbard <jhubbard@nvidia.com>
> cc: linux-mm@kvack.org
> cc: linux-block@vger.kernel.org
> cc: linux-fsdevel@vger.kernel.org
> ---
> 
> Notes:
>     ver #14)
>      - Rename to filemap_splice_read().
>      - Create a helper, pipe_head_buf(), to get the head buffer.
>      - Use init_sync_kiocb().
>      - Move to mm/filemap.c.
>      - Split the implementation of filemap_splice_read() from the patch to
>        make generic_file_splice_read() use it and direct_splice_read().
> 
>  include/linux/fs.h |   3 ++
>  mm/filemap.c       | 128 +++++++++++++++++++++++++++++++++++++++++++++
>  mm/internal.h      |   6 +++
>  3 files changed, 137 insertions(+)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c1769a2c5d70..28743e38df91 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3163,6 +3163,9 @@ ssize_t vfs_iocb_iter_write(struct file *file, struct kiocb *iocb,
>  			    struct iov_iter *iter);
>  
>  /* fs/splice.c */
> +ssize_t filemap_splice_read(struct file *in, loff_t *ppos,
> +			    struct pipe_inode_info *pipe,
> +			    size_t len, unsigned int flags);
>  extern ssize_t generic_file_splice_read(struct file *, loff_t *,
>  		struct pipe_inode_info *, size_t, unsigned int);
>  extern ssize_t iter_file_splice_write(struct pipe_inode_info *,
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 876e77278d2a..8c7b135c8e23 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -42,6 +42,8 @@
>  #include <linux/ramfs.h>
>  #include <linux/page_idle.h>
>  #include <linux/migrate.h>
> +#include <linux/pipe_fs_i.h>
> +#include <linux/splice.h>
>  #include <asm/pgalloc.h>
>  #include <asm/tlbflush.h>
>  #include "internal.h"
> @@ -2842,6 +2844,132 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>  }
>  EXPORT_SYMBOL(generic_file_read_iter);
>  
> +/*
> + * Splice subpages from a folio into a pipe.
> + */
> +size_t splice_folio_into_pipe(struct pipe_inode_info *pipe,
> +			      struct folio *folio, loff_t fpos, size_t size)
> +{
> +	struct page *page;
> +	size_t spliced = 0, offset = offset_in_folio(folio, fpos);
> +
> +	page = folio_page(folio, offset / PAGE_SIZE);
> +	size = min(size, folio_size(folio) - offset);
> +	offset %= PAGE_SIZE;
> +
> +	while (spliced < size &&
> +	       !pipe_full(pipe->head, pipe->tail, pipe->max_usage)) {
> +		struct pipe_buffer *buf = pipe_head_buf(pipe);
> +		size_t part = min_t(size_t, PAGE_SIZE - offset, size - spliced);
> +
> +		*buf = (struct pipe_buffer) {
> +			.ops	= &page_cache_pipe_buf_ops,
> +			.page	= page,
> +			.offset	= offset,
> +			.len	= part,
> +		};
> +		folio_get(folio);
> +		pipe->head++;
> +		page++;
> +		spliced += part;
> +		offset = 0;

It should be better to replace above with add_to_pipe().

> +	}
> +
> +	return spliced;
> +}
> +
> +/*
> + * Splice folios from the pagecache of a buffered (ie. non-O_DIRECT) file into
> + * a pipe.
> + */
> +ssize_t filemap_splice_read(struct file *in, loff_t *ppos,
> +			    struct pipe_inode_info *pipe,
> +			    size_t len, unsigned int flags)
> +{
> +	struct folio_batch fbatch;
> +	struct kiocb iocb;
> +	size_t total_spliced = 0, used, npages;
> +	loff_t isize, end_offset;
> +	bool writably_mapped;
> +	int i, error = 0;
> +
> +	init_sync_kiocb(&iocb, in);
> +	iocb.ki_pos = *ppos;
> +
> +	/* Work out how much data we can actually add into the pipe */
> +	used = pipe_occupancy(pipe->head, pipe->tail);
> +	npages = max_t(ssize_t, pipe->max_usage - used, 0);
> +	len = min_t(size_t, len, npages * PAGE_SIZE);

Do we need to consider offset in 1st page here?


thanks, 
Ming