Re: [PATCH 22/30] squashfs: Convert squashfs_symlink_read_folio to use folio APIs

[Phillip Lougher <phillip@squashfs.org.uk>]

On 20/04/2024 03:50, Matthew Wilcox (Oracle) wrote:
> Remove use of page APIs, return the errno instead of 0, switch from
> kmap_atomic to kmap_local and use folio_end_read() to unify the two
> exit paths.
> 
> Cc: Phillip Lougher <phillip@squashfs.org.uk>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Tested-by: Phillip Lougher <phillip@squashfs.org.uk>
Reviewed-by: Phillip Lougher <phillip@squashfs.org.uk>

You've mentioned a couple of times you prefer the patches in
the series to go through the fs maintainers.  Andrew Morton is
currently handling submission of Squashfs patches for me, and
I'm happy with either Andrew or you merging it.

CC'ing Andrew.

Regards

Phillip

> ---
>   fs/squashfs/symlink.c | 35 ++++++++++++++++-------------------
>   1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/squashfs/symlink.c b/fs/squashfs/symlink.c
> index 2bf977a52c2c..6ef735bd841a 100644
> --- a/fs/squashfs/symlink.c
> +++ b/fs/squashfs/symlink.c
> @@ -32,20 +32,19 @@
>   
>   static int squashfs_symlink_read_folio(struct file *file, struct folio *folio)
>   {
> -	struct page *page = &folio->page;
> -	struct inode *inode = page->mapping->host;
> +	struct inode *inode = folio->mapping->host;
>   	struct super_block *sb = inode->i_sb;
>   	struct squashfs_sb_info *msblk = sb->s_fs_info;
> -	int index = page->index << PAGE_SHIFT;
> +	int index = folio_pos(folio);
>   	u64 block = squashfs_i(inode)->start;
>   	int offset = squashfs_i(inode)->offset;
>   	int length = min_t(int, i_size_read(inode) - index, PAGE_SIZE);
> -	int bytes, copied;
> +	int bytes, copied, error;
>   	void *pageaddr;
>   	struct squashfs_cache_entry *entry;
>   
>   	TRACE("Entered squashfs_symlink_readpage, page index %ld, start block "
> -			"%llx, offset %x\n", page->index, block, offset);
> +			"%llx, offset %x\n", folio->index, block, offset);
>   
>   	/*
>   	 * Skip index bytes into symlink metadata.
> @@ -57,14 +56,15 @@ static int squashfs_symlink_read_folio(struct file *file, struct folio *folio)
>   			ERROR("Unable to read symlink [%llx:%x]\n",
>   				squashfs_i(inode)->start,
>   				squashfs_i(inode)->offset);
> -			goto error_out;
> +			error = bytes;
> +			goto out;
>   		}
>   	}
>   
>   	/*
>   	 * Read length bytes from symlink metadata.  Squashfs_read_metadata
>   	 * is not used here because it can sleep and we want to use
> -	 * kmap_atomic to map the page.  Instead call the underlying
> +	 * kmap_local to map the folio.  Instead call the underlying
>   	 * squashfs_cache_get routine.  As length bytes may overlap metadata
>   	 * blocks, we may need to call squashfs_cache_get multiple times.
>   	 */
> @@ -75,29 +75,26 @@ static int squashfs_symlink_read_folio(struct file *file, struct folio *folio)
>   				squashfs_i(inode)->start,
>   				squashfs_i(inode)->offset);
>   			squashfs_cache_put(entry);
> -			goto error_out;
> +			error = entry->error;
> +			goto out;
>   		}
>   
> -		pageaddr = kmap_atomic(page);
> +		pageaddr = kmap_local_folio(folio, 0);
>   		copied = squashfs_copy_data(pageaddr + bytes, entry, offset,
>   								length - bytes);
>   		if (copied == length - bytes)
>   			memset(pageaddr + length, 0, PAGE_SIZE - length);
>   		else
>   			block = entry->next_index;
> -		kunmap_atomic(pageaddr);
> +		kunmap_local(pageaddr);
>   		squashfs_cache_put(entry);
>   	}
>   
> -	flush_dcache_page(page);
> -	SetPageUptodate(page);
> -	unlock_page(page);
> -	return 0;
> -
> -error_out:
> -	SetPageError(page);
> -	unlock_page(page);
> -	return 0;
> +	flush_dcache_folio(folio);
> +	error = 0;
> +out:
> +	folio_end_read(folio, error == 0);
> +	return error;
>   }
>   
>