Re: [PATCH v2 08/10] ceph: convert page cleanup loop in writepages_finish() to folios

On 8/4/26 2:52 PM, Tal Zussman wrote:
> Convert the page cleanup loop in writepages_finish() to work on folios,
> resolving the folio directly after fscrypt_finalize_bounce_page() has
> replaced any bounce page with its pagecache page.
> 
> This removes a use of detach_page_private() and five calls to
> compound_head() per page, while adding one back via page_folio().
> 
> No functional change.
> 
> Signed-off-by: Tal Zussman <[email protected]>
> ---
>  fs/ceph/addr.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 59e559bdfdff..eaf5b3f6d13f 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -896,7 +896,6 @@ static void writepages_finish(struct ceph_osd_request *req)
>  	struct ceph_inode_info *ci = ceph_inode(inode);
>  	struct ceph_client *cl = ceph_inode_to_client(inode);
>  	struct ceph_osd_data *osd_data;
> -	struct page *page;
>  	int num_pages, total_pages = 0;
>  	int i, j;
>  	int rc = req->r_result;
> @@ -943,31 +942,33 @@ static void writepages_finish(struct ceph_osd_request *req)
>  					   (u64)osd_data->length);
>  		total_pages += num_pages;
>  		for (j = 0; j < num_pages; j++) {
> +			struct folio *folio;
> +
>  			fscrypt_finalize_bounce_page(&osd_data->pages[j]);
> -			page = osd_data->pages[j];
> -			BUG_ON(!page);
> -			WARN_ON(!PageUptodate(page));
> +			folio = page_folio(osd_data->pages[j]);
> +			BUG_ON(!folio);
> +			WARN_ON(!folio_test_uptodate(folio));

Sashiko complains:

"Since page_folio() invokes _compound_head() which dereferences the page
pointer, won't this cause a NULL pointer dereference before reaching the
BUG_ON(!folio) check if osd_data->pages[j] is actually NULL?
The previous code checked the page pointer before any dereference occurred.
Would it be safer to check the array element for NULL before passing it to
page_folio()?"

The BUG_ON can just be removed, as fscrypt_finalize_bounce_page() cannot take
or set its argument to NULL as constructed, so the check was unnecessary
to begin with.

>  
>  			if (atomic_long_dec_return(&fsc->writeback_count) <
>  			     CONGESTION_OFF_THRESH(
>  					fsc->mount_options->congestion_kb))
>  				fsc->write_congested = false;
>  
> -			ceph_put_snap_context(detach_page_private(page));
> -			end_page_writeback(page);
> +			ceph_put_snap_context(folio_detach_private(folio));
> +			folio_end_writeback(folio);
>  
>  			if (atomic64_dec_return(&mdsc->dirty_folios) <= 0) {
>  				wake_up_all(&mdsc->flush_end_wq);
>  				WARN_ON(atomic64_read(&mdsc->dirty_folios) < 0);
>  			}
>  
> -			doutc(cl, "unlocking %p\n", page);
> +			doutc(cl, "unlocking %p\n", folio);
>  
>  			if (remove_page)
>  				generic_error_remove_folio(inode->i_mapping,
> -							  page_folio(page));
> +							  folio);
>  
> -			unlock_page(page);
> +			folio_unlock(folio);
>  		}
>  		doutc(cl, "%llx.%llx wrote %llu bytes cleaned %d pages\n",
>  		      ceph_vinop(inode), osd_data->length,
>