Re: [PATCH] isofs: convert the zisofs read path to use folios

[Jan Kara]

On Sun 06-09-26 22:14:59, Tal Zussman wrote:
> Store folios in the array that zisofs_read_folio() passes to
> zisofs_fill_pages() and zisofs_uncompress_block(), use the folio APIs on
> them, and grab the folios with __filemap_get_folio() instead of
> grab_cache_page_nowait(). This removes six compound_head() calls, from
> page_offset(), the three SetPageUptodate() calls, unlock_page() and
> put_page(), and the last struct page usage in isofs.
> 
> grab_cache_page_nowait() passed FGP_NOFS as well, but FGP_NOWAIT already
> prevents reclaim entirely, so leave it out, as FGP_NOFS is on its way
> out [1].
> 
> Change poffset to be unsigned int rather than just unsigned while at
> it.
> 
> [1] https://lore.kernel.org/linux-mm/20260830041901.2668-9-willy@xxxxxxxxxxxxx/
> 
> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>

Thanks for the conversion! The patch looks mostly good, some smaller
comments below:

> @@ -66,11 +66,12 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
>  	if (block_size == 0) {
>  		for ( i = 0 ; i < pcount ; i++ ) {
>  			unsigned int off = i ? 0 : poffset;
> +			struct folio *folio = folios[i];
>  
> -			if (!pages[i])
> +			if (!folio)
>  				continue;
> -			memzero_page(pages[i], off, PAGE_SIZE - off);
> -			SetPageUptodate(pages[i]);
> +			folio_zero_range(folio, off, PAGE_SIZE - off);
						     ^^ perhaps
"folio_size(folio) - off" here?

> +			folio_mark_uptodate(folio);
>  		}
>  		return (((loff_t)pcount) << PAGE_SHIFT) - poffset;
>  	}
...
> @@ -119,9 +120,11 @@ static loff_t zisofs_uncompress_block(struct inode *inode, loff_t block_start,
>  
>  	while (curpage < pcount && curbh < haveblocks &&
>  	       zerr != Z_STREAM_END) {
> +		struct folio *folio = folios[curpage];
> +
>  		if (!stream.avail_out) {
> -			if (pages[curpage]) {
> -				stream.next_out = kmap_local_page(pages[curpage])
> +			if (folio) {
> +				stream.next_out = kmap_local_folio(folio, 0)
>  						+ poffset;
						  ^^^ this is just:
					kmap_local_folio(folio, poffset);

>  				stream.avail_out = PAGE_SIZE - poffset;

And this would be probably more idiomatic as "folio_size(folio) - poffset"
after the conversion.

...
> @@ -289,9 +292,11 @@ static int zisofs_fill_pages(struct inode *inode, int full_page, int pcount,
>  		cstart_block++;
>  	}
>  
> -	if (poffset && *pages) {
> -		memzero_page(*pages, poffset, PAGE_SIZE - poffset);
> -		SetPageUptodate(*pages);
> +	if (poffset && *folios) {
> +		struct folio *folio = *folios;
> +
> +		folio_zero_range(folio, poffset, PAGE_SIZE - poffset);
						 ^^^ again
folio_size(folio) here?

> +		folio_mark_uptodate(folio);
>  	}
>  	brelse(bh);
>  	return 0;

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR