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

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

[Chao Shi]

A WARN_ON_ONCE(!buffer_uptodate(bh)) in mark_buffer_dirty() is
reachable from the buffered write path on a block device when the
underlying device returns I/O errors at high density.  Reproduced
by fuzzing an NVMe controller (FEMU) that returns crafted error
completions for a sustained workload from /dev/nvme0n1.

The contract documented at set_buffer_uptodate() in
include/linux/buffer_head.h reads:

  Any other serialization (with IO errors or whatever that might
  clear the bit) has to come from other state (eg BH_Lock).

In fs/buffer.c, BH_Uptodate can be cleared from four I/O completion
callbacks: __end_buffer_read_notouch, end_buffer_write_sync,
end_buffer_async_read, end_buffer_async_write.

end_buffer_async_read() runs with BH_Lock held throughout, so its
clear is already serialized against any caller that also holds
BH_Lock around set_buffer_uptodate(); the call may in fact be
redundant, but addressing that is independent of this fix.

end_buffer_write_sync() likewise holds BH_Lock while it clears
BH_Uptodate on the write-error path.  Removing that clear would
change long-standing buffer-cache I/O-error semantics and is out
of scope here.

The race is therefore between block_commit_write() and
end_buffer_write_sync():

  CPU A: block_commit_write              CPU B: end_buffer_write_sync
         (folio lock held, BH_Lock NOT)         (BH_Lock held)
    set_buffer_uptodate(bh);
                                           clear_buffer_uptodate(bh);
                                           unlock_buffer(bh);
    mark_buffer_dirty(bh);  /* WARN */

CPU B observes the contract; CPU A does not.  With one side unlocked
the serialization is one-sided and ineffective: CPU A's set can be
immediately followed by CPU B's clear, tripping the WARN_ON_ONCE.
In the fuzzing reproducer, write-error completions are frequent
(visible as repeated "lost async page write" and per-LBA write
failures); buffer I/O completion callbacks on the write-error path
(e.g. end_buffer_write_sync, end_buffer_async_write) clear
BH_Uptodate while holding BH_Lock.

The bug is not benign: a not-uptodate buffer can be marked dirty
and subsequently written back; depending on whether the buffer was
fully or partially covered by the user write, this can leave on-disk
content that does not match the intended buffered write state.

Fix this by taking BH_Lock around set_buffer_uptodate() +
mark_buffer_dirty() in block_commit_write(), so both sides of the
contract use the documented serialization.

Found by FuzzNvme (Syzkaller with FEMU fuzzing framework).

Acked-by: Sungwoo Kim <iam@sung-woo.kim>
Acked-by: Dave Tian <daveti@purdue.edu>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
Hi Matthew, Hi Jan,

Thanks for the review on v1.  v2 takes the feedback, quick notes below.

To Matthew:

You were right that the v1's timing diagram named the wrong racer.

The actual race is with end_buffer_write_sync() on the write-error
path, as Jan pointed out -- it also clears BH_Uptodate under BH_Lock,
but block_commit_write()'s else branch was reaching set_buffer_uptodate
without BH_Lock, leaving the serialization one-sided.  v2's commit
message and in-code comment now name end_buffer_write_sync() as the
racer.

To Jan:

Thanks for confirming the racer and for the historical context on the
dirty + !uptodate state question.  v2 keeps the fix scoped to taking
BH_Lock in block_commit_write(); the broader semantic question and any
change to end_buffer_write_sync()'s clear is left out of scope here.

The redundant lock_buffer/unlock_buffer that v1 added to the
buffer_new branch of __block_write_begin_int() is also dropped in v2 --
that bh has no in-flight async I/O so no race exists there.

v1 thread for context:
https://lore.kernel.org/all/20260426020137.1221985-1-coshi036@gmail.com/

Thanks,
Chao

Changes in v2:
  - Drop the lock_buffer/unlock_buffer added in v1 to the buffer_new
    branch of __block_write_begin_int(): that bh is freshly BH_New
    and has no in-flight async I/O on it, so no race exists at that
    site.
  - Rewrite the commit message and the in-code comment to identify
    end_buffer_write_sync() as the actual racer, not
    end_buffer_async_read() as v1 claimed; end_buffer_async_read()
    holds BH_Lock across its clear so a caller that also holds
    BH_Lock would already be serialized.
  - Reference the BH_Lock contract at set_buffer_uptodate() in
    include/linux/buffer_head.h explicitly.
  - Drop verbose line-number citations and the WARN stack dump from
    the commit message; tighten wording around reproducer evidence
    and on-disk impact.

v1: https://lore.kernel.org/all/20260426020137.1221985-1-coshi036@gmail.com/

 fs/buffer.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/buffer.c b/fs/buffer.c
index 4d7f84e77d2..6fddb2f1e7c 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2104,8 +2104,22 @@ void block_commit_write(struct folio *folio, size_t from, size_t to)
 			if (!buffer_uptodate(bh))
 				partial = true;
 		} else {
+			/*
+			 * Per the contract documented at set_buffer_uptodate()
+			 * in include/linux/buffer_head.h, callers must hold
+			 * BH_Lock to serialize against concurrent clears of
+			 * BH_Uptodate.  Holding only the folio lock is not
+			 * sufficient: a concurrent end_buffer_write_sync() on
+			 * the write-error path clears BH_Uptodate while
+			 * holding BH_Lock; without BH_Lock here the clear can
+			 * land between set_buffer_uptodate() and
+			 * mark_buffer_dirty(), tripping the WARN_ON_ONCE in
+			 * mark_buffer_dirty().
+			 */
+			lock_buffer(bh);
 			set_buffer_uptodate(bh);
 			mark_buffer_dirty(bh);
+			unlock_buffer(bh);
 		}
 		if (buffer_new(bh))
 			clear_buffer_new(bh);

base-commit: ffe69af1f87fa77da975ad4b0b093d48c3cbe6c3
-- 
2.43.0

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

[Jan Kara]

On Thu 30-04-26 01:36:45, Chao Shi wrote:
> A WARN_ON_ONCE(!buffer_uptodate(bh)) in mark_buffer_dirty() is
> reachable from the buffered write path on a block device when the
> underlying device returns I/O errors at high density.  Reproduced
> by fuzzing an NVMe controller (FEMU) that returns crafted error
> completions for a sustained workload from /dev/nvme0n1.
> 
> The contract documented at set_buffer_uptodate() in
> include/linux/buffer_head.h reads:
> 
>   Any other serialization (with IO errors or whatever that might
>   clear the bit) has to come from other state (eg BH_Lock).
> 
> In fs/buffer.c, BH_Uptodate can be cleared from four I/O completion
> callbacks: __end_buffer_read_notouch, end_buffer_write_sync,
> end_buffer_async_read, end_buffer_async_write.
> 
> end_buffer_async_read() runs with BH_Lock held throughout, so its
> clear is already serialized against any caller that also holds
> BH_Lock around set_buffer_uptodate(); the call may in fact be
> redundant, but addressing that is independent of this fix.
> 
> end_buffer_write_sync() likewise holds BH_Lock while it clears
> BH_Uptodate on the write-error path.  Removing that clear would
> change long-standing buffer-cache I/O-error semantics and is out
> of scope here.
> 
> The race is therefore between block_commit_write() and
> end_buffer_write_sync():
> 
>   CPU A: block_commit_write              CPU B: end_buffer_write_sync
>          (folio lock held, BH_Lock NOT)         (BH_Lock held)
>     set_buffer_uptodate(bh);
>                                            clear_buffer_uptodate(bh);
>                                            unlock_buffer(bh);
>     mark_buffer_dirty(bh);  /* WARN */
> 
> CPU B observes the contract; CPU A does not.  With one side unlocked
> the serialization is one-sided and ineffective: CPU A's set can be
> immediately followed by CPU B's clear, tripping the WARN_ON_ONCE.
> In the fuzzing reproducer, write-error completions are frequent
> (visible as repeated "lost async page write" and per-LBA write
> failures); buffer I/O completion callbacks on the write-error path
> (e.g. end_buffer_write_sync, end_buffer_async_write) clear
> BH_Uptodate while holding BH_Lock.
> 
> The bug is not benign: a not-uptodate buffer can be marked dirty
> and subsequently written back; depending on whether the buffer was
> fully or partially covered by the user write, this can leave on-disk
> content that does not match the intended buffered write state.
> 
> Fix this by taking BH_Lock around set_buffer_uptodate() +
> mark_buffer_dirty() in block_commit_write(), so both sides of the
> contract use the documented serialization.
> 
> Found by FuzzNvme (Syzkaller with FEMU fuzzing framework).
> 
> Acked-by: Sungwoo Kim <iam@sung-woo.kim>
> Acked-by: Dave Tian <daveti@purdue.edu>
> Acked-by: Weidong Zhu <weizhu@fiu.edu>
> Signed-off-by: Chao Shi <coshi036@gmail.com>

Thanks for trying but this basically just silences the warning without
addressing the real problem. Sure enough it silences the warning in
mark_buffer_dirty() but effectively it just papers over the real problem -
the IO completion with error can come the moment you unlock the bh and it
will happily clear the uptodate bit, resulting in the same "dirty but not
uptodate" invalid buffer state. So I actually prefer keeping things as they
currently are so that we are reminded there is this unresolved issue.

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(). Yes, it is much
more work but also much more useful.

								Honza

> ---
> Hi Matthew, Hi Jan,
> 
> Thanks for the review on v1.  v2 takes the feedback, quick notes below.
> 
> To Matthew:
> 
> You were right that the v1's timing diagram named the wrong racer.
> 
> The actual race is with end_buffer_write_sync() on the write-error
> path, as Jan pointed out -- it also clears BH_Uptodate under BH_Lock,
> but block_commit_write()'s else branch was reaching set_buffer_uptodate
> without BH_Lock, leaving the serialization one-sided.  v2's commit
> message and in-code comment now name end_buffer_write_sync() as the
> racer.
> 
> To Jan:
> 
> Thanks for confirming the racer and for the historical context on the
> dirty + !uptodate state question.  v2 keeps the fix scoped to taking
> BH_Lock in block_commit_write(); the broader semantic question and any
> change to end_buffer_write_sync()'s clear is left out of scope here.
> 
> The redundant lock_buffer/unlock_buffer that v1 added to the
> buffer_new branch of __block_write_begin_int() is also dropped in v2 --
> that bh has no in-flight async I/O so no race exists there.
> 
> v1 thread for context:
> https://lore.kernel.org/all/20260426020137.1221985-1-coshi036@gmail.com/
> 
> Thanks,
> Chao
> 
> Changes in v2:
>   - Drop the lock_buffer/unlock_buffer added in v1 to the buffer_new
>     branch of __block_write_begin_int(): that bh is freshly BH_New
>     and has no in-flight async I/O on it, so no race exists at that
>     site.
>   - Rewrite the commit message and the in-code comment to identify
>     end_buffer_write_sync() as the actual racer, not
>     end_buffer_async_read() as v1 claimed; end_buffer_async_read()
>     holds BH_Lock across its clear so a caller that also holds
>     BH_Lock would already be serialized.
>   - Reference the BH_Lock contract at set_buffer_uptodate() in
>     include/linux/buffer_head.h explicitly.
>   - Drop verbose line-number citations and the WARN stack dump from
>     the commit message; tighten wording around reproducer evidence
>     and on-disk impact.
> 
> v1: https://lore.kernel.org/all/20260426020137.1221985-1-coshi036@gmail.com/
> 
>  fs/buffer.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 4d7f84e77d2..6fddb2f1e7c 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2104,8 +2104,22 @@ void block_commit_write(struct folio *folio, size_t from, size_t to)
>  			if (!buffer_uptodate(bh))
>  				partial = true;
>  		} else {
> +			/*
> +			 * Per the contract documented at set_buffer_uptodate()
> +			 * in include/linux/buffer_head.h, callers must hold
> +			 * BH_Lock to serialize against concurrent clears of
> +			 * BH_Uptodate.  Holding only the folio lock is not
> +			 * sufficient: a concurrent end_buffer_write_sync() on
> +			 * the write-error path clears BH_Uptodate while
> +			 * holding BH_Lock; without BH_Lock here the clear can
> +			 * land between set_buffer_uptodate() and
> +			 * mark_buffer_dirty(), tripping the WARN_ON_ONCE in
> +			 * mark_buffer_dirty().
> +			 */
> +			lock_buffer(bh);
>  			set_buffer_uptodate(bh);
>  			mark_buffer_dirty(bh);
> +			unlock_buffer(bh);
>  		}
>  		if (buffer_new(bh))
>  			clear_buffer_new(bh);
> 
> base-commit: ffe69af1f87fa77da975ad4b0b093d48c3cbe6c3
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Chao S]

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.

(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.

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?

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?

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.

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.

Build-tested only so far.

Chao

On Thu, Apr 30, 2026 at 3:28 AM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 30-04-26 01:36:45, Chao Shi wrote:
> > A WARN_ON_ONCE(!buffer_uptodate(bh)) in mark_buffer_dirty() is
> > reachable from the buffered write path on a block device when the
> > underlying device returns I/O errors at high density.  Reproduced
> > by fuzzing an NVMe controller (FEMU) that returns crafted error
> > completions for a sustained workload from /dev/nvme0n1.
> >
> > The contract documented at set_buffer_uptodate() in
> > include/linux/buffer_head.h reads:
> >
> >   Any other serialization (with IO errors or whatever that might
> >   clear the bit) has to come from other state (eg BH_Lock).
> >
> > In fs/buffer.c, BH_Uptodate can be cleared from four I/O completion
> > callbacks: __end_buffer_read_notouch, end_buffer_write_sync,
> > end_buffer_async_read, end_buffer_async_write.
> >
> > end_buffer_async_read() runs with BH_Lock held throughout, so its
> > clear is already serialized against any caller that also holds
> > BH_Lock around set_buffer_uptodate(); the call may in fact be
> > redundant, but addressing that is independent of this fix.
> >
> > end_buffer_write_sync() likewise holds BH_Lock while it clears
> > BH_Uptodate on the write-error path.  Removing that clear would
> > change long-standing buffer-cache I/O-error semantics and is out
> > of scope here.
> >
> > The race is therefore between block_commit_write() and
> > end_buffer_write_sync():
> >
> >   CPU A: block_commit_write              CPU B: end_buffer_write_sync
> >          (folio lock held, BH_Lock NOT)         (BH_Lock held)
> >     set_buffer_uptodate(bh);
> >                                            clear_buffer_uptodate(bh);
> >                                            unlock_buffer(bh);
> >     mark_buffer_dirty(bh);  /* WARN */
> >
> > CPU B observes the contract; CPU A does not.  With one side unlocked
> > the serialization is one-sided and ineffective: CPU A's set can be
> > immediately followed by CPU B's clear, tripping the WARN_ON_ONCE.
> > In the fuzzing reproducer, write-error completions are frequent
> > (visible as repeated "lost async page write" and per-LBA write
> > failures); buffer I/O completion callbacks on the write-error path
> > (e.g. end_buffer_write_sync, end_buffer_async_write) clear
> > BH_Uptodate while holding BH_Lock.
> >
> > The bug is not benign: a not-uptodate buffer can be marked dirty
> > and subsequently written back; depending on whether the buffer was
> > fully or partially covered by the user write, this can leave on-disk
> > content that does not match the intended buffered write state.
> >
> > Fix this by taking BH_Lock around set_buffer_uptodate() +
> > mark_buffer_dirty() in block_commit_write(), so both sides of the
> > contract use the documented serialization.
> >
> > Found by FuzzNvme (Syzkaller with FEMU fuzzing framework).
> >
> > Acked-by: Sungwoo Kim <iam@sung-woo.kim>
> > Acked-by: Dave Tian <daveti@purdue.edu>
> > Acked-by: Weidong Zhu <weizhu@fiu.edu>
> > Signed-off-by: Chao Shi <coshi036@gmail.com>
>
> Thanks for trying but this basically just silences the warning without
> addressing the real problem. Sure enough it silences the warning in
> mark_buffer_dirty() but effectively it just papers over the real problem -
> the IO completion with error can come the moment you unlock the bh and it
> will happily clear the uptodate bit, resulting in the same "dirty but not
> uptodate" invalid buffer state. So I actually prefer keeping things as they
> currently are so that we are reminded there is this unresolved issue.
>
> 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(). Yes, it is much
> more work but also much more useful.
>
>                                                                 Honza
>
> > ---
> > Hi Matthew, Hi Jan,
> >
> > Thanks for the review on v1.  v2 takes the feedback, quick notes below.
> >
> > To Matthew:
> >
> > You were right that the v1's timing diagram named the wrong racer.
> >
> > The actual race is with end_buffer_write_sync() on the write-error
> > path, as Jan pointed out -- it also clears BH_Uptodate under BH_Lock,
> > but block_commit_write()'s else branch was reaching set_buffer_uptodate
> > without BH_Lock, leaving the serialization one-sided.  v2's commit
> > message and in-code comment now name end_buffer_write_sync() as the
> > racer.
> >
> > To Jan:
> >
> > Thanks for confirming the racer and for the historical context on the
> > dirty + !uptodate state question.  v2 keeps the fix scoped to taking
> > BH_Lock in block_commit_write(); the broader semantic question and any
> > change to end_buffer_write_sync()'s clear is left out of scope here.
> >
> > The redundant lock_buffer/unlock_buffer that v1 added to the
> > buffer_new branch of __block_write_begin_int() is also dropped in v2 --
> > that bh has no in-flight async I/O so no race exists there.
> >
> > v1 thread for context:
> > https://lore.kernel.org/all/20260426020137.1221985-1-coshi036@gmail.com/
> >
> > Thanks,
> > Chao
> >
> > Changes in v2:
> >   - Drop the lock_buffer/unlock_buffer added in v1 to the buffer_new
> >     branch of __block_write_begin_int(): that bh is freshly BH_New
> >     and has no in-flight async I/O on it, so no race exists at that
> >     site.
> >   - Rewrite the commit message and the in-code comment to identify
> >     end_buffer_write_sync() as the actual racer, not
> >     end_buffer_async_read() as v1 claimed; end_buffer_async_read()
> >     holds BH_Lock across its clear so a caller that also holds
> >     BH_Lock would already be serialized.
> >   - Reference the BH_Lock contract at set_buffer_uptodate() in
> >     include/linux/buffer_head.h explicitly.
> >   - Drop verbose line-number citations and the WARN stack dump from
> >     the commit message; tighten wording around reproducer evidence
> >     and on-disk impact.
> >
> > v1: https://lore.kernel.org/all/20260426020137.1221985-1-coshi036@gmail.com/
> >
> >  fs/buffer.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index 4d7f84e77d2..6fddb2f1e7c 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -2104,8 +2104,22 @@ void block_commit_write(struct folio *folio, size_t from, size_t to)
> >                       if (!buffer_uptodate(bh))
> >                               partial = true;
> >               } else {
> > +                     /*
> > +                      * Per the contract documented at set_buffer_uptodate()
> > +                      * in include/linux/buffer_head.h, callers must hold
> > +                      * BH_Lock to serialize against concurrent clears of
> > +                      * BH_Uptodate.  Holding only the folio lock is not
> > +                      * sufficient: a concurrent end_buffer_write_sync() on
> > +                      * the write-error path clears BH_Uptodate while
> > +                      * holding BH_Lock; without BH_Lock here the clear can
> > +                      * land between set_buffer_uptodate() and
> > +                      * mark_buffer_dirty(), tripping the WARN_ON_ONCE in
> > +                      * mark_buffer_dirty().
> > +                      */
> > +                     lock_buffer(bh);
> >                       set_buffer_uptodate(bh);
> >                       mark_buffer_dirty(bh);
> > +                     unlock_buffer(bh);
> >               }
> >               if (buffer_new(bh))
> >                       clear_buffer_new(bh);
> >
> > base-commit: ffe69af1f87fa77da975ad4b0b093d48c3cbe6c3
> > --
> > 2.43.0
> >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

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

[Jan Kara]

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@suse.com>
SUSE Labs, CR

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

[Chris S]

Thank you! I'll update asap.

Best,
Chao

On Tue, Jul 28, 2026 at 3:45 PM Jan Kara <jack@suse.cz> 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@suse.com>
> SUSE Labs, CR

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@suse.cz> 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@suse.com>
> SUSE Labs, CR

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

[Matthew Wilcox]

On Thu, Jul 30, 2026 at 03:47:23PM -0400, 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.

... yeah.  That's one of the unclean things which keeps me awake at
night.  Sorry you ran into it.

My longterm plan for this particular usecase (pointing a bh at slab
memory) is to have bh->b_folio = NULL, bh->b_data = (address of data).
Maybe we could do that now?  That would make mark_buffer_write_io_error()
work without change today.

There might be a good reason I didn't do that yet, but I forget what it
was.

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

[Chris S]

Yes, that works. And appreciate your reply.
  I prototyped it, and it survives a journal replay.

Six sites, with bio_add_virt_nofail() covering the submit side:

  bh_offset()                     b_page may be NULL
  buffer_set_crypto_ctx()         NULL guard
  __bh_submit()                   bio_add_virt_nofail() when !b_folio, and
                                  skip wbc_account_cgroup_owner()
  jbd2_journal_write_metadata_buffer()
                                  b_folio = NULL, b_data =
                                  jh->b_frozen_data, not folio_set_bh()
  jbd2_checksum_data(), jbd2_block_tag_csum_set()   both kmap the temp bh
                                  (fs/jbd2/commit.c:705 and :745)

4 files, +38/-12.  I'll send it as the first patch of the series.

Tested: ext4 with metadata_csum, mounted data=journal,journal_checksum,
writing files whose every block starts with the JBD2 magic so escaping
forces copy-out into b_frozen_data, plus repeated modification of the same
metadata while commits run.  Then sysrq-b without unmounting, reboot,
mount to replay. Journal recovery completed, md5sums of the escaped files
matched, e2fsck -fn clean, no warnings.  An instrumented build confirmed
the b_folio == NULL path was actually taken rather than silently skipped.

Best，
Chao

On Thu, Jul 30, 2026 at 4:20 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Jul 30, 2026 at 03:47:23PM -0400, 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.
>
> ... yeah.  That's one of the unclean things which keeps me awake at
> night.  Sorry you ran into it.
>
> My longterm plan for this particular usecase (pointing a bh at slab
> memory) is to have bh->b_folio = NULL, bh->b_data = (address of data).
> Maybe we could do that now?  That would make mark_buffer_write_io_error()
> work without change today.
>
> There might be a good reason I didn't do that yet, but I forget what it
> was.
>

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

[Matthew Wilcox]

On Thu, Jul 30, 2026 at 06:09:24PM -0400, Chris S wrote:
> Yes, that works. And appreciate your reply.
>   I prototyped it, and it survives a journal replay.
> 
> Six sites, with bio_add_virt_nofail() covering the submit side:
> 
>   bh_offset()                     b_page may be NULL
>   buffer_set_crypto_ctx()         NULL guard
>   __bh_submit()                   bio_add_virt_nofail() when !b_folio, and
>                                   skip wbc_account_cgroup_owner()
>   jbd2_journal_write_metadata_buffer()
>                                   b_folio = NULL, b_data =
>                                   jh->b_frozen_data, not folio_set_bh()
>   jbd2_checksum_data(), jbd2_block_tag_csum_set()   both kmap the temp bh
>                                   (fs/jbd2/commit.c:705 and :745)
> 
> 4 files, +38/-12.  I'll send it as the first patch of the series.

Fantastic.  Would you care to also include this patch as part of your
series?

From f7cd3419ce0518ba429ac854282888833bf60147 Mon Sep 17 00:00:00 2001
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Date: Thu, 30 Jul 2026 20:43:22 -0400
Subject: [PATCH] buffer_head: Remove b_page

All users except bh_offset() have been converted to use b_folio instead.
Convert bh_offset() and remove b_page.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/buffer_head.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 8b23bc9a244c..3881141a3ff1 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -59,10 +59,7 @@ struct address_space;
 struct buffer_head {
 	unsigned long b_state;		/* buffer state bitmap (see above) */
 	struct buffer_head *b_this_page;/* circular list of page's buffers */
-	union {
-		struct page *b_page;	/* the page this bh is mapped to */
-		struct folio *b_folio;	/* the folio this bh is mapped to */
-	};
+	struct folio *b_folio;		/* the folio this bh is mapped to */
 
 	sector_t b_blocknr;		/* start block number */
 	size_t b_size;			/* size of mapping */
@@ -172,7 +169,7 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
 
 static inline unsigned long bh_offset(const struct buffer_head *bh)
 {
-	return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
+	return (unsigned long)(bh)->b_data & (folio_size(bh->b_folio) - 1);
 }
 
 /* If we *know* page->private refers to buffer_heads */
-- 
2.47.3

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

[Chris S]

My pleasure. I did a small check.  bh_offset() is indeed the only
b_page user left, and
it applies as-is: my patch guards the four callers that can see a
folio-less bh (__bh_submit(), buffer_set_crypto_ctx(), and the two jbd2
checksum helpers), so bh_offset() itself never sees one and stays the
one-liner you made it.

Built and ran the replay test with both applied - ext4 data=journal,
blocks starting with the JBD2 magic to force copy-out, sysrq-b without
unmounting, then mount to replay.  Recovery completed, md5sums matched,
e2fsck clean, and an instrumented build confirmed the b_folio == NULL path
was taken.

Series head:

  1  buffer_head: Remove b_page                           (yours)
  2  jbd2: point the temp bh at the frozen data directly  (b_folio = NULL)
  3  ... the buffer_write_io_error() conversion Jan asked for

I'll note in 2's changelog that a folio-less bh must not be passed to
bh_offset(), so the constraint is written down rather than implied.

Chao

On Thu, Jul 30, 2026 at 8:45 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Jul 30, 2026 at 06:09:24PM -0400, Chris S wrote:
> > Yes, that works. And appreciate your reply.
> >   I prototyped it, and it survives a journal replay.
> >
> > Six sites, with bio_add_virt_nofail() covering the submit side:
> >
> >   bh_offset()                     b_page may be NULL
> >   buffer_set_crypto_ctx()         NULL guard
> >   __bh_submit()                   bio_add_virt_nofail() when !b_folio, and
> >                                   skip wbc_account_cgroup_owner()
> >   jbd2_journal_write_metadata_buffer()
> >                                   b_folio = NULL, b_data =
> >                                   jh->b_frozen_data, not folio_set_bh()
> >   jbd2_checksum_data(), jbd2_block_tag_csum_set()   both kmap the temp bh
> >                                   (fs/jbd2/commit.c:705 and :745)
> >
> > 4 files, +38/-12.  I'll send it as the first patch of the series.
>
> Fantastic.  Would you care to also include this patch as part of your
> series?
>
> From f7cd3419ce0518ba429ac854282888833bf60147 Mon Sep 17 00:00:00 2001
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> Date: Thu, 30 Jul 2026 20:43:22 -0400
> Subject: [PATCH] buffer_head: Remove b_page
>
> All users except bh_offset() have been converted to use b_folio instead.
> Convert bh_offset() and remove b_page.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  include/linux/buffer_head.h | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index 8b23bc9a244c..3881141a3ff1 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -59,10 +59,7 @@ struct address_space;
>  struct buffer_head {
>         unsigned long b_state;          /* buffer state bitmap (see above) */
>         struct buffer_head *b_this_page;/* circular list of page's buffers */
> -       union {
> -               struct page *b_page;    /* the page this bh is mapped to */
> -               struct folio *b_folio;  /* the folio this bh is mapped to */
> -       };
> +       struct folio *b_folio;          /* the folio this bh is mapped to */
>
>         sector_t b_blocknr;             /* start block number */
>         size_t b_size;                  /* size of mapping */
> @@ -172,7 +169,7 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
>
>  static inline unsigned long bh_offset(const struct buffer_head *bh)
>  {
> -       return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
> +       return (unsigned long)(bh)->b_data & (folio_size(bh->b_folio) - 1);
>  }
>
>  /* If we *know* page->private refers to buffer_heads */
> --
> 2.47.3
>

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@suse.cz> 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@suse.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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@suse.cz> 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@suse.cz> 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@suse.com>
> > > SUSE Labs, CR
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR