buffer: stop clearing BH_Uptodate when a write fails

From: "Matthew Wilcox (Oracle)" <[email protected]>

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

Signed-off-by Matthew Wilcox (Oracle) <[email protected]>

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

---
 include/linux/buffer_head.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

include/linux/buffer_head.h +2 -5

index fd2c7115c054..699970b4bbf2 100644
@@ -59,10 +59,7 @@ struct address_space;
 struct buffer_head {
 	unsigned long b_state;		/* buffer state bitmap (see above) */
 	struct buffer_head *b_this_page;/* circular list of page's buffers */
-	union {
-		struct page *b_page;	/* the page this bh is mapped to */
-		struct folio *b_folio;	/* the folio this bh is mapped to */
-	};
+	struct folio *b_folio;		/* the folio this bh is mapped to */
 
 	sector_t b_blocknr;		/* start block number */
 	size_t b_size;			/* size of mapping */

@@ -172,7 +169,7 @@ static __always_inline int buffer_uptodate(const struct buffer_head *bh)
 
 static inline unsigned long bh_offset(const struct buffer_head *bh)
 {
-	return (unsigned long)(bh)->b_data & (page_size(bh->b_page) - 1);
+	return (unsigned long)(bh)->b_data & (folio_size(bh->b_folio) - 1);
 }
 
 /* If we *know* page->private refers to buffer_heads */

Signature · 1 line

2.43.0

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

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

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

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

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

This is deliberately not a general capability. Buffers over highmem have no permanent kernel virtual address, which is why folio_set_bh() records a folio and an offset instead of an address. A folio-less buffer_head is only valid over memory that is always mapped, and must not be passed to bh_offset().

Suggested-by Matthew Wilcox (Oracle) <[email protected]>

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

---
 fs/buffer.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

fs/buffer.c +13 -4

index be8b57a635cd..04fcc34e4fa6 100644
@@ -1099,12 +1099,16 @@ EXPORT_SYMBOL(__bforget);
 static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,
 				  gfp_t gfp_mask)
 {
-	const struct address_space *mapping = folio_mapping(bh->b_folio);
+	const struct address_space *mapping;
 
 	/*
 	 * The ext4 journal (jbd2) can submit a buffer_head it directly created
-	 * for a non-pagecache page.  fscrypt doesn't care about these.
+	 * for memory that is not in the page cache at all.  fscrypt doesn't
+	 * care about these.
 	 */
+	if (!bh->b_folio)
+		return;
+	mapping = folio_mapping(bh->b_folio);
 	if (!mapping)
 		return;
 	fscrypt_set_bio_crypt_ctx(bio, mapping->host,

@@ -1142,7 +1146,11 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
 	bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
 	bio->bi_write_hint = write_hint;
 
-	bio_add_folio_nofail(bio, bh->b_folio, bh->b_size, bh_offset(bh));
+	if (bh->b_folio)
+		bio_add_folio_nofail(bio, bh->b_folio, bh->b_size,
+				     bh_offset(bh));
+	else
+		bio_add_virt_nofail(bio, bh->b_data, bh->b_size);
 
 	bio->bi_end_io = end_bio;
 	bio->bi_private = bh;

@@ -1152,7 +1160,8 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
 
 	if (wbc) {
 		wbc_init_bio(wbc, bio);
-		wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
+		if (bh->b_folio)
+			wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
 	}
 
 	blk_crypto_submit_bio(bio);

Signature · 1 line

2.43.0

Quoted 19 lines · Chao Shi

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

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

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

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

Quoted 7 lines

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

This is deliberately not a general capability. Buffers over highmem have no permanent kernel virtual address, which is why folio_set_bh() records a folio and an offset instead of an address. A folio-less buffer_head is only valid over memory that is always mapped, and must not be passed to bh_offset().

There are much more things you cannot do with a bh that doesn't have valid b_folio - touch_buffer(), end_buffer_async_read(), ... and many more. But that's a bussiness of the code that sets up such temporary bhs. I agree that setting b_folio to NULL will if nothing else lead to much more obvious failures than when we accidentally get slab folio. So I'd prefer we update the description a bit in this direction but otherwise feel free to add:

Quoted 6 lines

static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,

fs/buffer.c +1 -1

 				  gfp_t gfp_mask)
 {
-	const struct address_space *mapping = folio_mapping(bh->b_folio);
+	const struct address_space *mapping;

fs/buffer.c EXPORT_SYMBOL(__bforget); · line 1113

Quoted 11 lines

fs/buffer.c +5 -1

 	/*
 	 * The ext4 journal (jbd2) can submit a buffer_head it directly created
-	 * for a non-pagecache page.  fscrypt doesn't care about these.
+	 * for memory that is not in the page cache at all.  fscrypt doesn't
+	 * care about these.
 	 */
+	if (!bh->b_folio)
+		return;
+	mapping = folio_mapping(bh->b_folio);
 	if (!mapping)
 		return;

Quoted 28 lines · Jan Kara

Quoted 19 lines · Chao Shi

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

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

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

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

Quoted 7 lines

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

This is deliberately not a general capability. Buffers over highmem have no permanent kernel virtual address, which is why folio_set_bh() records a folio and an offset instead of an address. A folio-less buffer_head is only valid over memory that is always mapped, and must not be passed to bh_offset().

Quoted 6 lines

There are much more things you cannot do with a bh that doesn't have valid b_folio - touch_buffer(), end_buffer_async_read(), ... and many more. But that's a bussiness of the code that sets up such temporary bhs. I agree that setting b_folio to NULL will if nothing else lead to much more obvious failures than when we accidentally get slab folio. So I'd prefer we update the description a bit in this direction but otherwise feel free to add:

Yes, agreed. I think it's fine to add in some 'if (!bh->b_folio)' tests, but only where they're needed for the occasional consumer. This really is a rare (but legitimate) case.

Quoted 6 lines

Quoted 6 lines

static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,

fs/buffer.c +1 -1

 				  gfp_t gfp_mask)
 {
-	const struct address_space *mapping = folio_mapping(bh->b_folio);
+	const struct address_space *mapping;

Quoted 11 lines

fs/buffer.c EXPORT_SYMBOL(__bforget); · line 1113

Quoted 11 lines

fs/buffer.c +5 -1

 	/*
 	 * The ext4 journal (jbd2) can submit a buffer_head it directly created
-	 * for a non-pagecache page.  fscrypt doesn't care about these.
+	 * for memory that is not in the page cache at all.  fscrypt doesn't
+	 * care about these.
 	 */
+	if (!bh->b_folio)
+		return;
+	mapping = folio_mapping(bh->b_folio);
 	if (!mapping)
 		return;

Do we want to call folio_mapping() here? The only case where I can see this being useful is if we attach a buffer_head to an anonymous folio, and I don't see a good reason to do that. I think this can just be

	mapping = bh->b_folio->mapping;

Quoted 3 lines

Yes, agreed. I think it's fine to add in some 'if (!bh->b_folio)' tests, but only where they're needed for the occasional consumer. This really is a rare (but legitimate) case.

Yes. If we were to write this code from scratch, I'd rather push for forming and submitting the bio directly instead of trying to bend the bh layer for these temporary bhs. Eventually we might want to do this cleanup anyway (since bhs don't really buy us much in this path) but not in this patch set.

Quoted 22 lines

Quoted 6 lines

Quoted 6 lines

static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,

fs/buffer.c +1 -1

 				  gfp_t gfp_mask)
 {
-	const struct address_space *mapping = folio_mapping(bh->b_folio);
+	const struct address_space *mapping;

Quoted 11 lines

fs/buffer.c EXPORT_SYMBOL(__bforget); · line 1113

Quoted 11 lines

fs/buffer.c +5 -1

 	/*
 	 * The ext4 journal (jbd2) can submit a buffer_head it directly created
-	 * for a non-pagecache page.  fscrypt doesn't care about these.
+	 * for memory that is not in the page cache at all.  fscrypt doesn't
+	 * care about these.
 	 */
+	if (!bh->b_folio)
+		return;
+	mapping = folio_mapping(bh->b_folio);
 	if (!mapping)
 		return;

Do we want to call folio_mapping() here? The only case where I can see this being useful is if we attach a buffer_head to an anonymous folio, and I don't see a good reason to do that. I think this can just be

Quoted 1 line

	mapping = bh->b_folio->mapping;

For all I know yes, bh->b_folio->mapping should be enough here. But I thought folio_mapping() is kind of preferred to open-coding?

Quoted 9 lines · Jan Kara

Quoted 3 lines

Yes, agreed. I think it's fine to add in some 'if (!bh->b_folio)' tests, but only where they're needed for the occasional consumer. This really is a rare (but legitimate) case.

Yes. If we were to write this code from scratch, I'd rather push for forming and submitting the bio directly instead of trying to bend the bh layer for these temporary bhs. Eventually we might want to do this cleanup anyway (since bhs don't really buy us much in this path) but not in this patch set.

I think that would be better, but it is out of scope.

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 17 lines

static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,
gfp_t gfp_mask)
{

fs/buffer.c +6 -2

-       const struct address_space *mapping = folio_mapping(bh->b_folio);
+       const struct address_space *mapping;
 
        /*
         * The ext4 journal (jbd2) can submit a buffer_head it directly created
-        * for a non-pagecache page.  fscrypt doesn't care about these.
+        * for memory that is not in the page cache at all.  fscrypt doesn't
+        * care about these.
         */
+       if (!bh->b_folio)
+               return;
+       mapping = folio_mapping(bh->b_folio);
        if (!mapping)
                return;

Quoted 8 lines

Quoted 5 lines

Do we want to call folio_mapping() here? The only case where I can see this being useful is if we attach a buffer_head to an anonymous folio, and I don't see a good reason to do that. I think this can just be

    mapping = bh->b_folio->mapping;

For all I know yes, bh->b_folio->mapping should be enough here. But I thought folio_mapping() is kind of preferred to open-coding?

It rather depends on the path we're talking about. Looking at the implementation of folio_mapping(), it's actually mildly dangerous to call folio_mapping() in this path. If we were ever to point a buffer_head at some memory on a page in swap cache, we'd get a pointer to a swap_address_space. I think we'd safely crash in that case because mapping->host would be NULL, but in other contexts doing things to a swap mapping would be quite bad.

So I think this should just be folio->mapping; don't risk getting a pointer to a swap mapping.

Agreed, and done - but not in this patch, which is worth flagging because it is not obvious.

folio_mapping() is doing two jobs here. The one you are objecting to is the swap cache mapping. The other is that it returns NULL for a slab folio, and that is the only reason this path is safe for jbd2's shadow buffers today - they are the slab-backed buffers this series is getting rid of, and they do reach buffer_set_crypto_ctx() through __bh_submit().

So doing it in this patch would leave one commit where shadow buffers still sit on slab folios, and this code reads folio->mapping directly, i.e., hands fscrypt a struct kmem_cache and dereferences ->host on it. Fine at the end of the series, broken in the middle of it.

v2 does it as its own patch 4, after the jbd2 change.

Chao

Quoted 51 lines · Matthew Wilcox

Quoted 9 lines · Jan Kara

Quoted 3 lines

Yes, agreed. I think it's fine to add in some 'if (!bh->b_folio)' tests, but only where they're needed for the occasional consumer. This really is a rare (but legitimate) case.

Yes. If we were to write this code from scratch, I'd rather push for forming and submitting the bio directly instead of trying to bend the bh layer for these temporary bhs. Eventually we might want to do this cleanup anyway (since bhs don't really buy us much in this path) but not in this patch set.

I think that would be better, but it is out of scope.

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 17 lines

static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,
gfp_t gfp_mask)
{

fs/buffer.c +6 -2

-       const struct address_space *mapping = folio_mapping(bh->b_folio);
+       const struct address_space *mapping;
 
        /*
         * The ext4 journal (jbd2) can submit a buffer_head it directly created
-        * for a non-pagecache page.  fscrypt doesn't care about these.
+        * for memory that is not in the page cache at all.  fscrypt doesn't
+        * care about these.
         */
+       if (!bh->b_folio)
+               return;
+       mapping = folio_mapping(bh->b_folio);
        if (!mapping)
                return;

Quoted 8 lines

Quoted 5 lines

Do we want to call folio_mapping() here? The only case where I can see this being useful is if we attach a buffer_head to an anonymous folio, and I don't see a good reason to do that. I think this can just be

    mapping = bh->b_folio->mapping;

For all I know yes, bh->b_folio->mapping should be enough here. But I thought folio_mapping() is kind of preferred to open-coding?

It rather depends on the path we're talking about. Looking at the implementation of folio_mapping(), it's actually mildly dangerous to call folio_mapping() in this path. If we were ever to point a buffer_head at some memory on a page in swap cache, we'd get a pointer to a swap_address_space. I think we'd safely crash in that case because mapping->host would be NULL, but in other contexts doing things to a swap mapping would be quite bad.

So I think this should just be folio->mapping; don't risk getting a pointer to a swap mapping.

Quoted 19 lines · Chao Shi

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

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

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

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

Quoted 7 lines

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

This is deliberately not a general capability. Buffers over highmem have no permanent kernel virtual address, which is why folio_set_bh() records a folio and an offset instead of an address. A folio-less buffer_head is only valid over memory that is always mapped, and must not be passed to bh_offset().

There are much more things you cannot do with a bh that doesn't have valid b_folio - touch_buffer(), end_buffer_async_read(), ... and many more. But that's a bussiness of the code that sets up such temporary bhs. I agree that setting b_folio to NULL will if nothing else lead to much more obvious failures than when we accidentally get slab folio. So I'd prefer we update the description a bit in this direction but otherwise feel free to add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 54 lines

Suggested-by Matthew Wilcox (Oracle) <[email protected]>

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

---
 fs/buffer.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

fs/buffer.c +13 -5

index be8b57a635cd..04fcc34e4fa6 100644
@@ -1099,12 +1099,16 @@ EXPORT_SYMBOL(__bforget);
 static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,
                                gfp_t gfp_mask)
 {
-     const struct address_space *mapping = folio_mapping(bh->b_folio);
+     const struct address_space *mapping;
 
      /*
       * The ext4 journal (jbd2) can submit a buffer_head it directly created
-      * for a non-pagecache page.  fscrypt doesn't care about these.
+      * for memory that is not in the page cache at all.  fscrypt doesn't
+      * care about these.
       */
+     if (!bh->b_folio)
+             return;
+     mapping = folio_mapping(bh->b_folio);
      if (!mapping)
              return;
      fscrypt_set_bio_crypt_ctx(bio, mapping->host,

@@ -1142,7 +1146,11 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
      bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
      bio->bi_write_hint = write_hint;
 
-     bio_add_folio_nofail(bio, bh->b_folio, bh->b_size, bh_offset(bh));
+     if (bh->b_folio)
+             bio_add_folio_nofail(bio, bh->b_folio, bh->b_size,
+                                  bh_offset(bh));
+     else
+             bio_add_virt_nofail(bio, bh->b_data, bh->b_size);
 
      bio->bi_end_io = end_bio;
      bio->bi_private = bh;

@@ -1152,7 +1160,8 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
 
      if (wbc) {
              wbc_init_bio(wbc, bio);
-             wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
+             if (bh->b_folio)
+                     wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
      }
 
      blk_crypto_submit_bio(bio);
--

Quoted 2 lines

2.43.0

--
Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 4 lines · Chao Shi

fs/jbd2/commit.c +0 -0

@@ -330,6 +330,8 @@ static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
 	char *addr;
 	__u32 checksum;
 

fs/jbd2/commit.c static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh) · line 337

Quoted 5 lines

fs/jbd2/commit.c +2 -0

+	if (!bh->b_folio)
+		return crc32_be(crc32_sum, bh->b_data, bh->b_size);
 	addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
 	checksum = crc32_be(crc32_sum, addr, bh->b_size);
 	kunmap_local(addr);

This is awkward. How about ...

static inline void *kmap_local_bh(const struct buffer_head *bh)

{
	if (bh->b_folio)
		return kmap_local_folio(bh->b_folio, bh_offset(bh));
	return kmap_local_page(virt_to_page(bh->b_data);
}

(this is also somewhat awkward because it feels like we could just return bh->b_data, but kunmap_local_indexed() does some ... stuff)

Anyway, it all gets optimised away on non-HIGHMEM. Or if it doesn't, you can force it to ;-)

Quoted 5 lines · Matthew Wilcox

Quoted 4 lines · Chao Shi

fs/jbd2/commit.c +0 -0

@@ -330,6 +330,8 @@ static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
    char *addr;
    __u32 checksum;
 

Quoted 7 lines

fs/jbd2/commit.c static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh) · line 337

Quoted 5 lines

fs/jbd2/commit.c +2 -0

+   if (!bh->b_folio)
+           return crc32_be(crc32_sum, bh->b_data, bh->b_size);
    addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
    checksum = crc32_be(crc32_sum, addr, bh->b_size);
    kunmap_local(addr);

This is awkward. How about ...

... Oh. It's not just awkward. The current code is actually wrong. If one uses ext4 on a bs>PS device and one has CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP enabled, it'll only map one page of the buffer and we should get a crash when trying to checksum the entire block.

Supporting mapping multiple pages from a folio simultaneously is something we haven't figured out how to support. Since HIGHMEM is dying (see Arnd's recent work), I'm not inclined to spend effort on it.

We probably just need to make ext4 depend on !DEBUG_KMAP_LOCAL_FORCE_MAP or something?

Quoted 14 lines

static inline void *kmap_local_bh(const struct buffer_head *bh)

{
      if (bh->b_folio)
              return kmap_local_folio(bh->b_folio, bh_offset(bh));
      return kmap_local_page(virt_to_page(bh->b_data);
}

(this is also somewhat awkward because it feels like we could just return bh->b_data, but kunmap_local_indexed() does some ... stuff)

Anyway, it all gets optimised away on non-HIGHMEM. Or if it doesn't, you can force it to ;-)

Thank you so much.
Taken, with a change: the helper needs a matching unmap, and must not map the folio-less case at all.

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

static inline void kunmap_local_bh(const struct buffer_head *bh, void *addr) {
if (bh->b_folio)
kunmap_local(addr);
}

Two reasons. kmap_local_page(virt_to_page(bh->b_data)) loses the offset within the page, and b_frozen_data is kmalloc(b_size), which is only guaranteed page aligned when the block size is at least PAGE_SIZE.

The second one is your other point. With CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP, __kmap_local_page_prot() maps even a lowmem page, one page at a time. So mapping memory that is already permanently mapped would walk this path straight into the bs>PS problem you just described, which it is otherwise immune to - b_data covers the whole block whatever its size. Returning b_data and calling kunmap_local() on it anyway is not an option either:
kunmap_local_indexed() WARNs on an address outside the fixmap range under that config. Hence the pair.

On the bs>PS folio path itself: agreed it is real and predates this series. I would rather not fold a fix into this one, but I will take it - I'll look at it separately once this has settled.

Quoted 11 lines · Chris S

Thank you so much.
Taken, with a change: the helper needs a matching unmap, and must not map the folio-less case at all.

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

Quoted 5 lines

static inline void kunmap_local_bh(const struct buffer_head *bh, void *addr) {
if (bh->b_folio)
kunmap_local(addr);
}

Ah, I hadn't considered that option. Yes, I think this is the right approach. Thanks!

Quoted 3 lines

Two reasons. kmap_local_page(virt_to_page(bh->b_data)) loses the offset within the page, and b_frozen_data is kmalloc(b_size), which is only guaranteed page aligned when the block size is at least PAGE_SIZE.

Yeah; my bad. I should have added in PAGE_MASK(bh->b_data) to that, but what you have is better.

Quoted 9 lines

The second one is your other point. With CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP, __kmap_local_page_prot() maps even a lowmem page, one page at a time. So mapping memory that is already permanently mapped would walk this path straight into the bs>PS problem you just described, which it is otherwise immune to - b_data covers the whole block whatever its size. Returning b_data and calling kunmap_local() on it anyway is not an option either:
kunmap_local_indexed() WARNs on an address outside the fixmap range under that config. Hence the pair.

Quoted 3 lines

On the bs>PS folio path itself: agreed it is real and predates this series. I would rather not fold a fix into this one, but I will take it - I'll look at it separately once this has settled.

I've been considering adding a folio_vmap() which would be a nop for !highmem, kmap_local() for order-0 highmem folios and vmap() for large highmem folios. It's not reached the top of my todo list yet.

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 use folio_mapping() to avoid tripping over 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 and set b_data. The previous patch taught fs/buffer.c to submit such a buffer. The two commit-path checksum helpers are the only other users of the shadow buffer's contents, and they take the data directly rather than kmapping a folio that is already mapped.

Note that the shadow buffer must not be passed to bh_offset() while b_folio is NULL. All four callers that can see one are handled here and in the previous patch.

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]>

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

fs/jbd2/journal.c +4 -5

index 09efa337649e..9e4cb04587b4 100644
@@ -329,6 +329,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	struct buffer_head *new_bh;
 	struct folio *new_folio;
 	unsigned int new_offset;
+	bool frozen = false;
 	struct buffer_head *bh_in = jh2bh(jh_in);
 	journal_t *journal = transaction->t_journal;
 

@@ -354,8 +355,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
 	 * 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);
+		frozen = true;
 		do_escape = jbd2_data_needs_escaping(jh_in->b_frozen_data);
 		if (do_escape)
 			jbd2_data_do_escape(jh_in->b_frozen_data);

@@ -400,13 +400,22 @@ 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);
+		frozen = true;
 		jbd2_data_do_escape(jh_in->b_frozen_data);
 	}
 
 escape_done:
-	folio_set_bh(new_bh, new_folio, new_offset);
+	if (frozen) {

Quoted 30 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 use folio_mapping() to avoid tripping over 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 and set b_data. The previous patch taught fs/buffer.c to submit such a buffer. The two commit-path checksum helpers are the only other users of the shadow buffer's contents, and they take the data directly rather than kmapping a folio that is already mapped.

Note that the shadow buffer must not be passed to bh_offset() while b_folio is NULL. All four callers that can see one are handled here and in the previous patch.

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]>

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

...

Quoted 35 lines

fs/jbd2/journal.c +4 -5

index 09efa337649e..9e4cb04587b4 100644
@@ -329,6 +329,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
      struct buffer_head *new_bh;
      struct folio *new_folio;
      unsigned int new_offset;
+     bool frozen = false;
      struct buffer_head *bh_in = jh2bh(jh_in);
      journal_t *journal = transaction->t_journal;
 

@@ -354,8 +355,7 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
       * 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);
+             frozen = true;
              do_escape = jbd2_data_needs_escaping(jh_in->b_frozen_data);
              if (do_escape)
                      jbd2_data_do_escape(jh_in->b_frozen_data);

@@ -400,13 +400,22 @@ 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);
+             frozen = true;
              jbd2_data_do_escape(jh_in->b_frozen_data);
      }
 
 escape_done:
-     folio_set_bh(new_bh, new_folio, new_offset);
+     if (frozen) {

fs/jbd2/journal.c int jbd2_journal_write_metadata_buffer(transaction_t *transaction, · line 418

Quoted 10 lines

fs/jbd2/journal.c +10 -0

+             /*
+              * b_frozen_data is slab memory, not page cache.  Point the
+              * buffer at it directly rather than at a slab folio, whose
+              * ->mapping is not an address_space.
+              */
+             new_bh->b_folio = NULL;
+             new_bh->b_data = jh_in->b_frozen_data;
+     } else {
+             folio_set_bh(new_bh, new_folio, new_offset);
+     }

Can we please simplify the flow in this function? folio_set_bh() is now done only for a single branch in this function. You can move it there and completely avoid new_folio, new_offset, and frozen variables (you just need to set new_bh->b_data in the two branches where frozen data is used).

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

BH_Write_EIO records that the last write of this buffer failed. It is cleared when the buffer is submitted for write again - see the test_set_buffer_req() check in __bh_submit() - but a filesystem freeing a metadata block never submits it again. It calls bforget() and hands the block back to the allocator, so the flag outlives the block it refers to.

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

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

Suggested-by Jan Kara <[email protected]>

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

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

fs/buffer.c +1 -0

index 04fcc34e4fa6..7889c30d8715 100644
@@ -1091,6 +1091,7 @@ EXPORT_SYMBOL(__brelse);
 void __bforget(struct buffer_head *bh)
 {
 	clear_buffer_dirty(bh);
+	clear_buffer_write_io_error(bh);
 	remove_assoc_queue(bh);
 	__brelse(bh);
 }

Signature · 1 line

2.43.0

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

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

Suggested-by Jan Kara <[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 7889c30d8715..50a63d964815 100644
@@ -1518,7 +1518,7 @@ EXPORT_SYMBOL(folio_set_bh);
 /* Bits that are cleared during an invalidate */
 #define BUFFER_FLAGS_DISCARD \
 	(1 << BH_Mapped | 1 << BH_New | 1 << BH_Req | \
-	 1 << BH_Delay | 1 << BH_Unwritten)
+	 1 << BH_Delay | 1 << BH_Unwritten | 1 << BH_Write_EIO)
 
 static void discard_buffer(struct buffer_head * bh)
 {

Signature · 1 line

2.43.0

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

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

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

In __sync_dirty_buffer() the flag also refers unambiguously to the write we just issued, because __bh_submit() clears it when it resubmits a buffer for write.

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

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

fs/buffer.c +2 -2

index 50a63d964815..ac978d9090c2 100644
@@ -618,7 +618,7 @@ int mmb_sync(struct mapping_metadata_bhs *mmb)
 		}
 		spin_unlock(&mmb->lock);
 		wait_on_buffer(bh);
-		if (!buffer_uptodate(bh))
+		if (buffer_write_io_error(bh))
 			err = -EIO;
 		brelse(bh);
 		spin_lock(&mmb->lock);

@@ -2743,7 +2743,7 @@ int __sync_dirty_buffer(struct buffer_head *bh, blk_opf_t op_flags)
 
 		bh_submit(bh, REQ_OP_WRITE | op_flags, bh_end_write);
 		wait_on_buffer(bh);
-		if (!buffer_uptodate(bh))
+		if (buffer_write_io_error(bh))
 			return -EIO;
 	} else {
 		unlock_buffer(bh);

Signature · 1 line

2.43.0

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

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

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

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

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

fs/ext2/xattr.c +1 -1

index be63f89402a3..39005ec23fe5 100644
@@ -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;
 		}
 	}

Signature · 1 line

2.43.0

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

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

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

Note that BH_Write_EIO stays set until the buffer is written again, forgotten or invalidated, so an unrepaired itable block now reports on every subsequent sync of that inode rather than only on the write that failed. That is the intended behaviour and matches what ocfs2 has always done with this flag.

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

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

---
 fs/ext4/ext4_jbd2.c | 2 +-
 fs/ext4/mmp.c       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

fs/ext4/ext4_jbd2.c +1 -1

index 02b066299164..f338d6e3c29f 100644
@@ -413,7 +413,7 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
 		}
 		if (inode && inode_needs_sync(inode)) {
 			sync_dirty_buffer(bh);
-			if (buffer_req(bh) && !buffer_uptodate(bh)) {
+			if (buffer_write_io_error(bh)) {
 				ext4_error_inode_err(inode, where, line,
 						     bh->b_blocknr, EIO,
 					"IO error syncing itable block");

fs/ext4/mmp.c +1 -1

index 7ce361484b38..4b18ddef468d 100644
@@ -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;
 }

Signature · 1 line

2.43.0

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

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

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

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

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

---
 fs/ocfs2/buffer_head_io.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

fs/ocfs2/buffer_head_io.c +7 -5

index 7bfe377af2df..733ceda79ca1 100644
@@ -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);
 	}

Signature · 1 line

2.43.0

Quoted 17 lines · Chao Shi

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

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

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

Quoted 5 lines

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.

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

This looks ok to me so feel free to add:

Reviewed-by Jan Kara <[email protected]>

but it would be good to get a feedback from ocfs2 maintainers whether the logic change in ocfs2_write_block() where we now keep the cluster cache block uptodate after write IO error doesn't break some expectations of ocfs2.

Honza

Quoted 37 lines

---
 fs/ocfs2/buffer_head_io.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

fs/ocfs2/buffer_head_io.c +7 -6

index 7bfe377af2df..733ceda79ca1 100644
@@ -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);
      }
--

Quoted 2 lines

2.43.0

--
Jan Kara <[email protected]>
SUSE Labs, CR

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

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

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

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

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

Suggested-by Jan Kara <[email protected]>

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

---
 fs/ocfs2/journal.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

fs/ocfs2/journal.c +13 -12

index d8afbc1a76bb..ea6802d894c2 100644
@@ -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");

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

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

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

fs/jbd2/commit.c +0 -0

index 60273cddf434..8912786ebebb 100644

fs/jbd2/commit.c +3 -3

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

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

@@ -883,7 +883,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

Quoted 36 lines · Chao Shi

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

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

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

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

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

fs/jbd2/commit.c +0 -0

index 60273cddf434..8912786ebebb 100644

fs/jbd2/commit.c static void journal_end_buffer_io_sync(struct bio *bio) · line 42

Quoted 5 lines

fs/jbd2/commit.c +1 -1

@@ -39,7 +39,7 @@ static void journal_end_buffer_io_sync(struct bio *bio)
 	if (uptodate)
 		set_buffer_uptodate(bh);
 	else
-		clear_buffer_uptodate(bh);
+		mark_buffer_write_io_error(bh);

There's no need to set uptodate flag here at all (all callers set it before submitting IO). So you can just simplify this to:

	if (!uptodate)
		mark_buffer_write_io_error(bh);

Also I'd rename the local variable from 'uptodate' to 'ioerr' or something like that.

Quoted 46 lines · Jan Kara

Quoted 36 lines · Chao Shi

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

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

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

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

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

fs/jbd2/commit.c +0 -0

index 60273cddf434..8912786ebebb 100644

fs/jbd2/commit.c static void journal_end_buffer_io_sync(struct bio *bio) · line 42

Quoted 5 lines

fs/jbd2/commit.c +1 -1

@@ -39,7 +39,7 @@ static void journal_end_buffer_io_sync(struct bio *bio)
    if (uptodate)
            set_buffer_uptodate(bh);
    else
-           clear_buffer_uptodate(bh);
+           mark_buffer_write_io_error(bh);

There's no need to set uptodate flag here at all (all callers set it before submitting IO). So you can just simplify this to:

Quoted 5 lines

      if (!uptodate)
              mark_buffer_write_io_error(bh);

Also I'd rename the local variable from 'uptodate' to 'ioerr' or something like that.

Sorry, not 'ioerr' but rather 'success'. I can see fs/buffer.c names it like that.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

jbd2_freeze_jh_data() asserts that the buffer it is about to copy out is up to date. Once this series stops clearing BH_Uptodate on write error, that assertion stops firing for the case it was written for, because a buffer whose write failed stays up to date - which is the point: the in-memory copy being frozen is still the data the filesystem wants written.

Assert on the condition that still means something went wrong. The message loses its "Possible", because BH_Write_EIO is not a guess.

Suggested-by Jan Kara <[email protected]>

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

fs/jbd2/transaction.c +0 -0

 	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

Quoted 11 lines · Chao Shi

jbd2_freeze_jh_data() asserts that the buffer it is about to copy out is up to date. Once this series stops clearing BH_Uptodate on write error, that assertion stops firing for the case it was written for, because a buffer whose write failed stays up to date - which is the point: the in-memory copy being frozen is still the data the filesystem wants written.

Assert on the condition that still means something went wrong. The message loses its "Possible", because BH_Write_EIO is not a guess.

Suggested-by Jan Kara <[email protected]>

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

...

fs/jbd2/transaction.c static void jbd2_freeze_jh_data(struct journal_head *jh) · line 923

Quoted 10 lines

fs/jbd2/transaction.c +1 -1

index 5cc7d097b2ac..fdabe34a7e22 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_write_io_error(bh), "IO failure.\n");

After some more thought I think getting here a buffer_write_io_error() buffer is actually OK and we'll rewrite it. Instead this assert should just have a different message:

        J_EXPECT_JH(jh, buffer_uptodate(bh), "Buffer not uptodate!\n");

to verify the buffer has valid data.

Honza

Quoted 6 lines

fs/jbd2/transaction.c +0 -1

      source = kmap_local_folio(bh->b_folio, bh_offset(bh));
      /* Fire data frozen trigger just before we copy the data */
      jbd2_buffer_frozen_trigger(jh, source, jh->b_triggers);
--

2.43.0

--
Jan Kara <[email protected]>
SUSE Labs, CR

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.

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.

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

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

fs/ext4/fast_commit.c +0 -0

index 8e2259799614..8cd60c9f2272 100644

fs/ext4/fast_commit.c +0 -0

 	unlock_buffer(bh);

fs/jbd2/journal.c +1 -1

index 9e4cb04587b4..ffe7a22e6699 100644
@@ -890,7 +890,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

Quoted 22 lines · Chao Shi

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

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.

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

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

fs/ext4/fast_commit.c +0 -0

index 8e2259799614..8cd60c9f2272 100644

fs/ext4/fast_commit.c static void ext4_end_buffer_io_sync(struct bio *bio) · line 217

Quoted 6 lines

fs/ext4/fast_commit.c +1 -1

@@ -213,7 +213,7 @@ static void ext4_end_buffer_io_sync(struct bio *bio)
      } else {
              ext4_debug("%s: Block %lld not up-to-date",
                         __func__, bh->b_blocknr);
-             clear_buffer_uptodate(bh);
+             mark_buffer_write_io_error(bh);
      }

Again, this is only ever used for writes and buffer is guaranteed to be uptodate so please rename 'uptodate' to 'ioerr' and remove the setting of uptodate bit as well. And the ext4_debug() messages need slight update as well.

Honza

Quoted 16 lines

fs/ext4/fast_commit.c +0 -0

      unlock_buffer(bh);

fs/jbd2/journal.c +1 -2

index 9e4cb04587b4..ffe7a22e6699 100644
@@ -890,7 +890,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;
              }
--

Quoted 2 lines

2.43.0

--
Jan Kara <[email protected]>
SUSE Labs, CR

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, so the clears can go.

Found by FuzzNvme.

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

---
 fs/buffer.c | 2 --
 1 file changed, 2 deletions(-)

fs/buffer.c +0 -1

index ac978d9090c2..0002f0736398 100644
@@ -207,7 +207,6 @@ void bh_end_write(struct bio *bio)
 	} else {
 		buffer_io_error(bh, ", lost sync page write");
 		mark_buffer_write_io_error(bh);
-		clear_buffer_uptodate(bh);
 	}
 	unlock_buffer(bh);
 }

Quoted 45 lines · Chao Shi

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

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

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

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

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

Found by FuzzNvme.

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

---
 fs/buffer.c | 2 --
 1 file changed, 2 deletions(-)

fs/buffer.c +0 -1

index ac978d9090c2..0002f0736398 100644
@@ -207,7 +207,6 @@ void bh_end_write(struct bio *bio)
      } else {
              buffer_io_error(bh, ", lost sync page write");
              mark_buffer_write_io_error(bh);
-             clear_buffer_uptodate(bh);
      }
      unlock_buffer(bh);
 }

fs/buffer.c void bh_end_async_write(struct bio *bio) · line 445

Quoted 7 lines

fs/buffer.c +0 -1

@@ -441,7 +440,6 @@ void bh_end_async_write(struct bio *bio)
      } else {
              buffer_io_error(bh, ", lost async page write");
              mark_buffer_write_io_error(bh);
-             clear_buffer_uptodate(bh);
      }
 
      first = folio_buffers(folio);

Please remove setting of the uptodate flag from these functions as well. Write IO completion shouldn't really touch the uptodate flag at all...

Hum, maybe we should actually clear the BH_Write_EIO flag on successful write completion in all the end io handlers. And then we can drop the clearing from __bh_submit(). And this will actually also address the concern Sahiko raised (mostly harmless but still) that if the user gets to check BH_Write_EIO after another process managed to redirty the buffer and resubmit it for IO, it can miss detecting IO error that happened.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

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, so the clears can go.

Found by FuzzNvme.

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

---
 fs/buffer.c | 2 --
 1 file changed, 2 deletions(-)

fs/buffer.c +0 -2

index ac978d9090c2..0002f0736398 100644
@@ -207,7 +207,6 @@ void bh_end_write(struct bio *bio)
 	} else {
 		buffer_io_error(bh, ", lost sync page write");
 		mark_buffer_write_io_error(bh);
-		clear_buffer_uptodate(bh);
 	}
 	unlock_buffer(bh);
 }

@@ -441,7 +440,6 @@ void bh_end_async_write(struct bio *bio)
 	} else {
 		buffer_io_error(bh, ", lost async page write");
 		mark_buffer_write_io_error(bh);
-		clear_buffer_uptodate(bh);
 	}
 
 	first = folio_buffers(folio);

Quoted 27 lines · Chao Shi

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

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

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

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

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

Found by FuzzNvme.

Quoted 1 line

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

It looks like after this series, we may need to revisit the necessity of ext4_buffer_uptodate() in the ext4 filesystem. IIRC, it was originally introduced as a compromise to address this issue. Now that we no longer clear the buffer uptodate flag after a failed IO write, this flag will still remain valid when getting/reading bh. Therefore, the necessity of ext4_buffer_uptodate() is worth reconsidering.

Quoted 24 lines

---
 fs/buffer.c | 2 --
 1 file changed, 2 deletions(-)

fs/buffer.c +0 -2

index ac978d9090c2..0002f0736398 100644
@@ -207,7 +207,6 @@ void bh_end_write(struct bio *bio)
 	} else {
 		buffer_io_error(bh, ", lost sync page write");
 		mark_buffer_write_io_error(bh);
-		clear_buffer_uptodate(bh);
 	}
 	unlock_buffer(bh);
 }

@@ -441,7 +440,6 @@ void bh_end_async_write(struct bio *bio)
 	} else {
 		buffer_io_error(bh, ", lost async page write");
 		mark_buffer_write_io_error(bh);
-		clear_buffer_uptodate(bh);
 	}
 
 	first = folio_buffers(folio);

Quoted 28 lines · Zhang Yi

Quoted 27 lines · Chao Shi

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

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

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

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

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

Found by FuzzNvme.

Quoted 8 lines

Quoted 1 line

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

It looks like after this series, we may need to revisit the necessity of ext4_buffer_uptodate() in the ext4 filesystem. IIRC, it was originally introduced as a compromise to address this issue. Now that we no longer clear the buffer uptodate flag after a failed IO write, this flag will still remain valid when getting/reading bh. Therefore, the necessity of ext4_buffer_uptodate() is worth reconsidering.

Deninitely :) There are several hacks in ext4 (and other filesystems as well) that could be removed after this patch set. But let's settle on the generic changes first.

Honza

Quoted 26 lines

Quoted 25 lines

---
 fs/buffer.c | 2 --
 1 file changed, 2 deletions(-)

fs/buffer.c +0 -2

index ac978d9090c2..0002f0736398 100644
@@ -207,7 +207,6 @@ void bh_end_write(struct bio *bio)
 	} else {
 		buffer_io_error(bh, ", lost sync page write");
 		mark_buffer_write_io_error(bh);
-		clear_buffer_uptodate(bh);
 	}
 	unlock_buffer(bh);
 }

@@ -441,7 +440,6 @@ void bh_end_async_write(struct bio *bio)
 	} else {
 		buffer_io_error(bh, ", lost async page write");
 		mark_buffer_write_io_error(bh);
-		clear_buffer_uptodate(bh);
 	}
 
 	first = folio_buffers(folio);