Re: [PATCH 15/15] xfs: use xfile_get_page and xfile_put_page in xfile_obj_load

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jan 03, 2024 at 08:41:26AM +0000, Christoph Hellwig wrote:
> Switch xfile_obj_load to use xfile_get_page and xfile_put_page to access
> the shmem page cache.  The former uses shmem_get_folio(..., SGP_READ),
> which returns a NULL folio for a hole instead of allocating one to
> optimize the case where the caller is reading from a whole and doesn't
> want to a zeroed folio to the page cache.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/scrub/xfile.c | 51 +++++++++++---------------------------------
>  1 file changed, 12 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c
> index 987b03df241b02..3f9e416376d2f7 100644
> --- a/fs/xfs/scrub/xfile.c
> +++ b/fs/xfs/scrub/xfile.c
> @@ -34,13 +34,6 @@
>   * xfiles assume that the caller will handle all required concurrency
>   * management; standard vfs locks (freezer and inode) are not taken.  Reads
>   * and writes are satisfied directly from the page cache.
> - *
> - * NOTE: The current shmemfs implementation has a quirk that in-kernel reads
> - * of a hole cause a page to be mapped into the file.  If you are going to
> - * create a sparse xfile, please be careful about reading from uninitialized
> - * parts of the file.  These pages are !Uptodate and will eventually be
> - * reclaimed if not written, but in the short term this boosts memory
> - * consumption.

Awright, this goes away finally!

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>   */
>  
>  /*
> @@ -117,58 +110,38 @@ xfile_obj_load(
>  	size_t			count,
>  	loff_t			pos)
>  {
> -	struct inode		*inode = file_inode(xf->file);
> -	struct address_space	*mapping = inode->i_mapping;
> -	struct page		*page = NULL;
> -	unsigned int		pflags;
> -	int			error = 0;
> -
>  	if (count > MAX_RW_COUNT)
>  		return -ENOMEM;
> -	if (inode->i_sb->s_maxbytes - pos < count)
> +	if (file_inode(xf->file)->i_sb->s_maxbytes - pos < count)
>  		return -ENOMEM;
>  
>  	trace_xfile_obj_load(xf, pos, count);
>  
> -	pflags = memalloc_nofs_save();
>  	while (count > 0) {
> +		struct page	*page;
>  		unsigned int	len;
>  
>  		len = min_t(ssize_t, count, PAGE_SIZE - offset_in_page(pos));
> -
> -		/*
> -		 * In-kernel reads of a shmem file cause it to allocate a page
> -		 * if the mapping shows a hole.  Therefore, if we hit ENOMEM
> -		 * we can continue by zeroing the caller's buffer.
> -		 */
> -		page = shmem_read_mapping_page_gfp(mapping, pos >> PAGE_SHIFT,
> -				__GFP_NOWARN);
> -		if (IS_ERR(page)) {
> -			error = PTR_ERR(page);
> -			if (error != -ENOMEM) {
> -				error = -ENOMEM;
> -				break;
> -			}
> -
> +		page = xfile_get_page(xf, pos, len, 0);
> +		if (IS_ERR(page))
> +			return -ENOMEM;
> +		if (!page) {
> +			/*
> +			 * No data stored at this offset, just zero the output
> +			 * buffer.
> +			 */
>  			memset(buf, 0, len);
>  			goto advance;
>  		}
> -
> -		/*
> -		 * xfile pages must never be mapped into userspace, so
> -		 * we skip the dcache flush.
> -		 */
>  		memcpy(buf, page_address(page) + offset_in_page(pos), len);
> -		put_page(page);
> -
> +		xfile_put_page(xf, page);
>  advance:
>  		count -= len;
>  		pos += len;
>  		buf += len;
>  	}
> -	memalloc_nofs_restore(pflags);
>  
> -	return error;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.39.2
> 
>