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

Viacheslav Dubeyko
2026-01-08T20:11:36Z

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.