buffer: stop clearing BH_Uptodate when a write fails

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) <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/jbd2/commit.c            |  8 ++++----
 fs/jbd2/journal.c           | 29 +++++++++++++++++------------
 include/linux/buffer_head.h | 29 +++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 16 deletions(-)

fs/jbd2/commit.c +4 -4

index 3029cb6f6d64..0c85af91f9b2 100644
@@ -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);

fs/jbd2/journal.c +17 -12

index 09efa337649e..6e05dc47e20a 100644
@@ -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;

include/linux/buffer_head.h +29 -0

index 699970b4bbf2..20b8fca1abfa 100644
@@ -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)					\
 	({							\

Signature · 1 line

2.43.0

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) <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/jbd2/commit.c            |  8 ++++----
 fs/jbd2/journal.c           | 29 +++++++++++++++++------------
 include/linux/buffer_head.h | 29 +++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 16 deletions(-)

fs/jbd2/commit.c +4 -4

index 3029cb6f6d64..0c85af91f9b2 100644
@@ -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);

fs/jbd2/journal.c +17 -12

index 09efa337649e..6e05dc47e20a 100644
@@ -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;

include/linux/buffer_head.h +22 -0

index 699970b4bbf2..20b8fca1abfa 100644
@@ -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().

Quoted 187 lines · 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) <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
  fs/jbd2/commit.c            |  8 ++++----
  fs/jbd2/journal.c           | 29 +++++++++++++++++------------
  include/linux/buffer_head.h | 29 +++++++++++++++++++++++++++++
  3 files changed, 50 insertions(+), 16 deletions(-)

fs/jbd2/commit.c +4 -4

index 3029cb6f6d64..0c85af91f9b2 100644
@@ -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);

fs/jbd2/journal.c +17 -12

index 09efa337649e..6e05dc47e20a 100644
@@ -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;

include/linux/buffer_head.h +22 -0

index 699970b4bbf2..20b8fca1abfa 100644
@@ -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().

include/linux/buffer_head.h static inline unsigned long bh_offset(const struct buffer_head *bh) · line 206

Quoted 10 lines

include/linux/buffer_head.h +7 -0

+ */
+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)					\
  	({							\

When tested ocfs2 on next-20260831, I've encountered the following NULL pointer dereference:

   BUG: kernel NULL pointer dereference, address: 0000000000000000
   RIP: 0010:__bh_submit.constprop.0+0x87/0x120
   Call Trace:
     jbd2_journal_commit_transaction+0x932/0x1b10
     kjournald2+0xb2/0x250

Commit a2c924c240e7 ("buffer: set BIO_COMPLETE_IN_TASK for dropbehind writeback") added an unconditional folio_test_dropbehind(bh->b_folio) in __bh_submit(). But jbd2 shadow buffers have a NULL b_folio since commit 5febcba29792 ("jbd2: point the shadow buffer at the frozen data directly") made them point b_data at the kmalloced frozen data rather than a folio. So submitting such a buffer during journal commit oopses.

A simple fix:

fs/buffer.c +2 -1

index 427d8a817cd5..f46fa6413032 100644
@@ -1106,7 +1106,8 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
 
  	bio = bio_alloc(bh->b_bdev, 1, opf, GFP_NOIO);
 
-	if (folio_test_dropbehind(bh->b_folio) && op_is_write(opf))
+	if (bh->b_folio && folio_test_dropbehind(bh->b_folio) &&
+	    op_is_write(opf))
  		bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
 
  	if (IS_ENABLED(CONFIG_FS_ENCRYPTION))

Thanks,
Joseph

Quoted 187 lines · 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) <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/jbd2/commit.c            |  8 ++++----
 fs/jbd2/journal.c           | 29 +++++++++++++++++------------
 include/linux/buffer_head.h | 29 +++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 16 deletions(-)

fs/jbd2/commit.c +4 -4

index 3029cb6f6d64..0c85af91f9b2 100644
@@ -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);

fs/jbd2/journal.c +17 -12

index 09efa337649e..6e05dc47e20a 100644
@@ -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;

include/linux/buffer_head.h +22 -0

index 699970b4bbf2..20b8fca1abfa 100644
@@ -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().

include/linux/buffer_head.h static inline unsigned long bh_offset(const struct buffer_head *bh) · line 206

Quoted 10 lines

include/linux/buffer_head.h +7 -0

+ */
+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)					\
 	({							\

When tested ocfs2 on next-20260831, I've encountered the following NULL pointer dereference:

  BUG: kernel NULL pointer dereference, address: 0000000000000000
  RIP: 0010:__bh_submit.constprop.0+0x87/0x120
  Call Trace:
    jbd2_journal_commit_transaction+0x932/0x1b10
    kjournald2+0xb2/0x250

Commit a2c924c240e7 ("buffer: set BIO_COMPLETE_IN_TASK for dropbehind writeback") added an unconditional folio_test_dropbehind(bh->b_folio) in __bh_submit(). But jbd2 shadow buffers have a NULL b_folio since commit 5febcba29792 ("jbd2: point the shadow buffer at the frozen data directly") made them point b_data at the kmalloced frozen data rather than a folio. So submitting such a buffer during journal commit oopses.

A simple fix:

fs/buffer.c +0 -0

index 427d8a817cd5..f46fa6413032 100644

Thanks,
Joseph

Quoted 187 lines · 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) <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/jbd2/commit.c            |  8 ++++----
 fs/jbd2/journal.c           | 29 +++++++++++++++++------------
 include/linux/buffer_head.h | 29 +++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 16 deletions(-)

fs/jbd2/commit.c +4 -4

index 3029cb6f6d64..0c85af91f9b2 100644
@@ -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);

fs/jbd2/journal.c +17 -12

index 09efa337649e..6e05dc47e20a 100644
@@ -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;

include/linux/buffer_head.h +22 -0

index 699970b4bbf2..20b8fca1abfa 100644
@@ -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().

include/linux/buffer_head.h static inline unsigned long bh_offset(const struct buffer_head *bh) · line 206

Quoted 10 lines

include/linux/buffer_head.h +7 -0

+ */
+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)					\
 	({							\

When tested ocfs2 on next-20260831, I've encountered the following NULL pointer dereference:

  BUG: kernel NULL pointer dereference, address: 0000000000000000
  RIP: 0010:__bh_submit.constprop.0+0x87/0x120
  Call Trace:
    jbd2_journal_commit_transaction+0x932/0x1b10
    kjournald2+0xb2/0x250

Commit a2c924c240e7 ("buffer: set BIO_COMPLETE_IN_TASK for dropbehind writeback") added an unconditional folio_test_dropbehind(bh->b_folio) in __bh_submit(). But jbd2 shadow buffers have a NULL b_folio since commit 5febcba29792 ("jbd2: point the shadow buffer at the frozen data directly") made them point b_data at the kmalloced frozen data rather than a folio. So submitting such a buffer during journal commit oopses.

A simple fix:

fs/buffer.c +0 -0

index 427d8a817cd5..f46fa6413032 100644

Quoted 187 lines · 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) <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---

fs/jbd2/commit.c            |  8 ++++----
fs/jbd2/journal.c           | 29 +++++++++++++++++------------
include/linux/buffer_head.h | 29 +++++++++++++++++++++++++++++
3 files changed, 50 insertions(+), 16 deletions(-)

fs/jbd2/commit.c +4 -4

index 3029cb6f6d64..0c85af91f9b2 100644
@@ -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);

fs/jbd2/journal.c +16 -9

index 09efa337649e..6e05dc47e20a 100644
@@ -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;

include/linux/buffer_head.h +22 -0

index 699970b4bbf2..20b8fca1abfa 100644
@@ -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().

include/linux/buffer_head.h static inline unsigned long bh_offset(const struct buffer_head *bh) · line 206

Quoted 10 lines

include/linux/buffer_head.h +7 -0

+ */
+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) \
 ({ \

When tested ocfs2 on next-20260831, I've encountered the following NULL pointer dereference:

 BUG: kernel NULL pointer dereference, address: 0000000000000000
 RIP: 0010:__bh_submit.constprop.0+0x87/0x120
 Call Trace:
   jbd2_journal_commit_transaction+0x932/0x1b10
   kjournald2+0xb2/0x250

Commit a2c924c240e7 ("buffer: set BIO_COMPLETE_IN_TASK for dropbehind writeback") added an unconditional folio_test_dropbehind(bh->b_folio) in __bh_submit(). But jbd2 shadow buffers have a NULL b_folio since commit 5febcba29792 ("jbd2: point the shadow buffer at the frozen data directly") made them point b_data at the kmalloced frozen data rather than a folio. So submitting such a buffer during journal commit oopses.

A simple fix:

fs/buffer.c +0 -0

index 427d8a817cd5..f46fa6413032 100644

Thanks,
Joseph

Just a quick reminder, in linux next-20260909 this has not been fixed, yet.

Bert Karwatzki

Quoted 187 lines · 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) <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/jbd2/commit.c            |  8 ++++----
 fs/jbd2/journal.c           | 29 +++++++++++++++++------------
 include/linux/buffer_head.h | 29 +++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 16 deletions(-)

fs/jbd2/commit.c +4 -4

index 3029cb6f6d64..0c85af91f9b2 100644
@@ -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);

fs/jbd2/journal.c +17 -12

index 09efa337649e..6e05dc47e20a 100644
@@ -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;

include/linux/buffer_head.h +22 -0

index 699970b4bbf2..20b8fca1abfa 100644
@@ -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().

include/linux/buffer_head.h static inline unsigned long bh_offset(const struct buffer_head *bh) · line 206

Quoted 10 lines

include/linux/buffer_head.h +7 -0

+ */
+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)                                   \
      ({                                                      \

When tested ocfs2 on next-20260831, I've encountered the following NULL pointer dereference:

  BUG: kernel NULL pointer dereference, address: 0000000000000000
  RIP: 0010:__bh_submit.constprop.0+0x87/0x120
  Call Trace:
    jbd2_journal_commit_transaction+0x932/0x1b10
    kjournald2+0xb2/0x250

Commit a2c924c240e7 ("buffer: set BIO_COMPLETE_IN_TASK for dropbehind writeback") added an unconditional folio_test_dropbehind(bh->b_folio) in __bh_submit(). But jbd2 shadow buffers have a NULL b_folio since commit 5febcba29792 ("jbd2: point the shadow buffer at the frozen data directly") made them point b_data at the kmalloced frozen data rather than a folio. So submitting such a buffer during journal commit oopses.

A simple fix:

fs/buffer.c +2 -1

index 427d8a817cd5..f46fa6413032 100644
@@ -1106,7 +1106,8 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
 
        bio = bio_alloc(bh->b_bdev, 1, opf, GFP_NOIO);
 
-       if (folio_test_dropbehind(bh->b_folio) && op_is_write(opf))
+       if (bh->b_folio && folio_test_dropbehind(bh->b_folio) &&
+           op_is_write(opf))
                bio_set_flag(bio, BIO_COMPLETE_IN_TASK);
 
        if (IS_ENABLED(CONFIG_FS_ENCRYPTION))

Thanks,
Joseph

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) <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/buffer.c +1 -1

index 04fcc34e4fa6..2851830995d8 100644
@@ -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,

Signature · 1 line

2.43.0

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 <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/adfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/adfs/dir.c +1 -1

index 11afa9e157aa..b8cc6a697a05 100644
@@ -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;
 	}
 

Signature · 1 line

2.43.0

__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 <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/omfs/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/omfs/inode.c +2 -2

index 1d915ef72119..bc37029a4afb 100644
@@ -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);

Signature · 1 line

2.43.0

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 <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/exfat/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/exfat/misc.c +1 -1

index 6f11a96a4ffa..dfd0bbf31c94 100644
@@ -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;

Signature · 1 line

2.43.0

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 <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/fat/misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/fat/misc.c +1 -1

index be18f6b5819b..4a4cd0111e47 100644
@@ -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;

Signature · 1 line

2.43.0

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 <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/gfs2/log.c  | 4 ++--
 fs/gfs2/lops.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

fs/gfs2/log.c +2 -2

index 78bba8cc10b8..e3e0dcb1f567 100644
@@ -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);
 		/*

fs/gfs2/lops.c +1 -1

index 6dabe73ad790..3df6e4b7e8b9 100644
@@ -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

Signature · 1 line

2.43.0

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 <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/jbd2/commit.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

fs/jbd2/commit.c +6 -8

index 0c85af91f9b2..cd7ef783bd36 100644
@@ -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");

Signature · 1 line

2.43.0

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 <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/jbd2/transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/jbd2/transaction.c +1 -1

index 5cc7d097b2ac..85d84d909f78 100644
@@ -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);

Signature · 1 line

2.43.0

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 <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/ext4/fast_commit.c | 11 +++++------
 fs/jbd2/journal.c     |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

fs/ext4/fast_commit.c +5 -6

index 8e2259799614..a2028fbd4540 100644
@@ -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);

fs/jbd2/journal.c +1 -1

index 6e05dc47e20a..72e8ccbf7de4 100644
@@ -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;
 		}

Signature · 1 line

2.43.0

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 <[email protected]>

Signed-off-by Chao Shi <[email protected]>

---
 fs/buffer.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

fs/buffer.c +2 -8

index aebf74abbc49..425fbfe72ad1 100644
@@ -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);

Signature · 1 line

2.43.0

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 <[email protected]>

Acked-by Weidong Zhu <[email protected]>

Signed-off-by Chao Shi <[email protected]>

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

fs/buffer.c +7 -8

index 425fbfe72ad1..68ea0ef8470e 100644
@@ -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;

fs/ext4/fast_commit.c +1 -0

index a2028fbd4540..95998827ff01 100644
@@ -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);

fs/gfs2/lops.c +2 -0

index 3df6e4b7e8b9..7440e5b72f8a 100644
@@ -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;

fs/jbd2/commit.c +3 -1

index cd7ef783bd36..ebf6ba58ff4d 100644
@@ -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);

Signature · 1 line

2.43.0

buffer: stop clearing BH_Uptodate when a write fails

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:

[...]