Re: [PATCH 13/19] ocfs2: check for a metadata write error with buffer_write_io_error()

Agreed, and thanks for the tag.

ocfs2-devel and the ocfs2 maintainers are on Cc for v2, and the cover letter
calls this out by name so it does not get lost in a 21 patch series.

To be concrete about what they are being asked to look at.
ocfs2_write_block() has never removed a block from the cluster uptodate
cache when its write failed, and the comment it carries today says why: the
buffer is not marked locally up-to-date, so the stale cache entry can do no
harm.  That reasoning stops holding at patch 20.  ocfs2_read_blocks()
decides whether to go to disk on the cluster cache alone, so it skips the
read either way; today it then fails the !buffer_uptodate(bh) test and
returns -EIO, but once the buffer stays up to date that read succeeds and
hands back the in-memory copy - the right data, but data that never reached
the disk, and another node reading the same block will not see it.

I do not have a cluster set up to test this on at the moment, so I would
rather ask than assume.  If it turns out to be a problem for ocfs2, I am
happy to fix it in the next version.

Best,
Chao

On Tue, Aug 4, 2026 at 4:49 AM Jan Kara <[email protected]> wrote:
>
> On Sat 01-08-26 18:00:57, Chao Shi wrote:
> > ocfs2_write_block() and ocfs2_write_super_or_backup() detect a failed write
> > by looking at BH_Uptodate afterwards.  That relies on the write completion
> > handler clearing BH_Uptodate on error, which this series removes: a buffer
> > whose write failed still holds the data the filesystem asked to be written,
> > so declaring it not up to date is wrong and makes callers re-read it.
> >
> > Test BH_Write_EIO instead.  Note that ocfs2_write_block()'s test is the
> > positive one, so the sense has to be inverted rather than the flag simply
> > swapped.
> >
> > The comment in ocfs2_write_block()'s error arm needs updating for the same
> > reason.  It said the clustered uptodate information did not have to be
> > removed because the buffer was not marked locally uptodate; after this
> > series it is, so the reason no longer holds.  Not advertising the block to
> > the cluster is still the right thing to do - the data is in memory but not
> > on disk - so only the justification changes, not the behaviour.
> >
> > No behaviour change today - a failed write sets BH_Write_EIO and clears
> > BH_Uptodate together.  It stops being a no-op at the end of the series,
> > where the new test is the one that still works.
> >
> > Signed-off-by: Chao Shi <[email protected]>
>
> This looks ok to me so feel free to add:
>
> Reviewed-by: Jan Kara <[email protected]>
>
> but it would be good to get a feedback from ocfs2 maintainers whether the
> logic change in ocfs2_write_block() where we now keep the cluster cache
> block uptodate after write IO error doesn't break some expectations of
> ocfs2.
>
>                                                                 Honza
>
> > ---
> >  fs/ocfs2/buffer_head_io.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
> > index 7bfe377af2df..733ceda79ca1 100644
> > --- a/fs/ocfs2/buffer_head_io.c
> > +++ b/fs/ocfs2/buffer_head_io.c
> > @@ -66,12 +66,14 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh,
> >
> >       wait_on_buffer(bh);
> >
> > -     if (buffer_uptodate(bh)) {
> > +     if (!buffer_write_io_error(bh)) {
> >               ocfs2_set_buffer_uptodate(ci, bh);
> >       } else {
> > -             /* We don't need to remove the clustered uptodate
> > -              * information for this bh as it's not marked locally
> > -              * uptodate. */
> > +             /*
> > +              * The buffer still holds what we tried to write, but it did
> > +              * not reach the disk, so don't advertise it to the cluster
> > +              * as up to date.
> > +              */
> >               ret = -EIO;
> >               mlog_errno(ret);
> >       }
> > @@ -446,7 +448,7 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
> >
> >       wait_on_buffer(bh);
> >
> > -     if (!buffer_uptodate(bh)) {
> > +     if (buffer_write_io_error(bh)) {
> >               ret = -EIO;
> >               mlog_errno(ret);
> >       }
> > --
> > 2.43.0
> >
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR