[PATCH v2 00/21] buffer: stop clearing BH_Uptodate when a write fails

[PATCH v2 00/21] buffer: stop clearing BH_Uptodate when a write fails

[Chao Shi]

When a metadata write fails, the buffer_head completion handlers clear
BH_Uptodate.  The buffer still holds exactly the data the filesystem asked
to be written - it is the disk that is stale, not the buffer - and saying
otherwise has consequences:

  - mark_buffer_dirty() has a WARN_ON_ONCE(!buffer_uptodate(bh)), so a
    filesystem that dirties the buffer again to retry the write trips it.
    That is the warning that started this.

  - a buffer that is not up to date gets re-read from disk, which silently
    replaces the data the filesystem was trying to write with the stale
    on-disk copy.

  - the state is not self consistent while it lasts: the window between the
    write completing and BH_Uptodate being cleared is visible to anyone
    holding the folio lock.

BH_Write_EIO already records that the last write failed.  This series moves
every consumer over to it, then stops write completion touching BH_Uptodate
at all.

Patches 1-4 are groundwork.  Matthew's patch 1 removes b_page.  Patches 2
and 3 let a buffer_head point at memory outside the page cache and use that
for jbd2's shadow buffers, which today sit on a slab folio - a slab folio
overloads ->mapping, so mark_buffer_write_io_error() cannot be called on
them at all.  That is what blocked the jbd2 conversion.  Patch 4 then drops
the folio_mapping() call that was only there to cope with those slab
folios.

Patches 5-6 make BH_Write_EIO safe to leave set: clear it in bforget() and
discard it on invalidate, so a freed or reused block does not inherit
somebody else's write error.

Patches 7-19 convert the consumers, one filesystem at a time: the two core
helpers in fs/buffer.c, then adfs, ext2, omfs, exfat, fat, ext4, ocfs2,
gfs2 and jbd2.  Patch 20 stops write completion touching BH_Uptodate.
Patch 21 moves the point at which BH_Write_EIO is cleared from submission
to successful completion.

Every patch builds on its own and the tree behaves identically at each step
until patch 20, because a failed write currently sets BH_Write_EIO and
clears BH_Uptodate together.

Changes since v1:

  - 02: changelog reworked - a folio-less buffer is a narrow thing that
    most of the buffer_head API will not tolerate, and keeping it away from
    all of that is the caller's job; NULL just makes getting it wrong loud
    instead of quiet (Jan).

  - 03: jbd2_journal_write_metadata_buffer() simplified.  folio_set_bh() is
    now needed on one path only, so it moved there and new_folio,
    new_offset and the flag that chose between them are gone (Jan).  The two
    checksum helpers now use a new kmap_local_bh()/kunmap_local_bh() pair
    instead of open coding the folio test (Matthew).  The pair does not map
    a folio-less buffer at all: that memory is always mapped, and under
    CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP mapping it would hand back one page,
    which is not enough for a block bigger than a page.

  - 04 is new: read bh->b_folio->mapping directly instead of via
    folio_mapping(), which would hand fscrypt a swap_address_space if a
    buffer ever sat on a swap cache folio (Matthew).  It is a separate patch
    after 03 on purpose - folio_mapping() is also what turns a slab folio
    into NULL, so doing this before jbd2 stops using slab folios would leave
    a window where this path reads slab-internal state as an address_space.

  - 17, 19: the jbd2 and ext4 fast commit completion handlers stop setting
    BH_Uptodate as well, and their local flag is renamed to match
    fs/buffer.c.  ext4's debug messages now describe the write rather than
    the buffer's contents (Jan).

  - 18: the assertion in jbd2_freeze_jh_data() keeps testing BH_Uptodate,
    which is what it is really about; only its message changes.  v1
    converted it to BH_Write_EIO, which was wrong - a buffer with a failed
    write still has valid data here and will be written again (Jan).

  - 20: write completion no longer sets BH_Uptodate either.  Checked rather
    than assumed: an instrumented build with
    WARN_ONCE(success && !buffer_uptodate(bh)) in all four write completion
    handlers, exercised over ext4 in two configurations - data=journal with
    journal_checksum, and fast_commit with -o sync - never fired.  That is
    ext4 evidence only: vfat and exfat were in the same run, but neither
    reached a mount in that image.

  - 21 is new (Jan): clear BH_Write_EIO on successful write completion
    instead of on resubmission, and drop the clear from __bh_submit().  As
    well as being the more honest point to clear it, this closes the case
    Sahiko raised, where a task's write fails and another task's
    resubmission clears the flag before the first task looks at it.

  - Jan's Reviewed-by added to v1's 1, 2, 4, 5, 6, 8, 12, 13, 14, which are
    v2's 1, 2, 5, 6, 7, 9, 13, 14, 15.  The code in those is unchanged; some
    of their changelogs are reworded so that they no longer describe where
    BH_Write_EIO gets cleared, which patch 21 moves.

Three things are worth a second look, and I would rather point at them than
let you find them:

  - gfs2 (patch 16) is not a pure conversion.  gfs2_end_log_write_bh()
    already marks BH_Write_EIO without clearing BH_Uptodate, so the two ail
    checks are blind to log write errors today and start catching them.
    Jan and I agreed that is the desired fix rather than a regression, but
    it is a real behaviour change for gfs2.

  - ocfs2 (patch 14).  ocfs2_write_block() has never removed a block from
    the cluster uptodate cache when its write failed, because the buffer was
    not locally uptodate and so the stale entry could do no harm.  After
    patch 20 it is uptodate, and ocfs2_read_blocks() - which decides whether
    to go to disk on the cluster cache alone - stops returning -EIO for such
    a block and hands back the in-memory copy instead.  Jan asked for ocfs2
    maintainer eyes on that; the ask stands, and I cannot test a real
    cluster here.

  - after patch 20, BH_Write_EIO stays set until the buffer is written
    successfully, forgotten or invalidated.  So a site like ext4's itable
    sync (patch 13) now reports on every subsequent sync rather than only on
    the write that failed.  That is intended, and matches what ocfs2 has
    always done with this flag.  The read path changes too: __bread_gfp()
    and bh_uptodate_or_lock() stop re-reading a buffer whose write failed,
    and return the in-memory data instead.

Zhang Yi pointed out on v1 that ext4_buffer_uptodate() exists for exactly
this reason: it puts BH_Uptodate back on a buffer whose write failed, so
that ext4 does not go and re-read a block whose in-memory copy is the good
one.  That is this series' argument, open coded in one filesystem, and it
can go once the generic behaviour is fixed.  Jan noted there are more such
workarounds about.  I have left every one of them alone here, so that this
series stays a behaviour change rather than a cleanup.

On the original report: patch 20 provably closes that WARN for the write
error case, since the state it warns about can no longer be produced that
way.  I could not reproduce the WARN itself with fail_make_request, which
fails synchronously at submit; the original came from a fuzzer injecting
delayed error completions, which is what opens the window.  So there is no
ready reproducer to offer, only the argument.

Based on vfs.git vfs.all, because Jan's "fs: Fix missed inode write during
fsync" series is in it and rewrites __ext4_handle_dirty_metadata(), which
patch 13 touches.

Testing: each of the 21 patches built individually, warning free;
checkpatch --strict clean.  The jbd2 shadow buffer changes were validated by
crashing with sysrq-b during ext4 data=journal,journal_checksum traffic
engineered to force copy-out into b_frozen_data, then replaying the journal
on the next mount - recovery completed, contents matched, e2fsck -fn clean,
and an instrumented build confirmed the folio-less path was taken.  Write
error detection was compared against an unpatched build under injected write
errors: same error reports, same journal abort, same read-only remount, same
errno to userspace.  All of that is ext4, and so jbd2 underneath it; the
adfs, ext2, exfat, fat, gfs2, ocfs2 and omfs conversions are compile tested
only.

v1: https://lore.kernel.org/linux-fsdevel/cover.1785621505.git.coshi036@gmail.com/

The conversion was asked for here:
  https://lore.kernel.org/linux-fsdevel/xaqwfkwjnp7h2lg7ir6wy2tnyay26t3im3ftkdl64rhys3rhmu@lc5vfh5tc2f5/


Chao Shi (20):
  buffer: allow a buffer_head to point at memory outside the page cache
  jbd2: point the shadow buffer at the frozen data directly
  buffer: read the folio's mapping directly in buffer_set_crypto_ctx()
  buffer: clear BH_Write_EIO when a buffer is forgotten
  buffer: discard BH_Write_EIO along with the rest of the buffer state
  buffer: detect metadata write errors with buffer_write_io_error()
  adfs: check for a directory write error with buffer_write_io_error()
  ext2: check for an xattr block write error with
    buffer_write_io_error()
  omfs: check for an inode write error with buffer_write_io_error()
  exfat: check for a directory write error with buffer_write_io_error()
  fat: check for a metadata write error with buffer_write_io_error()
  ext4: check for a metadata write error with buffer_write_io_error()
  ocfs2: check for a metadata write error with buffer_write_io_error()
  ocfs2: check for a stale write error before reusing a metadata buffer
  gfs2: check for a metadata write error with buffer_write_io_error()
  jbd2: report journal write errors with BH_Write_EIO
  jbd2: say what jbd2_freeze_jh_data()'s assertion is actually checking
  ext4, jbd2: report fast commit write errors with BH_Write_EIO
  buffer: stop touching BH_Uptodate on write completion
  buffer: clear BH_Write_EIO when a write succeeds, not when one starts

Matthew Wilcox (Oracle) (1):
  buffer_head: Remove b_page

 fs/adfs/dir.c               |  2 +-
 fs/buffer.c                 | 37 ++++++++++++++++++++-----------------
 fs/exfat/misc.c             |  2 +-
 fs/ext2/xattr.c             |  2 +-
 fs/ext4/ext4_jbd2.c         |  2 +-
 fs/ext4/fast_commit.c       | 12 ++++++------
 fs/ext4/mmp.c               |  2 +-
 fs/fat/misc.c               |  2 +-
 fs/gfs2/log.c               |  4 ++--
 fs/gfs2/lops.c              |  4 +++-
 fs/jbd2/commit.c            | 22 +++++++++++-----------
 fs/jbd2/journal.c           | 31 ++++++++++++++++++-------------
 fs/jbd2/transaction.c       |  2 +-
 fs/ocfs2/buffer_head_io.c   | 12 +++++++-----
 fs/ocfs2/journal.c          | 25 +++++++++++++------------
 fs/omfs/inode.c             |  4 ++--
 include/linux/buffer_head.h | 36 +++++++++++++++++++++++++++++++-----
 17 files changed, 120 insertions(+), 81 deletions(-)


base-commit: 05c09c9c8a79d5539fef30d42e732eba90a15dcf
-- 
2.43.0

[PATCH v2 01/21] buffer_head: Remove b_page

[Chao Shi]

From: "Matthew Wilcox (Oracle)" <willy@infradead.org>

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>
Signed-off-by: Chao Shi <coshi036@gmail.com>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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 fd2c7115c054..699970b4bbf2 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.43.0

[PATCH v2 02/21] buffer: allow a buffer_head to point at memory outside the page cache

[Chao Shi]

jbd2 builds a temporary buffer_head to write out the frozen copy of a
metadata block, and that copy lives in slab memory.  Today jbd2 points the
temporary buffer at the slab folio backing it.  A slab folio's ->mapping is
not an address_space, so anything that follows bh->b_folio->mapping there
gets garbage rather than NULL; mark_buffer_write_io_error() does exactly
that, and we are about to start calling it on this buffer.

Rather than teach every such helper about slab folios, allow bh->b_folio to
be NULL and let b_data point straight at the memory.  Code that needs the
folio has to check.  There are two places in this file:

 - __bh_submit() adds the data by virtual address using
   bio_add_virt_nofail(), and skips the cgroup accounting: a buffer that is
   not in the page cache has no owning folio to attribute writeback to.

 - buffer_set_crypto_ctx() returns early.  fscrypt has no interest in a
   buffer that is not part of a file mapping, which is why it already
   returns when the folio has no mapping.

Nothing sets b_folio to NULL yet, so this patch is a no-op on its own.

A buffer_head without a folio is a narrow thing, not a new general
capability.  Most of the buffer_head API assumes a folio and will fault or
corrupt state without one - touch_buffer(), bh_offset(), the async read
completion path, and plenty more - so it is up to whoever builds such a
buffer to keep it away from all of that.  What NULL buys us is that getting
it wrong fails loudly instead of quietly following a slab folio's
overloaded ->mapping.  It is also only valid over memory that is always
mapped: buffers over highmem have no permanent kernel virtual address,
which is why folio_set_bh() records a folio and an offset instead.

Suggested-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index be8b57a635cd..04fcc34e4fa6 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1099,12 +1099,16 @@ EXPORT_SYMBOL(__bforget);
 static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,
 				  gfp_t gfp_mask)
 {
-	const struct address_space *mapping = folio_mapping(bh->b_folio);
+	const struct address_space *mapping;
 
 	/*
 	 * The ext4 journal (jbd2) can submit a buffer_head it directly created
-	 * for a non-pagecache page.  fscrypt doesn't care about these.
+	 * for memory that is not in the page cache at all.  fscrypt doesn't
+	 * care about these.
 	 */
+	if (!bh->b_folio)
+		return;
+	mapping = folio_mapping(bh->b_folio);
 	if (!mapping)
 		return;
 	fscrypt_set_bio_crypt_ctx(bio, mapping->host,
@@ -1142,7 +1146,11 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
 	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
 	bio->bi_write_hint = write_hint;
 
-	bio_add_folio_nofail(bio, bh->b_folio, bh->b_size, bh_offset(bh));
+	if (bh->b_folio)
+		bio_add_folio_nofail(bio, bh->b_folio, bh->b_size,
+				     bh_offset(bh));
+	else
+		bio_add_virt_nofail(bio, bh->b_data, bh->b_size);
 
 	bio->bi_end_io = end_bio;
 	bio->bi_private = bh;
@@ -1152,7 +1160,8 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
 
 	if (wbc) {
 		wbc_init_bio(wbc, bio);
-		wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
+		if (bh->b_folio)
+			wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
 	}
 
 	blk_crypto_submit_bio(bio);
-- 
2.43.0

[PATCH v2 03/21] jbd2: point the shadow buffer at the frozen data directly

[Chao Shi]

When a metadata buffer has to be copied out before it can be journalled,
jbd2_journal_write_metadata_buffer() writes jh->b_frozen_data rather than
the page cache copy.  b_frozen_data is kmalloc()ed, so folio_set_bh() makes
the shadow buffer point at a slab folio.

That is not something the buffer_head layer can reason about.  A slab folio
overloads ->mapping, so a shadow buffer looks like it belongs to an
address_space when it does not.  buffer_set_crypto_ctx() already has to
work around this, and it is the reason mark_buffer_write_io_error() cannot
be called on a shadow buffer today.

Point the shadow buffer at the frozen data itself instead: leave b_folio
NULL, which it already is out of alloc_buffer_head(), and set b_data.  The
previous patch taught fs/buffer.c to submit such a buffer.  folio_set_bh()
is now needed on only one path - the one that journals the page cache copy
directly - so it moves there, and new_folio, new_offset and the flag that
used to pick between them all go away.

The two commit-path checksum helpers reach the shadow buffer's contents
through a new kmap_local_bh()/kunmap_local_bh() pair, which handle a buffer
with or without a folio.  Memory outside the page cache is always mapped,
so for those there is nothing to map or unmap.  Mapping it anyway would be
worse than pointless: with CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP,
kmap_local_page() hands back a one page mapping even for such memory, which
is not enough for a buffer bigger than a page.

Tested with ext4 mounted data=journal,journal_checksum on a metadata_csum
filesystem, writing files whose every block begins with the JBD2 magic so
that escaping forces the copy-out, then crashing with sysrq-b without
unmounting and replaying the journal on the next mount.  Recovery
completed, the file contents matched, e2fsck -fn was clean, and an
instrumented build confirmed the b_folio == NULL path was taken.

Suggested-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/jbd2/commit.c            |  8 ++++----
 fs/jbd2/journal.c           | 29 +++++++++++++++++------------
 include/linux/buffer_head.h | 29 +++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 3029cb6f6d64..0c85af91f9b2 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -330,9 +330,9 @@ static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
 	char *addr;
 	__u32 checksum;
 
-	addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
+	addr = kmap_local_bh(bh);
 	checksum = crc32_be(crc32_sum, addr, bh->b_size);
-	kunmap_local(addr);
+	kunmap_local_bh(bh, addr);
 
 	return checksum;
 }
@@ -357,10 +357,10 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
 		return;
 
 	seq = cpu_to_be32(sequence);
-	addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
+	addr = kmap_local_bh(bh);
 	csum32 = jbd2_chksum(j->j_csum_seed, (__u8 *)&seq, sizeof(seq));
 	csum32 = jbd2_chksum(csum32, addr, bh->b_size);
-	kunmap_local(addr);
+	kunmap_local_bh(bh, addr);
 
 	if (jbd2_has_feature_csum3(j))
 		tag3->t_checksum = cpu_to_be32(csum32);
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 09efa337649e..6e05dc47e20a 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -327,8 +327,6 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 {
 	int do_escape = 0;
 	struct buffer_head *new_bh;
-	struct folio *new_folio;
-	unsigned int new_offset;
 	struct buffer_head *bh_in = jh2bh(jh_in);
 	journal_t *journal = transaction->t_journal;
 
@@ -348,24 +346,31 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	/* keep subsequent assertions sane */
 	atomic_set(&new_bh->b_count, 1);
 
+	/*
+	 * b_frozen_data is slab memory, not page cache, so when we use it the
+	 * shadow buffer gets no folio at all: b_folio stays NULL from the
+	 * allocation and b_data points straight at the copy.  Pointing it at
+	 * the slab folio instead would hand its overloaded ->mapping to
+	 * anything that goes looking for an address_space.
+	 */
+
 	spin_lock(&jh_in->b_state_lock);
 	/*
 	 * If a new transaction has already done a buffer copy-out, then
 	 * we use that version of the data for the commit.
 	 */
 	if (jh_in->b_frozen_data) {
-		new_folio = virt_to_folio(jh_in->b_frozen_data);
-		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
 		do_escape = jbd2_data_needs_escaping(jh_in->b_frozen_data);
 		if (do_escape)
 			jbd2_data_do_escape(jh_in->b_frozen_data);
+		new_bh->b_data = jh_in->b_frozen_data;
 	} else {
+		struct folio *folio = bh_in->b_folio;
+		unsigned int offset = offset_in_folio(folio, bh_in->b_data);
 		char *tmp;
 		char *mapped_data;
 
-		new_folio = bh_in->b_folio;
-		new_offset = offset_in_folio(new_folio, bh_in->b_data);
-		mapped_data = kmap_local_folio(new_folio, new_offset);
+		mapped_data = kmap_local_folio(folio, offset);
 		/*
 		 * Fire data frozen trigger if data already wasn't frozen. Do
 		 * this before checking for escaping, as the trigger may modify
@@ -379,8 +384,10 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 		/*
 		 * Do we need to do a data copy?
 		 */
-		if (!do_escape)
+		if (!do_escape) {
+			folio_set_bh(new_bh, folio, offset);
 			goto escape_done;
+		}
 
 		spin_unlock(&jh_in->b_state_lock);
 		tmp = kmalloc(bh_in->b_size, GFP_NOFS | __GFP_NOFAIL);
@@ -391,7 +398,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 		}
 
 		jh_in->b_frozen_data = tmp;
-		memcpy_from_folio(tmp, new_folio, new_offset, bh_in->b_size);
+		memcpy_from_folio(tmp, folio, offset, bh_in->b_size);
 		/*
 		 * This isn't strictly necessary, as we're using frozen
 		 * data for the escaping, but it keeps consistency with
@@ -400,13 +407,11 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 		jh_in->b_frozen_triggers = jh_in->b_triggers;
 
 copy_done:
-		new_folio = virt_to_folio(jh_in->b_frozen_data);
-		new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
 		jbd2_data_do_escape(jh_in->b_frozen_data);
+		new_bh->b_data = jh_in->b_frozen_data;
 	}
 
 escape_done:
-	folio_set_bh(new_bh, new_folio, new_offset);
 	new_bh->b_size = bh_in->b_size;
 	new_bh->b_bdev = journal->j_dev;
 	new_bh->b_blocknr = blocknr;
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 699970b4bbf2..20b8fca1abfa 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -172,6 +172,35 @@ static inline unsigned long bh_offset(const struct buffer_head *bh)
 	return (unsigned long)(bh)->b_data & (folio_size(bh->b_folio) - 1);
 }
 
+/**
+ * kmap_local_bh - Map the data of a buffer.
+ * @bh: The buffer.
+ *
+ * Buffers usually live in the page cache, but a few are built over memory
+ * which is not.  Those carry no folio and b_data is already a kernel address
+ * which is always mapped, so there is nothing to do for them.  Pair with
+ * kunmap_local_bh().
+ *
+ * Return: A pointer to the buffer's data.
+ */
+static inline void *kmap_local_bh(const struct buffer_head *bh)
+{
+	if (!bh->b_folio)
+		return bh->b_data;
+	return kmap_local_folio(bh->b_folio, bh_offset(bh));
+}
+
+/**
+ * kunmap_local_bh - Unmap the data of a buffer.
+ * @bh: The buffer.
+ * @addr: The address returned by kmap_local_bh().
+ */
+static inline void kunmap_local_bh(const struct buffer_head *bh, void *addr)
+{
+	if (bh->b_folio)
+		kunmap_local(addr);
+}
+
 /* If we *know* page->private refers to buffer_heads */
 #define page_buffers(page)					\
 	({							\
-- 
2.43.0

[PATCH v2 04/21] buffer: read the folio's mapping directly in buffer_set_crypto_ctx()

[Chao Shi]

folio_mapping() was doing two jobs here.  One was to turn a slab folio into
NULL, which is what made this safe for jbd2's shadow buffers; the previous
patch removed the need for that by giving those buffers no folio at all.

The other is a hazard.  folio_mapping() maps a folio in the swap cache to
its swap_address_space, so if a buffer_head were ever attached to such a
folio this would hand fscrypt a swap mapping and dereference ->host on it.
There is no reason to want that here: this path wants the file's mapping or
nothing.

Read ->mapping directly.  Buffers with no folio are already handled above.

Suggested-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 04fcc34e4fa6..2851830995d8 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1108,7 +1108,7 @@ static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,
 	 */
 	if (!bh->b_folio)
 		return;
-	mapping = folio_mapping(bh->b_folio);
+	mapping = bh->b_folio->mapping;
 	if (!mapping)
 		return;
 	fscrypt_set_bio_crypt_ctx(bio, mapping->host,
-- 
2.43.0

[PATCH v2 05/21] buffer: clear BH_Write_EIO when a buffer is forgotten

[Chao Shi]

BH_Write_EIO records that the last write of this buffer failed.  It is
cleared when the buffer is written again, but a filesystem freeing a
metadata block never writes it again.  It calls bforget() and hands the
block back to the allocator, so the flag outlives the block it refers to.

That does not matter much today, because the write error is also recorded
by clearing BH_Uptodate and the buffer is discarded soon after.  It starts
to matter in the rest of this series, which stops clearing BH_Uptodate on
write error and makes BH_Write_EIO the way a failed metadata write is
reported.

bforget() is where a filesystem says it no longer cares about this
buffer's contents, so clear the error there alongside the dirty flag.

Suggested-by: Jan Kara <jack@suse.cz>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/buffer.c b/fs/buffer.c
index 2851830995d8..381690479520 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1091,6 +1091,7 @@ EXPORT_SYMBOL(__brelse);
 void __bforget(struct buffer_head *bh)
 {
 	clear_buffer_dirty(bh);
+	clear_buffer_write_io_error(bh);
 	remove_assoc_queue(bh);
 	__brelse(bh);
 }
-- 
2.43.0

[PATCH v2 06/21] buffer: discard BH_Write_EIO along with the rest of the buffer state

[Chao Shi]

discard_buffer() strips the state that describes where a buffer lives and
what has happened to it, because after an invalidate none of it applies any
more.  BH_Write_EIO belongs in that set for the same reason: it describes a
write of the data that is being thrown away.

Leaving it set means a buffer_head reused for a different block starts life
carrying somebody else's write error.  Like the bforget() change, this is
mostly theoretical today and becomes load bearing once the rest of the
series makes BH_Write_EIO the report of a failed metadata write.

Suggested-by: Jan Kara <jack@suse.cz>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 381690479520..570ce7f495d3 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1518,7 +1518,7 @@ EXPORT_SYMBOL(folio_set_bh);
 /* Bits that are cleared during an invalidate */
 #define BUFFER_FLAGS_DISCARD \
 	(1 << BH_Mapped | 1 << BH_New | 1 << BH_Req | \
-	 1 << BH_Delay | 1 << BH_Unwritten)
+	 1 << BH_Delay | 1 << BH_Unwritten | 1 << BH_Write_EIO)
 
 static void discard_buffer(struct buffer_head * bh)
 {
-- 
2.43.0

[PATCH v2 07/21] buffer: detect metadata write errors with buffer_write_io_error()

[Chao Shi]

Both places in this file that report a metadata write error to a caller do
it by testing !buffer_uptodate() after waiting for the write.  That works
only because the write completion handlers clear BH_Uptodate when the write
fails, which is what this series is removing: a buffer whose write failed
still holds the correct data, and saying otherwise makes callers rewrite,
re-read or WARN over a buffer that was never wrong.

BH_Write_EIO is the flag that actually means "the last write of this buffer
failed", and both handlers already set it via mark_buffer_write_io_error().
Test that instead.

No behaviour change: today a failed write through bh_end_write() or
bh_end_async_write() sets BH_Write_EIO and clears BH_Uptodate together, so
the two tests agree.  They stop agreeing at the end of the series, and this
one stays right.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 570ce7f495d3..aebf74abbc49 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -618,7 +618,7 @@ int mmb_sync(struct mapping_metadata_bhs *mmb)
 		}
 		spin_unlock(&mmb->lock);
 		wait_on_buffer(bh);
-		if (!buffer_uptodate(bh))
+		if (buffer_write_io_error(bh))
 			err = -EIO;
 		brelse(bh);
 		spin_lock(&mmb->lock);
@@ -2743,7 +2743,7 @@ int __sync_dirty_buffer(struct buffer_head *bh, blk_opf_t op_flags)
 
 		bh_submit(bh, REQ_OP_WRITE | op_flags, bh_end_write);
 		wait_on_buffer(bh);
-		if (!buffer_uptodate(bh))
+		if (buffer_write_io_error(bh))
 			return -EIO;
 	} else {
 		unlock_buffer(bh);
-- 
2.43.0

[PATCH v2 08/21] adfs: check for a directory write error with buffer_write_io_error()

[Chao Shi]

adfs_dir_sync() spots a failed write by testing BH_Req together with
!BH_Uptodate.  That relies on the write completion handler clearing
BH_Uptodate on error, which this series removes: a buffer whose write
failed still holds the data the filesystem asked to be written, so
declaring it not up to date is wrong and makes callers re-read it.

BH_Write_EIO says exactly what this code wants to know, and it implies
BH_Req, so the pair collapses into one test.

No behaviour change today - a failed write sets BH_Write_EIO and clears
BH_Uptodate together.  It stops being a no-op at the end of the series,
where the new test is the one that still works.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/adfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 11afa9e157aa..b8cc6a697a05 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -191,7 +191,7 @@ static int adfs_dir_sync(struct adfs_dir *dir)
 	for (i = dir->nr_buffers - 1; i >= 0; i--) {
 		struct buffer_head *bh = dir->bhs[i];
 		sync_dirty_buffer(bh);
-		if (buffer_req(bh) && !buffer_uptodate(bh))
+		if (buffer_write_io_error(bh))
 			err = -EIO;
 	}
 
-- 
2.43.0

[PATCH v2 09/21] ext2: check for an xattr block write error with buffer_write_io_error()

[Chao Shi]

ext2_xattr_set2() spots a failed synchronous write by testing BH_Req
together with !BH_Uptodate.  That relies on the write completion handler
clearing BH_Uptodate on error, which this series removes: a buffer whose
write failed still holds the data the filesystem asked to be written, so
declaring it not up to date is wrong and makes callers re-read it.

BH_Write_EIO says exactly what this code wants to know, and it implies
BH_Req, so the pair collapses into one test.

No behaviour change today - a failed write sets BH_Write_EIO and clears
BH_Uptodate together.  It stops being a no-op at the end of the series,
where the new test is the one that still works.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/xattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index be63f89402a3..39005ec23fe5 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -769,7 +769,7 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
 		if (IS_SYNC(inode)) {
 			sync_dirty_buffer(new_bh);
 			error = -EIO;
-			if (buffer_req(new_bh) && !buffer_uptodate(new_bh))
+			if (buffer_write_io_error(new_bh))
 				goto cleanup;
 		}
 	}
-- 
2.43.0

[PATCH v2 10/21] omfs: check for an inode write error with buffer_write_io_error()

[Chao Shi]

__omfs_write_inode() spots a failed synchronous write, on both the primary
block and each mirror, by testing BH_Req together with !BH_Uptodate.  That
relies on the write completion handler clearing BH_Uptodate on error, which
this series removes: a buffer whose write failed still holds the data the
filesystem asked to be written, so declaring it not up to date is wrong and
makes callers re-read it.

BH_Write_EIO says exactly what this code wants to know, and it implies
BH_Req, so each pair collapses into one test.

No behaviour change today - a failed write sets BH_Write_EIO and clears
BH_Uptodate together.  It stops being a no-op at the end of the series,
where the new test is the one that still works.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/omfs/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/omfs/inode.c b/fs/omfs/inode.c
index 1d915ef72119..bc37029a4afb 100644
--- a/fs/omfs/inode.c
+++ b/fs/omfs/inode.c
@@ -145,7 +145,7 @@ static int __omfs_write_inode(struct inode *inode, int wait)
 	mark_buffer_dirty(bh);
 	if (wait) {
 		sync_dirty_buffer(bh);
-		if (buffer_req(bh) && !buffer_uptodate(bh))
+		if (buffer_write_io_error(bh))
 			sync_failed = 1;
 	}
 
@@ -159,7 +159,7 @@ static int __omfs_write_inode(struct inode *inode, int wait)
 		mark_buffer_dirty(bh2);
 		if (wait) {
 			sync_dirty_buffer(bh2);
-			if (buffer_req(bh2) && !buffer_uptodate(bh2))
+			if (buffer_write_io_error(bh2))
 				sync_failed = 1;
 		}
 		brelse(bh2);
-- 
2.43.0

[PATCH v2 11/21] exfat: check for a directory write error with buffer_write_io_error()

[Chao Shi]

exfat_update_bhs() waits for the writes it issued and then tests
!buffer_uptodate() to find the ones that failed.  That relies on the write
completion handler clearing BH_Uptodate on error, which this series
removes: a buffer whose write failed still holds the data the filesystem
asked to be written, so declaring it not up to date is wrong and makes
callers re-read it.

Test BH_Write_EIO, which is what the completion handler sets and what this
code actually wants to know.

No behaviour change today - a failed write sets BH_Write_EIO and clears
BH_Uptodate together.  It stops being a no-op at the end of the series,
where the new test is the one that still works.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/exfat/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
index 6f11a96a4ffa..dfd0bbf31c94 100644
--- a/fs/exfat/misc.c
+++ b/fs/exfat/misc.c
@@ -187,7 +187,7 @@ int exfat_update_bhs(struct buffer_head **bhs, int nr_bhs, int sync)
 
 	for (i = 0; i < nr_bhs && sync; i++) {
 		wait_on_buffer(bhs[i]);
-		if (!err && !buffer_uptodate(bhs[i]))
+		if (!err && buffer_write_io_error(bhs[i]))
 			err = -EIO;
 	}
 	return err;
-- 
2.43.0

[PATCH v2 12/21] fat: check for a metadata write error with buffer_write_io_error()

[Chao Shi]

fat_sync_bhs() waits for the writes it issued and then tests
!buffer_uptodate() to find the ones that failed.  That relies on the write
completion handler clearing BH_Uptodate on error, which this series
removes: a buffer whose write failed still holds the data the filesystem
asked to be written, so declaring it not up to date is wrong and makes
callers re-read it.

Test BH_Write_EIO, which is what the completion handler sets and what this
code actually wants to know.

No behaviour change today - a failed write sets BH_Write_EIO and clears
BH_Uptodate together.  It stops being a no-op at the end of the series,
where the new test is the one that still works.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/fat/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index be18f6b5819b..4a4cd0111e47 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -356,7 +356,7 @@ int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs)
 
 	for (i = 0; i < nr_bhs; i++) {
 		wait_on_buffer(bhs[i]);
-		if (!err && !buffer_uptodate(bhs[i]))
+		if (!err && buffer_write_io_error(bhs[i]))
 			err = -EIO;
 	}
 	return err;
-- 
2.43.0

[PATCH v2 13/21] ext4: check for a metadata write error with buffer_write_io_error()

[Chao Shi]

Two places detect a failed metadata write by testing !buffer_uptodate()
after waiting for it.  That relies on the write completion handler clearing
BH_Uptodate on error, which this series removes: a buffer whose write
failed still holds the data the filesystem asked to be written, so
declaring it not up to date is wrong and makes callers re-read it.

ext4 already does this correctly for the superblock - see
ext4_commit_super(), which tests buffer_write_io_error() - so this brings
the other two into line.

In __ext4_handle_dirty_metadata() the old test also required BH_Req.
BH_Write_EIO implies it, so the pair collapses into one test.  The new test
is also strictly stronger than consuming sync_dirty_buffer()'s return
value, because it still fires when the buffer was written by background
writeback and that write hit an error, which sync_dirty_buffer() does not
report.

Note that the failing write does not clear BH_Write_EIO, so an unrepaired
itable block now reports on every subsequent sync of that inode rather than
only on the write that failed.  That is the intended behaviour, and matches
what ocfs2 has always done with this flag.

No behaviour change today - a failed write sets BH_Write_EIO and clears
BH_Uptodate together.  It stops being a no-op at the end of the series,
where the new test is the one that still works.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4_jbd2.c | 2 +-
 fs/ext4/mmp.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index 02b066299164..f338d6e3c29f 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -413,7 +413,7 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
 		}
 		if (inode && inode_needs_sync(inode)) {
 			sync_dirty_buffer(bh);
-			if (buffer_req(bh) && !buffer_uptodate(bh)) {
+			if (buffer_write_io_error(bh)) {
 				ext4_error_inode_err(inode, where, line,
 						     bh->b_blocknr, EIO,
 					"IO error syncing itable block");
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 7ce361484b38..4b18ddef468d 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -49,7 +49,7 @@ static int write_mmp_block_thawed(struct super_block *sb,
 	bh_submit(bh, REQ_OP_WRITE | REQ_SYNC | REQ_META | REQ_PRIO,
 			bh_end_write);
 	wait_on_buffer(bh);
-	if (unlikely(!buffer_uptodate(bh)))
+	if (unlikely(buffer_write_io_error(bh)))
 		return -EIO;
 	return 0;
 }
-- 
2.43.0

[PATCH v2 14/21] ocfs2: check for a metadata write error with buffer_write_io_error()

[Chao Shi]

ocfs2_write_block() and ocfs2_write_super_or_backup() detect a failed write
by looking at BH_Uptodate afterwards.  That relies on the write completion
handler clearing BH_Uptodate on error, which this series removes: a buffer
whose write failed still holds the data the filesystem asked to be written,
so declaring it not up to date is wrong and makes callers re-read it.

Test BH_Write_EIO instead.  Note that ocfs2_write_block()'s test is the
positive one, so the sense has to be inverted rather than the flag simply
swapped.

The comment in ocfs2_write_block()'s error arm needs updating for the same
reason.  It said the clustered uptodate information did not have to be
removed because the buffer was not marked locally uptodate; after this
series it is, so the reason no longer holds.  Not advertising the block to
the cluster is still the right thing to do - the data is in memory but not
on disk - so only the justification changes, not the behaviour.

No behaviour change today - a failed write sets BH_Write_EIO and clears
BH_Uptodate together.  It stops being a no-op at the end of the series,
where the new test is the one that still works.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/buffer_head_io.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
index 7bfe377af2df..733ceda79ca1 100644
--- a/fs/ocfs2/buffer_head_io.c
+++ b/fs/ocfs2/buffer_head_io.c
@@ -66,12 +66,14 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh,
 
 	wait_on_buffer(bh);
 
-	if (buffer_uptodate(bh)) {
+	if (!buffer_write_io_error(bh)) {
 		ocfs2_set_buffer_uptodate(ci, bh);
 	} else {
-		/* We don't need to remove the clustered uptodate
-		 * information for this bh as it's not marked locally
-		 * uptodate. */
+		/*
+		 * The buffer still holds what we tried to write, but it did
+		 * not reach the disk, so don't advertise it to the cluster
+		 * as up to date.
+		 */
 		ret = -EIO;
 		mlog_errno(ret);
 	}
@@ -446,7 +448,7 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
 
 	wait_on_buffer(bh);
 
-	if (!buffer_uptodate(bh)) {
+	if (buffer_write_io_error(bh)) {
 		ret = -EIO;
 		mlog_errno(ret);
 	}
-- 
2.43.0

[PATCH v2 15/21] ocfs2: check for a stale write error before reusing a metadata buffer

[Chao Shi]

__ocfs2_journal_access() refuses to journal a buffer whose previous write
failed, and turns the filesystem read-only rather than risk metadata
inconsistency.  That check sits inside an if (!buffer_uptodate(bh)) block,
because until now a failed write also cleared BH_Uptodate.

This series stops clearing BH_Uptodate on write error, so that outer test
would never fire again and ocfs2 would silently start reusing buffers whose
last write failed.  Hoist the check out of the debug block, where it does
not depend on BH_Uptodate any more, and drop the now dead second half of
its condition.

The mlog() pair keeps its own !buffer_uptodate() guard: it is a separate
"we can safely remove this assertion after testing" debug aid about being
handed a buffer with no valid contents, which is a different question from
whether the last write of that buffer failed.

The unlocked test followed by a locked retest is deliberate.  BH_Write_EIO
is cleared under the buffer lock, so taking the lock and looking a second
time avoids turning the
filesystem read-only over an error that a concurrent rewrite has already
cleared, while keeping the common case lock-free.

The code in this patch is Jan's, from the review discussion linked in the
cover letter.

Suggested-by: Jan Kara <jack@suse.cz>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/journal.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index d8afbc1a76bb..ea6802d894c2 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -676,19 +676,20 @@ static int __ocfs2_journal_access(handle_t *handle,
 		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);
-		/*
-		 * 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) && !buffer_uptodate(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");
-- 
2.43.0

[PATCH v2 16/21] gfs2: check for a metadata write error with buffer_write_io_error()

[Chao Shi]

gfs2_ail1_start_one() and gfs2_ail1_empty_one() decide whether a buffer on
the ail reached the disk by looking at BH_Uptodate once it is no longer
busy.  That relies on the write completion handler clearing BH_Uptodate on
error, which this series removes: a buffer whose write failed still holds
the data the filesystem asked to be written, so declaring it not up to date
is wrong and makes callers re-read it.  Test BH_Write_EIO instead.  In
gfs2_ail1_start_one() the test is the positive one, so the sense has to be
inverted rather than the flag simply swapped.

This is not a pure conversion for gfs2, because gfs2 already has a private
write completion handler that behaves the way this series is heading:
gfs2_end_log_write_bh() calls mark_buffer_write_io_error() and leaves
BH_Uptodate alone.  Buffers completed through it are therefore invisible to
both tests today, and start being caught once they look at BH_Write_EIO.
That is a real behaviour change, and it is the one gfs2 wanted: a failed
log write now withdraws the filesystem instead of passing silently.

gfs2_pin() is a different case and gets a different treatment.  Its
!buffer_uptodate() test is not only a proxy for a failed write - a buffer
with no valid contents at all is equally a reason to withdraw before
pinning it into a transaction - so the write error test is added to it
rather than replacing it.

Left alone deliberately: the BUG_ON(!buffer_uptodate(bh)) in gfs2_unpin()
and the two WARN_ON()s in fs/gfs2/rgrp.c.  After this series they simply
stop firing for write errors, which is correct; turning them into
BUG_ON(buffer_write_io_error(bh)) would newly panic on an I/O error.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/gfs2/log.c  | 4 ++--
 fs/gfs2/lops.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 78bba8cc10b8..e3e0dcb1f567 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -107,7 +107,7 @@ __acquires(&sdp->sd_ail_lock)
 		gfs2_assert(sdp, bd->bd_tr == tr);
 
 		if (!buffer_busy(bh)) {
-			if (buffer_uptodate(bh)) {
+			if (!buffer_write_io_error(bh)) {
 				list_move(&bd->bd_ail_st_list,
 					  &tr->tr_ail2_list);
 				continue;
@@ -321,7 +321,7 @@ static int gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr,
 			active_count++;
 			continue;
 		}
-		if (!buffer_uptodate(bh) &&
+		if (buffer_write_io_error(bh) &&
 		    !cmpxchg(&sdp->sd_log_error, 0, -EIO))
 			gfs2_io_error_bh(sdp, bh);
 		/*
diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 6dabe73ad790..3df6e4b7e8b9 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -48,7 +48,7 @@ void gfs2_pin(struct gfs2_sbd *sdp, struct buffer_head *bh)
 	clear_buffer_dirty(bh);
 	if (test_set_buffer_pinned(bh))
 		gfs2_assert_withdraw(sdp, 0);
-	if (!buffer_uptodate(bh))
+	if (!buffer_uptodate(bh) || buffer_write_io_error(bh))
 		gfs2_io_error_bh(sdp, bh);
 	bd = bh->b_private;
 	/* If this buffer is in the AIL and it has already been written
-- 
2.43.0

[PATCH v2 17/21] jbd2: report journal write errors with BH_Write_EIO

[Chao Shi]

The journal's own write completion handler,
journal_end_buffer_io_sync(), reports a failed write by clearing
BH_Uptodate, and the three places that wait for journal writes look for
that.  This series is removing that convention: a buffer whose write failed
still holds the data that was supposed to reach the disk, and saying it is
not up to date makes callers rewrite, re-read or WARN over data that was
never wrong.

Set BH_Write_EIO instead, with mark_buffer_write_io_error(), and test it in
journal_wait_on_commit_record() and in the two commit-phase waits.

The handler stops touching BH_Uptodate in either direction.  Setting it on
success was never needed: every caller marks the buffer up to date before
submitting the write, because a buffer with no valid data is not something
you can write out.  The local flag is renamed to match what it now means.

The two changes have to go together, because commit phase 4 waits on a
mixed list: descriptor blocks are submitted with
journal_end_buffer_io_sync(), while revoke blocks go through
write_dirty_buffer() and land in bh_end_write().  bh_end_write() already
sets BH_Write_EIO, so converting the consumer alone would keep working for
revoke blocks and silently stop detecting failed descriptor writes.  With
the handler converted, both halves of the list report the same way.

mark_buffer_write_io_error() is safe on all of these buffers.  The shadow
buffers from jbd2_journal_write_metadata_buffer() have no folio and no
associated mapping, so it does nothing beyond setting the flag.  Descriptor
and commit blocks are ordinary buffers on the journal device, and marking
the journal's mapping with the error is what write_dirty_buffer() already
does for revoke blocks on the same device.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/jbd2/commit.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 0c85af91f9b2..cd7ef783bd36 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -32,14 +32,12 @@
 static void journal_end_buffer_io_sync(struct bio *bio)
 {
 	struct buffer_head *bh;
-	bool uptodate = bio_endio_bh(bio, &bh);
+	bool success = bio_endio_bh(bio, &bh);
 	struct buffer_head *orig_bh = bh->b_private;
 
 	BUFFER_TRACE(bh, "");
-	if (uptodate)
-		set_buffer_uptodate(bh);
-	else
-		clear_buffer_uptodate(bh);
+	if (!success)
+		mark_buffer_write_io_error(bh);
 	if (orig_bh) {
 		clear_and_wake_up_bit(BH_Shadow, &orig_bh->b_state);
 	}
@@ -169,7 +167,7 @@ static int journal_wait_on_commit_record(journal_t *journal,
 	clear_buffer_dirty(bh);
 	wait_on_buffer(bh);
 
-	if (unlikely(!buffer_uptodate(bh)))
+	if (unlikely(buffer_write_io_error(bh)))
 		ret = -EIO;
 	put_bh(bh);            /* One for getblk() */
 
@@ -834,7 +832,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		wait_on_buffer(bh);
 		cond_resched();
 
-		if (unlikely(!buffer_uptodate(bh)))
+		if (unlikely(buffer_write_io_error(bh)))
 			err = -EIO;
 		jbd2_unfile_log_bh(bh);
 		stats.run.rs_blocks_logged++;
@@ -877,7 +875,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		wait_on_buffer(bh);
 		cond_resched();
 
-		if (unlikely(!buffer_uptodate(bh)))
+		if (unlikely(buffer_write_io_error(bh)))
 			err = -EIO;
 
 		BUFFER_TRACE(bh, "ph5: control buffer writeout done: unfile");
-- 
2.43.0

[PATCH v2 18/21] jbd2: say what jbd2_freeze_jh_data()'s assertion is actually checking

[Chao Shi]

The assertion that the buffer about to be copied out is up to date is
correct and stays, but its message - "Possible IO failure" - describes what
a buffer that is not up to date used to mean rather than what is being
checked.

Once this series stops clearing BH_Uptodate on write error, that reading is
wrong twice over.  A failed write no longer makes a buffer not up to date,
and a buffer that does carry BH_Write_EIO is fine here: it still holds
valid data and the journal will write it again.  What the assertion is
really guarding is that there is something valid to copy at all.

Say that instead.

Suggested-by: Jan Kara <jack@suse.cz>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/jbd2/transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 5cc7d097b2ac..85d84d909f78 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -920,7 +920,7 @@ static void jbd2_freeze_jh_data(struct journal_head *jh)
 	char *source;
 	struct buffer_head *bh = jh2bh(jh);
 
-	J_EXPECT_JH(jh, buffer_uptodate(bh), "Possible IO failure.\n");
+	J_EXPECT_JH(jh, buffer_uptodate(bh), "Buffer not uptodate!\n");
 	source = kmap_local_folio(bh->b_folio, bh_offset(bh));
 	/* Fire data frozen trigger just before we copy the data */
 	jbd2_buffer_frozen_trigger(jh, source, jh->b_triggers);
-- 
2.43.0

[PATCH v2 19/21] ext4, jbd2: report fast commit write errors with BH_Write_EIO

[Chao Shi]

ext4_end_buffer_io_sync() is the third completion handler in this
series that reports a failed write by clearing BH_Uptodate, and
jbd2_fc_wait_bufs() is the only thing that looks at the result.
Convert both.

Like the jbd2 handler, this one stops touching BH_Uptodate at all.
ext4_fc_submit_bh() marks the buffer up to date before submitting, so
setting it again on completion said nothing.  The local flag and the debug
messages are reworded to describe the write rather than the buffer's
contents, which the write does not change.

They have to move in the same patch.  The handler lives in ext4 and
the wait in jbd2, but neither is used by anything else: the buffers
are ext4's fast commit blocks, submitted by ext4_fc_submit_bh() and
waited for by jbd2_fc_wait_bufs().  Converting one without the other
silently disables fast commit write error reporting.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/ext4/fast_commit.c | 11 +++++------
 fs/jbd2/journal.c     |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 8e2259799614..a2028fbd4540 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -203,17 +203,16 @@ static inline void ext4_fc_set_snap_err(int *snap_err, int err)
 static void ext4_end_buffer_io_sync(struct bio *bio)
 {
 	struct buffer_head *bh;
-	bool uptodate = bio_endio_bh(bio, &bh);
+	bool success = bio_endio_bh(bio, &bh);
 
 	BUFFER_TRACE(bh, "");
-	if (uptodate) {
-		ext4_debug("%s: Block %lld up-to-date",
+	if (success) {
+		ext4_debug("%s: Block %lld written",
 			   __func__, bh->b_blocknr);
-		set_buffer_uptodate(bh);
 	} else {
-		ext4_debug("%s: Block %lld not up-to-date",
+		ext4_debug("%s: Block %lld write failed",
 			   __func__, bh->b_blocknr);
-		clear_buffer_uptodate(bh);
+		mark_buffer_write_io_error(bh);
 	}
 
 	unlock_buffer(bh);
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 6e05dc47e20a..72e8ccbf7de4 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -886,7 +886,7 @@ int jbd2_fc_wait_bufs(journal_t *journal, int num_blks)
 		 * Update j_fc_off so jbd2_fc_release_bufs can release remain
 		 * buffer head.
 		 */
-		if (unlikely(!buffer_uptodate(bh))) {
+		if (unlikely(buffer_write_io_error(bh))) {
 			journal->j_fc_off = i + 1;
 			return -EIO;
 		}
-- 
2.43.0

[PATCH v2 20/21] buffer: stop touching BH_Uptodate on write completion

[Chao Shi]

A buffer whose write failed still holds exactly the data the filesystem
asked to be written.  It is the disk that is out of date, not the buffer.
Clearing BH_Uptodate says the opposite, and callers act on it:

 - mark_buffer_dirty() has a WARN_ON_ONCE(!buffer_uptodate(bh)).  A
   filesystem that dirties the buffer again after a failed write - which is
   the normal way to retry - trips it.  That is the warning this series
   started from.

 - a buffer that is not up to date gets re-read from disk, which replaces
   the data the filesystem was trying to write with the stale on-disk copy,
   silently.

 - the window between the write completing and the buffer being marked not
   up to date is visible to anyone holding the folio lock, so the state is
   not even self consistent while it lasts.

BH_Write_EIO already records the failure, and by now every place in the
tree that needs to know about it tests that flag instead: the two core
helpers in this file, adfs, exfat, ext2, ext4, fat, gfs2, jbd2, ocfs2 and
omfs, converted one filesystem at a time in the preceding patches.  The
private completion handlers in jbd2 and ext4 fast commit were converted
along with their waiters.  Nothing is left that reads BH_Uptodate to find
out whether a write failed.

Setting BH_Uptodate on success goes too.  A buffer has to be up to date
before it can be written - you cannot write out data you do not have - so
the only thing that assignment could do is paper over a caller that got
that wrong.  Write completion now leaves BH_Uptodate alone in both
directions.

What this changes for readers.  A buffer whose write failed stays up to
date, so the read paths stop replacing it with the on-disk copy:
__bread_gfp() no longer sends it to __bread_slow(), and
bh_uptodate_or_lock() reports it as usable.  That is the intent.  ocfs2
changes the most, because ocfs2_read_blocks() decides whether to go to disk
on its own cluster uptodate cache and only tests BH_Uptodate after the
wait, so a block whose write failed makes that read return -EIO today and
from here it succeeds and hands back the in-memory data.  A caller that
needs to know the write failed asks BH_Write_EIO.

Found by FuzzNvme.

Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/buffer.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index aebf74abbc49..425fbfe72ad1 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -202,12 +202,9 @@ void bh_end_write(struct bio *bio)
 	struct buffer_head *bh;
 	bool success = bio_endio_bh(bio, &bh);
 
-	if (success) {
-		set_buffer_uptodate(bh);
-	} else {
+	if (!success) {
 		buffer_io_error(bh, ", lost sync page write");
 		mark_buffer_write_io_error(bh);
-		clear_buffer_uptodate(bh);
 	}
 	unlock_buffer(bh);
 }
@@ -436,12 +433,9 @@ void bh_end_async_write(struct bio *bio)
 	BUG_ON(!buffer_async_write(bh));
 
 	folio = bh->b_folio;
-	if (success) {
-		set_buffer_uptodate(bh);
-	} else {
+	if (!success) {
 		buffer_io_error(bh, ", lost async page write");
 		mark_buffer_write_io_error(bh);
-		clear_buffer_uptodate(bh);
 	}
 
 	first = folio_buffers(folio);
-- 
2.43.0

[PATCH v2 21/21] buffer: clear BH_Write_EIO when a write succeeds, not when one starts

[Chao Shi]

BH_Write_EIO is cleared in __bh_submit(), when a buffer that has been
written before is submitted for write again.  That is early: it says the
error is gone at the moment we start trying to fix it, rather than when we
have.

It also loses errors.  A task whose write fails sets the flag and then goes
to look at it; if another task redirties the buffer and resubmits it in
between, the submission clears the flag and the first task sees no error at
all.  Neither of them is doing anything wrong.

Clear it on successful write completion instead, in the end io handlers -
the same four the rest of this series has been converting, plus gfs2's,
which already marked errors this way.  Then the flag means what it says:
the last write of this buffer that finished, failed.  A resubmission no
longer hides an error that has not been fixed yet, and one that has been
fixed clears the flag when the data reaches the disk.

__bh_submit() keeps setting BH_Req, which is what the rest of the tree
reads it for.

Suggested-by: Jan Kara <jack@suse.cz>
Acked-by: Weidong Zhu <weizhu@fiu.edu>
Signed-off-by: Chao Shi <coshi036@gmail.com>
---
 fs/buffer.c           | 15 +++++++--------
 fs/ext4/fast_commit.c |  1 +
 fs/gfs2/lops.c        |  2 ++
 fs/jbd2/commit.c      |  4 +++-
 4 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index 425fbfe72ad1..68ea0ef8470e 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -202,7 +202,9 @@ void bh_end_write(struct bio *bio)
 	struct buffer_head *bh;
 	bool success = bio_endio_bh(bio, &bh);
 
-	if (!success) {
+	if (success) {
+		clear_buffer_write_io_error(bh);
+	} else {
 		buffer_io_error(bh, ", lost sync page write");
 		mark_buffer_write_io_error(bh);
 	}
@@ -433,7 +435,9 @@ void bh_end_async_write(struct bio *bio)
 	BUG_ON(!buffer_async_write(bh));
 
 	folio = bh->b_folio;
-	if (!success) {
+	if (success) {
+		clear_buffer_write_io_error(bh);
+	} else {
 		buffer_io_error(bh, ", lost async page write");
 		mark_buffer_write_io_error(bh);
 	}
@@ -1114,7 +1118,6 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
 		enum rw_hint write_hint, struct writeback_control *wbc,
 		bio_end_io_t end_bio)
 {
-	const enum req_op op = opf & REQ_OP_MASK;
 	struct bio *bio;
 
 	BUG_ON(!buffer_locked(bh));
@@ -1122,11 +1125,7 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
 	BUG_ON(buffer_delay(bh));
 	BUG_ON(buffer_unwritten(bh));
 
-	/*
-	 * Only clear out a write error when rewriting
-	 */
-	if (test_set_buffer_req(bh) && (op == REQ_OP_WRITE))
-		clear_buffer_write_io_error(bh);
+	set_buffer_req(bh);
 
 	if (buffer_meta(bh))
 		opf |= REQ_META;
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index a2028fbd4540..95998827ff01 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -209,6 +209,7 @@ static void ext4_end_buffer_io_sync(struct bio *bio)
 	if (success) {
 		ext4_debug("%s: Block %lld written",
 			   __func__, bh->b_blocknr);
+		clear_buffer_write_io_error(bh);
 	} else {
 		ext4_debug("%s: Block %lld write failed",
 			   __func__, bh->b_blocknr);
diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 3df6e4b7e8b9..7440e5b72f8a 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -179,6 +179,8 @@ static void gfs2_end_log_write_bh(struct gfs2_sbd *sdp, struct folio *folio,
 	do {
 		if (error)
 			mark_buffer_write_io_error(bh);
+		else
+			clear_buffer_write_io_error(bh);
 		unlock_buffer(bh);
 		next = bh->b_this_page;
 		size -= bh->b_size;
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index cd7ef783bd36..ebf6ba58ff4d 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -36,7 +36,9 @@ static void journal_end_buffer_io_sync(struct bio *bio)
 	struct buffer_head *orig_bh = bh->b_private;
 
 	BUFFER_TRACE(bh, "");
-	if (!success)
+	if (success)
+		clear_buffer_write_io_error(bh);
+	else
 		mark_buffer_write_io_error(bh);
 	if (orig_bh) {
 		clear_and_wake_up_bit(BH_Shadow, &orig_bh->b_state);
-- 
2.43.0