Re: [PATCH v2] fs/buffer: serialize set_buffer_uptodate against concurrent clears

[Chris S]

Hi Jan, I'm currently working on the changes we discussed.  Based on
vfs.all now.  Four things came out of writing it that I would rather
resolve before posting.

1. mark_buffer_write_io_error() is not safe for jbd2's buffers yet.  It
dereferences bh->b_folio->mapping directly (fs/buffer.c:1057), and
jbd2_journal_write_metadata_buffer() points the temporary bh at
virt_to_folio(jh->b_frozen_data), which is slab-backed.
buffer_set_crypto_ctx() right next to it already uses folio_mapping() for
exactly this reason.  So I would send a prerequisite converting
mark_buffer_write_io_error() to folio_mapping() before the jbd2 change.
Also worth noting the consequence of A2: in ordered mode the temp bh
inherits the source folio, so the error lands on the bdev mapping and
mapping_set_error() errseq_set()s s_wb_err - a later syncfs() on an
unrelated fd of the same fs can then return EIO.

2. gfs2 already has what we are building: gfs2_end_log_write_bh()
(fs/gfs2/lops.c:171) calls mark_buffer_write_io_error() and does not clear
uptodate.  Which means fs/gfs2/log.c:110 and :324 are blind to log write
errors today, and converting them is not behaviour-preserving - it makes
them start catching those.  I think that is right, but say if you would
rather it were separate.

3. On the sticky BH_Write_EIO: I am adding it to BUFFER_FLAGS_DISCARD as
you suggested.  That covers discard_buffer(), but there are four other
clear_buffer_req() sites (fs/buffer.c:1687, fs/gfs2/aops.c:600,
fs/jbd2/commit.c:1035, fs/jbd2/transaction.c:2462).  Three of them also
clear_buffer_mapped() and NULL b_bdev, so __bh_submit()'s
BUG_ON(!buffer_mapped(bh)) makes them unreachable.  Only
clean_bdev_aliases() leaves the buffer writable, and I could not construct
a workload that reaches it.  So I am not sending the ungating patch.  The
one place it would matter is ocfs2: fs/ocfs2/journal.c:691 is nested
inside an if (!buffer_uptodate(bh)) at :675 that goes dead once the clear
is gone, so :691 has to be hoisted out, and it then becomes a pre-use
check on a sticky flag rather than a post-write one. I will Cc ocfs2-devel
on that patch.

4. fs/jbd2/transaction.c:923, J_EXPECT_JH(jh, buffer_uptodate(bh)) in
jbd2_freeze_jh_data(): after the series a failed write leaves the buffer
uptodate, so this stops firing for write errors.  Arguably correct - the
in-memory copy being frozen is still valid - but it is your assert.  Leave
it, or convert it?  I instrumented it and ran ext4 with data=journal under
injected write errors, forcing copy-out; it never saw a non-uptodate
buffer, so I have no evidence either way.

One thing I did settle by testing.  fs/ext4/ext4_jbd2.c:416 open-codes
buffer_req(bh) && !buffer_uptodate(bh) after sync_dirty_buffer(), which
already returns -EIO.  On an instrumented kernel (ext4 without a journal,
-o sync, injected write errors) the two agreed on all 40 occurrences and
never diverged, with BH_Write_EIO set every time.  So that site just
consumes the return value and the buffer_req() question goes away.

Best,
Chao

On Tue, Jul 28, 2026 at 3:45 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Sun 19-07-26 02:28:54, Chao S wrote:
> > On Thu 30-04-26 09:28:13, Jan Kara wrote:
> > > But it would be actually very welcome if you took the work, went through
> > > all the places using end_buffer_write_sync() and converted them in
> > > filesystem-by-filesystem to check for IO error by checking
> > > buffer_write_io_error() instead of buffer_uptodate() and then we can drop
> > > clear_buffer_uptodate() call from end_buffer_write_sync().
> >
> > Sorry for the long delay, I've been working on some patches and a paper
> > deadline.  Also, you were right about v2, and I have dropped it.  I
> > would like to
> > take the conversion work.
> >
> > Before writing it, I found your "fs: Fix missed inode write during fsync"
> > series, v4 of 16 July.  It removes four of the sites I had on my list, so
> > I will base this on top of it once it lands.  Say if you would rather
> > sequence it differently.
>
> Yes, please base your changes on top of vfs tree
> (https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all). My
> patches are already in there now.
>
> > (end_buffer_write_sync() is now bh_end_write(), per b20f15420f78.  Current
> > names below; line numbers against v7.2-rc1.)
> >
> > The sites, one patch each:
> >
> >   core     fs/buffer.c:621 (mmb_sync), :2803 (__sync_dirty_buffer)
> >   adfs     fs/adfs/dir.c:194
> >   ext2     fs/ext2/xattr.c:772
> >   ext4     fs/ext4/ext4_jbd2.c:399, fs/ext4/mmp.c:52
> >   omfs     fs/omfs/inode.c:148, :162
> >   exfat    fs/exfat/misc.c:190
> >   fat      fs/fat/misc.c:358
> >   ocfs2    fs/ocfs2/buffer_head_io.c:69, :449, fs/ocfs2/journal.c:675, :691
> >   gfs2     fs/gfs2/log.c:110, :324
> >   jbd2     fs/jbd2/commit.c:880
> >   removal  fs/buffer.c:210 and :444
> >
> > Conversions first, removal last.  The list is short because
> > __sync_dirty_buffer() and mmb_sync() test internally and return -EIO, so
> > the ~40 callers that use the return value need no edit of their own.
>
> Sounds good.
>
> > I would like your suggestions on the following.
> >
> > 1. BH_Write_EIO is sticky.  It is cleared only on rewrite
> > (fs/buffer.c:1196), and only if BH_Req was already set, which
> > BUFFER_FLAGS_DISCARD clears on its own.  So a converted test can see a
> > stale error.  I would fix this first, either by making the clear at :1196
> > unconditional for REQ_OP_WRITE, or by adding BH_Write_EIO to
> > BUFFER_FLAGS_DISCARD.  Which do you prefer?
>
> I think we should add it to BUFFER_FLAGS_DISCARD. It doesn't make sense to
> keep it on a discarded buffer.
>
> > 2. jbd2: log_bufs is a mixed list.  Commit descriptors come from
> > journal_end_buffer_io_sync(), which never sets BH_Write_EIO; revoke
> > descriptors from write_dirty_buffer(), so bh_end_write().  I would convert
> > fs/jbd2/commit.c:880 to the disjunct !buffer_uptodate(bh) ||
> > buffer_write_io_error(bh).  The alternative is to add
> > mark_buffer_write_io_error() to journal_end_buffer_io_sync() and convert
> > all three jbd2 sites.  Which do you prefer?
>
> Please convert journal_end_buffer_io_sync() to use
> mark_buffer_write_io_error() so that we completely get rid of this
> antipattern.
>
> > 3. bh_end_async_write() has the same clear at fs/buffer.c:444.  I would
> > remove both, which needs the gfs2 patch first since gfs2 reads that bit at
> > fs/gfs2/log.c:110 and :324.  I can also leave the async half out.
>
> Please change gfs2 the same way as you are fixing jbd2. Keeping the mix of
> uptodate & write_io_error checks only makes things more fragile...
>
> > 4. ocfs2: fs/ocfs2/journal.c:691 sits inside an outer
> > if (!buffer_uptodate(bh)) at :675, so dropping the clear stops ocfs2 going
> > read-only after a failed metadata checkpoint.  I would convert both sites
> > and Cc ocfs2-devel.
>
> Yes, please.
>
>                                                                 Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR