Re: [PATCH v1 1/1] fs/splice: add missing callback for inaccessible pages

[Dave Hansen]

On 4/28/20 3:50 PM, Claudio Imbrenda wrote:
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -106,6 +106,9 @@ static int page_cache_pipe_buf_confirm(struct pipe_inode_info *pipe,
>  	struct page *page = buf->page;
>  	int err;
>  
> +	if (arch_make_page_accessible(page))
> +		return -EIO;
> +
>  	if (!PageUptodate(page)) {
>  		lock_page(page);

This is a cute fix, but doesn't it 100% depend on the internal
implementation detail of page cache sendfile() being implemented with a
pipe?  Depending on that seems rather fragile.  While I'm glad that you
surgically plugged the one single, specific case that I pointed out, I
can't help but suspect there are more of these.

For instance, I tried a file-to-file sendfile, basically:

	fd1 = open("file1");
	fd2 = open("file2");
	sendfile(fd1, fd2, ...);

ftrace showed page_cache_pipe_buf_confirm() getting called for the
source pipe pages but not the receiver.  There were no calls to
arch_make_page_accessible() outside of page_cache_pipe_buf_confirm() (I
put a stub in for it on x86 so I could trace it).

That indicates to me that one side of this might be fixed (the sender),
but the receiver is not.

This also doesn't even have the maintainer of fs/splice.c on cc.  The
changelog about what this is trying to do probably also lacks enough
context to bring Al up to speed about what this is trying to do.