Re: [PATCH v2 4/6] ceph: Split out page-array discarding to a function

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Wed, 2026-01-07 at 13:01 -0800, Sam Edwards wrote:
> Discarding a page array (i.e. after failure to submit it) is a little
> complex:
> - Every folio in the batch needs to be redirtied and unlocked.
> - Some folios are bounce pages created for fscrypt; the underlying
>   plaintext folios also need to be redirtied and unlocked.
> - The array itself can come either from the mempool or general kalloc,
>   so different free functions need to be used depending on which.
> 
> Although currently only ceph_submit_write() does this, this logic is
> complex enough to warrant its own function. Move it to a new
> ceph_discard_page_array() function that is called by ceph_submit_write()
> instead.
> 
> Suggested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  fs/ceph/addr.c | 67 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 38 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 467aa7242b49..3becb13a09fe 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -1222,6 +1222,43 @@ void ceph_allocate_page_array(struct address_space *mapping,
>  	ceph_wbc->len = 0;
>  }
>  
> +static inline
> +void ceph_discard_page_array(struct writeback_control *wbc,
> +			     struct ceph_writeback_ctl *ceph_wbc)
> +{
> +	int i;
> +	struct page *page;
> +
> +	for (i = 0; i < folio_batch_count(&ceph_wbc->fbatch); i++) {
> +		struct folio *folio = ceph_wbc->fbatch.folios[i];
> +
> +		if (!folio)
> +			continue;
> +
> +		page = &folio->page;
> +		redirty_page_for_writepage(wbc, page);
> +		unlock_page(page);
> +	}
> +
> +	for (i = 0; i < ceph_wbc->locked_pages; i++) {
> +		page = ceph_fscrypt_pagecache_page(ceph_wbc->pages[i]);
> +
> +		if (!page)
> +			continue;
> +
> +		redirty_page_for_writepage(wbc, page);
> +		unlock_page(page);
> +	}
> +
> +	if (ceph_wbc->from_pool) {
> +		mempool_free(ceph_wbc->pages, ceph_wb_pagevec_pool);
> +		ceph_wbc->from_pool = false;
> +	} else
> +		kfree(ceph_wbc->pages);
> +	ceph_wbc->pages = NULL;
> +	ceph_wbc->locked_pages = 0;
> +}
> +
>  static inline
>  bool is_folio_index_contiguous(const struct ceph_writeback_ctl *ceph_wbc,
>  			      const struct folio *folio)
> @@ -1445,35 +1482,7 @@ int ceph_submit_write(struct address_space *mapping,
>  	BUG_ON(len < ceph_fscrypt_page_offset(page) + thp_size(page) - offset);
>  
>  	if (!ceph_inc_osd_stopping_blocker(fsc->mdsc)) {
> -		for (i = 0; i < folio_batch_count(&ceph_wbc->fbatch); i++) {
> -			struct folio *folio = ceph_wbc->fbatch.folios[i];
> -
> -			if (!folio)
> -				continue;
> -
> -			page = &folio->page;
> -			redirty_page_for_writepage(wbc, page);
> -			unlock_page(page);
> -		}
> -
> -		for (i = 0; i < ceph_wbc->locked_pages; i++) {
> -			page = ceph_fscrypt_pagecache_page(ceph_wbc->pages[i]);
> -
> -			if (!page)
> -				continue;
> -
> -			redirty_page_for_writepage(wbc, page);
> -			unlock_page(page);
> -		}
> -
> -		if (ceph_wbc->from_pool) {
> -			mempool_free(ceph_wbc->pages, ceph_wb_pagevec_pool);
> -			ceph_wbc->from_pool = false;
> -		} else
> -			kfree(ceph_wbc->pages);
> -		ceph_wbc->pages = NULL;
> -		ceph_wbc->locked_pages = 0;
> -
> +		ceph_discard_page_array(wbc, ceph_wbc);
>  		ceph_osdc_put_request(req);
>  		return -EIO;
>  	}

This patch makes sense to me. Looks good.

Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.