Merge patch series "buffer: stop clearing BH_Uptodate when a write fails"

Chao Shi <coshi036@gmail.com> says:

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.

* patches from https://patch.msgid.link/cover.1785951556.git.coshi036@gmail.com: (21 commits)
  buffer: clear BH_Write_EIO when a write succeeds, not when one starts
  buffer: stop touching BH_Uptodate on write completion
  jbd2: check for a fast commit write error with buffer_write_io_error()
  jbd2: say what jbd2_freeze_jh_data()'s assertion is actually checking
  jbd2: report journal write errors with BH_Write_EIO
  gfs2: check for a metadata write error with buffer_write_io_error()
  ocfs2: check for a stale write error before reusing a metadata buffer
  ocfs2: check for a metadata write error with buffer_write_io_error()
  ext4: check for a metadata write error with buffer_write_io_error()
  fat: check for a metadata write error with buffer_write_io_error()
  exfat: check for a directory write error with buffer_write_io_error()
  omfs: check for an inode write error with buffer_write_io_error()
  ext2: check for an xattr block write error with buffer_write_io_error()
  adfs: check for a directory write error with buffer_write_io_error()
  buffer: detect metadata write errors with buffer_write_io_error()
  buffer: discard BH_Write_EIO along with the rest of the buffer state
  buffer: clear BH_Write_EIO when a buffer is forgotten
  buffer: read the folio's mapping directly in buffer_set_crypto_ctx()
  jbd2: point the shadow buffer at the frozen data directly
  buffer: allow a buffer_head to point at memory outside the page cache
  ...

Link: https://patch.msgid.link/cover.1785951556.git.coshi036@gmail.com
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
This commit is contained in:
Christian Brauner 2026-08-31 10:19:24 +02:00
commit 00db7c70bf
No known key found for this signature in database
GPG Key ID: 91C61BC06578DCA2
16 changed files with 116 additions and 76 deletions

View File

@ -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;
}

View File

@ -203,11 +203,10 @@ void bh_end_write(struct bio *bio)
bool success = bio_endio_bh(bio, &bh);
if (success) {
set_buffer_uptodate(bh);
clear_buffer_write_io_error(bh);
} else {
buffer_io_error(bh, ", lost sync page write");
mark_buffer_write_io_error(bh);
clear_buffer_uptodate(bh);
}
unlock_buffer(bh);
}
@ -408,11 +407,10 @@ void bh_end_async_write(struct bio *bio)
folio = bh->b_folio;
if (success) {
set_buffer_uptodate(bh);
clear_buffer_write_io_error(bh);
} else {
buffer_io_error(bh, ", lost async page write");
mark_buffer_write_io_error(bh);
clear_buffer_uptodate(bh);
}
first = folio_buffers(folio);
@ -589,7 +587,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);
@ -1062,6 +1060,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);
}
@ -1070,12 +1069,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 = bh->b_folio->mapping;
if (!mapping)
return;
fscrypt_set_bio_crypt_ctx(bio, mapping->host,
@ -1086,7 +1089,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));
@ -1094,11 +1096,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;
@ -1107,7 +1105,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))
@ -1116,7 +1115,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;
@ -1126,7 +1129,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);
@ -1482,7 +1486,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)
{
@ -2710,7 +2714,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);

View File

@ -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;

View File

@ -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;
}
}

View File

@ -421,7 +421,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");

View File

@ -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;
}

View File

@ -360,7 +360,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;

View File

@ -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);
/*

View File

@ -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
@ -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;

View File

@ -32,14 +32,14 @@
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);
if (success)
clear_buffer_write_io_error(bh);
else
clear_buffer_uptodate(bh);
mark_buffer_write_io_error(bh);
if (orig_bh) {
clear_and_wake_up_bit(BH_Shadow, &orig_bh->b_state);
}
@ -169,7 +169,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() */
@ -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);
@ -834,7 +834,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 +877,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");

View File

@ -328,8 +328,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;
@ -349,24 +347,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
@ -380,8 +385,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);
@ -392,7 +399,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
@ -401,13 +408,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;
@ -882,7 +887,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;
}

View File

@ -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);

View File

@ -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);
}

View File

@ -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");

View File

@ -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);

View File

@ -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,36 @@ 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);
}
/**
* 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 */