Message-ID: <CAH5Ym4jaZO5NWYZCzf9NpmhVm58MB0+aD4-4NR+iiozHRXDmhQ@mail.gmail.com>
Date: Tue, 6 Jan 2026 16:15:13 -0800
From: Sam Edwards <cfsworks@...il.com>
To: Viacheslav Dubeyko <Slava.Dubeyko@....com>
Cc: Milind Changire <mchangir@...hat.com>, Xiubo Li <xiubli@...hat.com>, 
	"idryomov@...il.com" <idryomov@...il.com>, "brauner@...nel.org" <brauner@...nel.org>, 
	"ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.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 Tue, Jan 6, 2026 at 2:47 PM Viacheslav Dubeyko <Slava.Dubeyko@....com> wrote:
>
> On Mon, 2026-01-05 at 22:52 -0800, Sam Edwards wrote:
> > 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 haven't made your statement more clear. :)
>
> As far as I can see, this statement has no connection to the patch 2. It is more
> relevant to the patch 3.
>
> >
> > 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.
>
> I am still not convinced that patch 1 is correct.

Then we should resolve patch 1 first before continuing with this
discussion. This patch is predicated on the correctness of patch 1, so
until that premise is agreed upon, any review of this patch is
necessarily blocked on that outcome.

If you have specific technical objections to patch 1, let’s address
those directly in that thread. Once we reach a consensus there, we can
continue the discussion of this patch on solid ground.


> So, we should expect to
> receive error code from move_dirty_folio_in_page_array(), especially for the
> case if no one folio has been processed. And if no one folio has been processed,
> then we need to return error code.
>
> The ceph_process_folio_batch() is complex function and we need to have the way
> to return the error code from internal function's logic to the caller's logic.
> We cannot afford not to have the return error code from this function. Because
> we need to be ready to release unprocessed folios if something was wrong in the
> function's logic.
>
> Thanks,
> Slava.
>
> > 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