Message-ID: <CAH5Ym4jnsYNp7Y5icBtQJZvX_JW=nvprj61ZH1XDX3Js0ePggA@mail.gmail.com>
Date: Mon, 5 Jan 2026 22:52:17 -0800
From: Sam Edwards <cfsworks@...il.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>
Cc: Xiubo Li <xiubli@...hat.com>, "idryomov@...il.com" <idryomov@...il.com>, 
	Milind Changire <mchangir@...hat.com>, 
	"ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>, "brauner@...nel.org" <brauner@...nel.org>, 
	"jlayton@...nel.org" <jlayton@...nel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] ceph: Remove error return from ceph_process_folio_batch()

On Mon, Jan 5, 2026 at 12:36 PM Viacheslav Dubeyko
<Slava.Dubeyko@....com> wrote:
>
> On Tue, 2025-12-30 at 18:43 -0800, Sam Edwards wrote:
> > Following the previous patch, ceph_process_folio_batch() no longer
> > returns errors because the writeback loop cannot handle them.
>

Hi Slava,

> I am not completely convinced that we can remove returning error code here. What
> if we don't process any folio in ceph_process_folio_batch(), then we cannot call
> the ceph_submit_write(). It sounds to me that we need to have error code to jump
> to release_folios in such case.

This goto is already present (search the comment "did we get anything?").

>
> >
> > Since this function already indicates failure to lock any pages by
> > leaving `ceph_wbc.locked_pages == 0`, and the writeback loop has no way
>
> Frankly speaking, I don't quite follow what do you mean by "this function
> already indicates failure to lock any pages". What do you mean here?

I feel like there's a language barrier here. I understand from your
homepage that you speak Russian. I believe the Russian translation of
what I'm trying to say is:

Эта функция уже сигнализирует о том, что ни одна страница не была
заблокирована, поскольку ceph_wbc.locked_pages остаётся равным 0.

It's likely that I didn't phrase the English version clearly enough.
Do you have a clearer phrasing I could use? This is the central point
of this patch, so it's crucial that it's well-understood.

>
> > to handle abandonment of a locked batch, change the return type of
> > ceph_process_folio_batch() to `void` and remove the pathological goto in
> > the writeback loop. The lack of a return code emphasizes that
> > ceph_process_folio_batch() is designed to be abort-free: that is, once
> > it commits a folio for writeback, it will not later abandon it or
> > propagate an error for that folio.
>
> I think you need to explain your point more clear. Currently, I am not convinced
> that this modification makes sense.

ACK; a good commit message needs to be clear to everyone. I'm not sure
where I went wrong in my wording, but I'll try to restate my thought
process; so maybe you can tell me where I lose you:
1) At this point in the series (after patch 1 is applied), there is no
remaining possible way for ceph_process_folio_batch() to return a
nonzero value. All possible codepaths result in rc=0.
2) Therefore, the `if` statement that checks the
ceph_process_folio_batch() return code is dead code.
3) Trying to `goto release_folios` when the page array is active
constitutes a bug. So the `if (!ceph_wbc.locked_pages) goto
release_folios;` condition is correct, but the `if (rc) goto
release_folios;` is incorrect. (It's dead code anyway, see #2 above.)
4) Therefore, delete the `if (rc) goto release_folios;` dead code and
rely solely on `if (!ceph_wbc.locked_pages) goto release_folios;`
5) Since we aren't using the return code of ceph_process_folio_batch()
-- a static function with no other callers -- it should not return a
status in the first place.
6) By design ceph_process_folio_batch() is a "best-effort" function:
it tries to lock as many pages as it *can* (and that might be 0!) and
returns once it can't proceed. It is *not* allowed to abort: that is,
it cannot commit some pages for writeback, then change its mind and
prevent writeback of the whole batch.
7) Removing the return code from ceph_process_folio_batch() does not
prevent ceph_writepages_start() from knowing if a failure happened on
the first folio. ceph_writepages_start() already checks whether
ceph_wbc.locked_pages == 0.
8) Removing the return code from ceph_process_folio_batch() *does*
prevent ceph_writepages_start() from knowing *what* went wrong when
the first folio failed, but ceph_writepages_start() wasn't doing
anything with that information anyway. It only cared about the error
status as a boolean.
9) Most importantly: This patch does NOT constitute a behavioral
change. It is removing unreachable (and, when reached, buggy)
codepaths.

Warm regards,
Sam



>
> Thanks,
> Slava.
>
> >
> > Signed-off-by: Sam Edwards <CFSworks@...il.com>
> > ---
> >  fs/ceph/addr.c | 17 +++++------------
> >  1 file changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index 3462df35d245..2b722916fb9b 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -1283,16 +1283,16 @@ static inline int move_dirty_folio_in_page_array(struct address_space *mapping,
> >  }
> >
> >  static
> > -int ceph_process_folio_batch(struct address_space *mapping,
> > -                          struct writeback_control *wbc,
> > -                          struct ceph_writeback_ctl *ceph_wbc)
> > +void ceph_process_folio_batch(struct address_space *mapping,
> > +                           struct writeback_control *wbc,
> > +                           struct ceph_writeback_ctl *ceph_wbc)
> >  {
> >       struct inode *inode = mapping->host;
> >       struct ceph_fs_client *fsc = ceph_inode_to_fs_client(inode);
> >       struct ceph_client *cl = fsc->client;
> >       struct folio *folio = NULL;
> >       unsigned i;
> > -     int rc = 0;
> > +     int rc;
> >
> >       for (i = 0; can_next_page_be_processed(ceph_wbc, i); i++) {
> >               folio = ceph_wbc->fbatch.folios[i];
> > @@ -1322,12 +1322,10 @@ int ceph_process_folio_batch(struct address_space *mapping,
> >               rc = ceph_check_page_before_write(mapping, wbc,
> >                                                 ceph_wbc, folio);
> >               if (rc == -ENODATA) {
> > -                     rc = 0;
> >                       folio_unlock(folio);
> >                       ceph_wbc->fbatch.folios[i] = NULL;
> >                       continue;
> >               } else if (rc == -E2BIG) {
> > -                     rc = 0;
> >                       folio_unlock(folio);
> >                       ceph_wbc->fbatch.folios[i] = NULL;
> >                       break;
> > @@ -1369,7 +1367,6 @@ int ceph_process_folio_batch(struct address_space *mapping,
> >               rc = move_dirty_folio_in_page_array(mapping, wbc, ceph_wbc,
> >                               folio);
> >               if (rc) {
> > -                     rc = 0;
> >                       folio_redirty_for_writepage(wbc, folio);
> >                       folio_unlock(folio);
> >                       break;
> > @@ -1380,8 +1377,6 @@ int ceph_process_folio_batch(struct address_space *mapping,
> >       }
> >
> >       ceph_wbc->processed_in_fbatch = i;
> > -
> > -     return rc;
> >  }
> >
> >  static inline
> > @@ -1685,10 +1680,8 @@ static int ceph_writepages_start(struct address_space *mapping,
> >                       break;
> >
> >  process_folio_batch:
> > -             rc = ceph_process_folio_batch(mapping, wbc, &ceph_wbc);
> > +             ceph_process_folio_batch(mapping, wbc, &ceph_wbc);
> >               ceph_shift_unused_folios_left(&ceph_wbc.fbatch);
> > -             if (rc)
> > -                     goto release_folios;
> >
> >               /* did we get anything? */
> >               if (!ceph_wbc.locked_pages)

Powered by blists - more mailing lists