Re: [PATCH 13/20] xfs: don't allow highmem pages in xfile mappings

[Hugh Dickins <hughd@google.com>]

On Mon, 29 Jan 2024, Christoph Hellwig wrote:

> XFS is generally used on 64-bit, non-highmem platforms and xfile
> mappings are accessed all the time.  Reduce our pain by not allowing
> any highmem mappings in the xfile page cache and remove all the kmap
> calls for it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/scrub/xfarray.c |  3 +--
>  fs/xfs/scrub/xfile.c   | 21 +++++++++------------
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/scrub/xfarray.c b/fs/xfs/scrub/xfarray.c
> index 95ac14bceeadd6..d0f98a43b2ba0a 100644
> --- a/fs/xfs/scrub/xfarray.c
> +++ b/fs/xfs/scrub/xfarray.c
> @@ -580,7 +580,7 @@ xfarray_sort_get_page(
>  	 * xfile pages must never be mapped into userspace, so we skip the
>  	 * dcache flush when mapping the page.
>  	 */
> -	si->page_kaddr = kmap_local_page(si->xfpage.page);
> +	si->page_kaddr = page_address(si->xfpage.page);
>  	return 0;
>  }
>  
> @@ -592,7 +592,6 @@ xfarray_sort_put_page(
>  	if (!si->page_kaddr)
>  		return 0;
>  
> -	kunmap_local(si->page_kaddr);
>  	si->page_kaddr = NULL;
>  
>  	return xfile_put_page(si->array->xfile, &si->xfpage);
> diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c
> index 7e915385ef0011..a669ebbbc02d1d 100644
> --- a/fs/xfs/scrub/xfile.c
> +++ b/fs/xfs/scrub/xfile.c
> @@ -77,6 +77,12 @@ xfile_create(
>  	inode = file_inode(xf->file);
>  	lockdep_set_class(&inode->i_rwsem, &xfile_i_mutex_key);
>  
> +	/*
> +	 * We don't want to bother with kmapping data during repair, so don't
> +	 * allow highmem pages to back this mapping.
> +	 */
> +	mapping_set_gfp_mask(inode->i_mapping, GFP_KERNEL);

I had been going to point you to inode_nohighmem(inode), which is how
that is usually done.  But I share Matthew's uncertainty about cpusets
and the __GFP_HARDWALL inside GFP_USER: you may well be right to stick
with GFP_KERNEL, just as you've done it here.  I suppose it depends on
whether you see this as a system operation or a user-invoked operation.

I did also think that perhaps you could mask off __GFP_FS here, to
avoid having to remember those memalloc_nofs_save()s elsewhere; but
that's probably a bad idea, I doubt anywhere else does it that way.

So, no change required, but I wanted to think about it.

Hugh

> +
>  	trace_xfile_create(xf);
>  
>  	*xfilep = xf;
> @@ -126,7 +132,6 @@ xfile_load(
>  
>  	pflags = memalloc_nofs_save();
>  	while (count > 0) {
> -		void		*p, *kaddr;
>  		unsigned int	len;
>  
>  		len = min_t(ssize_t, count, PAGE_SIZE - offset_in_page(pos));
> @@ -153,10 +158,7 @@ xfile_load(
>  		 * xfile pages must never be mapped into userspace, so
>  		 * we skip the dcache flush.
>  		 */
> -		kaddr = kmap_local_page(page);
> -		p = kaddr + offset_in_page(pos);
> -		memcpy(buf, p, len);
> -		kunmap_local(kaddr);
> +		memcpy(buf, page_address(page) + offset_in_page(pos), len);
>  		put_page(page);
>  
>  advance:
> @@ -221,14 +223,13 @@ xfile_store(
>  		 * the dcache flush.  If the page is not uptodate, zero it
>  		 * before writing data.
>  		 */
> -		kaddr = kmap_local_page(page);
> +		kaddr = page_address(page);
>  		if (!PageUptodate(page)) {
>  			memset(kaddr, 0, PAGE_SIZE);
>  			SetPageUptodate(page);
>  		}
>  		p = kaddr + offset_in_page(pos);
>  		memcpy(p, buf, len);
> -		kunmap_local(kaddr);
>  
>  		ret = aops->write_end(NULL, mapping, pos, len, len, page,
>  				fsdata);
> @@ -314,11 +315,7 @@ xfile_get_page(
>  	 * to the caller and make sure the backing store will hold on to them.
>  	 */
>  	if (!PageUptodate(page)) {
> -		void	*kaddr;
> -
> -		kaddr = kmap_local_page(page);
> -		memset(kaddr, 0, PAGE_SIZE);
> -		kunmap_local(kaddr);
> +		memset(page_address(page), 0, PAGE_SIZE);
>  		SetPageUptodate(page);
>  	}
>  
> -- 
> 2.39.2