[PATCH] btrfs: fix a check-integrity warning on write caching disabled disk

[PATCH] btrfs: fix a check-integrity warning on write caching disabled disk

[wangyugui]

xfstest/btrfs/220 tigger a check-integrity warning
  btrfs: attempt to write superblock which references block which is not flushed
    out of disk's write cache (block flush_gen=1, dev->flush_gen=0)!
  WARNING: at fs/btrfs/check-integrity.c:2196
    btrfsic_process_written_superblock+0x22a/0x2a0 [btrfs]
when
1) CONFIG_BTRFS_FS_CHECK_INTEGRITY=y
2) on a disk with WCE=0

When a disk has write caching disabled, we skip submission of a bio
with flush and sync requests before writing the superblock, since
it's not needed. However when the integrity checker is enabled,
this results in reports that there are metadata blocks referred
by a superblock that were not properly flushed. So don't skip the
bio submission only when the integrity checker is enabled
for the sake of simplicity, since this is a debug tool and
not meant for use in non-debug builds.

Signed-off-by: wangyugui <wangyugui@e16-tech.com>
---
Changes since v1:
- update the changlog/code comment. (Filipe Manana)
- var(struct request_queue *q) is only needed when
   !CONFIG_BTRFS_FS_CHECK_INTEGRITY
---
 fs/btrfs/disk-io.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 355ea88d5c5f..4ef06d0555b0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3968,11 +3968,23 @@ static void btrfs_end_empty_barrier(struct bio *bio)
  */
 static void write_dev_flush(struct btrfs_device *device)
 {
-	struct request_queue *q = bdev_get_queue(device->bdev);
 	struct bio *bio = device->flush_bio;
 
+	#ifndef CONFIG_BTRFS_FS_CHECK_INTEGRITY
+	/*
+	* When a disk has write caching disabled, we skip submission of a bio
+	* with flush and sync requests before writing the superblock, since
+	* it's not needed. However when the integrity checker is enabled,
+	* this results in reports that there are metadata blocks referred
+	* by a superblock that were not properly flushed. So don't skip the
+	* bio submission only when the integrity checker is enabled
+	* for the sake of simplicity, since this is a debug tool and
+	* not meant for use in non-debug builds.
+	*/
+	struct request_queue *q = bdev_get_queue(device->bdev);
 	if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
 		return;
+	#endif
 
 	bio_reset(bio);
 	bio->bi_end_io = btrfs_end_empty_barrier;
-- 
2.32.0

Re: [PATCH] btrfs: fix a check-integrity warning on write caching disabled disk

[Filipe Manana]

On Sun, Oct 24, 2021 at 4:40 AM wangyugui <wangyugui@e16-tech.com> wrote:
>
> xfstest/btrfs/220 tigger a check-integrity warning
>   btrfs: attempt to write superblock which references block which is not flushed
>     out of disk's write cache (block flush_gen=1, dev->flush_gen=0)!
>   WARNING: at fs/btrfs/check-integrity.c:2196
>     btrfsic_process_written_superblock+0x22a/0x2a0 [btrfs]

For stack traces it's best to paste the full version, not just the
first line or two, and
for readability you can skip the reformatting to make it fit within 75
character wide columns.
This is on the borderline of subjectivity / personal preference, so
take it only as a note for
future submissions.

Also a blank line before and after the stack trace helps with readability.

> when
> 1) CONFIG_BTRFS_FS_CHECK_INTEGRITY=y
> 2) on a disk with WCE=0
>
> When a disk has write caching disabled, we skip submission of a bio
> with flush and sync requests before writing the superblock, since
> it's not needed. However when the integrity checker is enabled,
> this results in reports that there are metadata blocks referred
> by a superblock that were not properly flushed. So don't skip the
> bio submission only when the integrity checker is enabled
> for the sake of simplicity, since this is a debug tool and
> not meant for use in non-debug builds.
>
> Signed-off-by: wangyugui <wangyugui@e16-tech.com>

Looks good, thanks for doing it.

Reviewed-by: Filipe Manana <fdmanana@suse.com>



> ---
> Changes since v1:
> - update the changlog/code comment. (Filipe Manana)
> - var(struct request_queue *q) is only needed when
>    !CONFIG_BTRFS_FS_CHECK_INTEGRITY
> ---
>  fs/btrfs/disk-io.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 355ea88d5c5f..4ef06d0555b0 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3968,11 +3968,23 @@ static void btrfs_end_empty_barrier(struct bio *bio)
>   */
>  static void write_dev_flush(struct btrfs_device *device)
>  {
> -       struct request_queue *q = bdev_get_queue(device->bdev);
>         struct bio *bio = device->flush_bio;
>
> +       #ifndef CONFIG_BTRFS_FS_CHECK_INTEGRITY
> +       /*
> +       * When a disk has write caching disabled, we skip submission of a bio
> +       * with flush and sync requests before writing the superblock, since
> +       * it's not needed. However when the integrity checker is enabled,
> +       * this results in reports that there are metadata blocks referred
> +       * by a superblock that were not properly flushed. So don't skip the
> +       * bio submission only when the integrity checker is enabled
> +       * for the sake of simplicity, since this is a debug tool and
> +       * not meant for use in non-debug builds.
> +       */
> +       struct request_queue *q = bdev_get_queue(device->bdev);
>         if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
>                 return;
> +       #endif
>
>         bio_reset(bio);
>         bio->bi_end_io = btrfs_end_empty_barrier;
> --
> 2.32.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”