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

[Chris S]

Got you. Working on it. Will try my best to make it ASAP.
Thanks for your help and time!

Best,
Chao

On Fri, Jul 31, 2026 at 1:51 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Thu 30-07-26 15:47:23, Chris S wrote:
> > 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.
>
> This you've already solved with Matthew.
>
> > 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.
>
> Yes, that's a desirable change. I think it's minor enough that we can keep
> it in the series.
>
> > 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.
>
> That get's used e.g. when a block is first used for metadata, then it's
> freed before it is written, and then the block gets reallocated as data.
> Whatever is set on the bdev buffer head is irrelevant at that point. But
> clean_bdev_aliases() is practically dead (only called from legacy DIO code
> and ntfs). I'd just leave it alone. What would make sense though is to
> clear the BH_Write_EIO in bforget() which is generally used by filesystems
> on metadata blocks to be freed.
>
> > 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.
>
> Not sure what you exactly mean here but it needs to become something like:
>
>
>         if (!buffer_uptodate(bh)) {
>                 mlog(ML_ERROR, "giving me a buffer that's not uptodate!\n");
>                 mlog(ML_ERROR, "b_blocknr=%llu, b_state=0x%lx\n",
>                      (unsigned long long)bh->b_blocknr, bh->b_state);
>         }
>         /*
>          * A previous transaction with a couple of buffer heads fail
>          * to checkpoint, so all the bhs are marked as BH_Write_EIO.
>          * For current transaction, the bh is just among those error
>          * bhs which previous transaction handle. We can't just clear
>          * its BH_Write_EIO and reuse directly, since other bhs are
>          * not written to disk yet and that will cause metadata
>          * inconsistency. So we should set fs read-only to avoid
>          * further damage.
>          */
>         if (buffer_write_io_error(bh)) {
>                 lock_buffer(bh);
>                 if (buffer_write_io_error(bh)) {
>                         unlock_buffer(bh);
>                         return ocfs2_error(osb->sb, "A previous attempt to "
>                                         "write this buffer head failed\n");
>                 }
>                 unlock_buffer(bh);
>         }
>
>
> And yes, CCing lists & maintainers for the corresponding filesystems that
> are touched is needed in all the cases.
>
> > 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.
>
> I'd convert it to:
>
>         J_EXPECT_JH(jh, !buffer_write_io_error(bh), "IO failure.\n");
>
> > 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.
>
> The test
>
>         if (buffer_req(bh) && !buffer_uptodate(bh)) {
>
> is stronger than just checking the return value of sync_dirty_buffer()
> because it triggers also if the buffer was written by background writeback
> and hit IO error. Arguably sync_dirty_buffer() should return EIO in that
> case as well but that's another cleanup I don't want to entangle with this.
> So I'd rather change that test to buffer_write_io_error().
>
>                                                                 Honza
>
> > 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
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR