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

[Jan Kara]

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