Re: new scrub code vs zoned file systems

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/5/31 22:04, Johannes Thumshirn wrote:
> On 31.05.23 15:31, Christoph Hellwig wrote:
>> On Wed, May 31, 2023 at 01:25:14PM +0000, Johannes Thumshirn wrote:
>>> Hmm at least flush_scrub_stripes() should not go into the simple write
>>> path at all:
>>
>> Except for the dev-replace case, which seems to trigger this
>> write.
>>
>
> Heh and this has never actually worked IMHO.
>
> I did a crude hack to bandaid scrub:
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index d7d8faf1978a..b20115bd0675 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -1709,9 +1709,20 @@ static int flush_scrub_stripes(struct scrub_ctx *sctx)
>
>                          ASSERT(stripe->dev == fs_info->dev_replace.srcdev);
>
> -                       bitmap_andnot(&good, &stripe->extent_sector_bitmap,
> -                                     &stripe->error_bitmap, stripe->nr_sectors);
> -                       scrub_write_sectors(sctx, stripe, good, true);
> +                       if (btrfs_is_zoned(fs_info)) {
> +                               if (!bitmap_empty(&stripe->extent_sector_bitmap,
> +                                                 stripe->nr_sectors)) {
> +                                       btrfs_repair_one_zone(fs_info,
> +                                                             sctx->stripes[0].bg->start);
> +                                       break;

This doesn't look good, is this a hack to use repair to do the dev-replace?

> +                               }
> +                       } else {
> +                               bitmap_andnot(&good,
> +                                             &stripe->extent_sector_bitmap,
> +                                             &stripe->error_bitmap,
> +                                             stripe->nr_sectors);
> +                               scrub_write_sectors(sctx, stripe, good, true);
> +                       }
>                  }
>          }
>
>
>
> But then it doesn't work as well because:
>
> static int relocating_repair_kthread(void *data)
> {
> 	[...]
>          sb_start_write(fs_info->sb);
>          if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_BALANCE)) {
>                  btrfs_info(fs_info,
>                             "zoned: skip relocating block group %llu to repair: EBUSY",
>                             target);
>                  sb_end_write(fs_info->sb);
>                  return -EBUSY;
>
> That will always fail, because in the case of dev-replace we already have
> BTRFS_EXCLOP_DEV_REPLACE set.
>
> I've just spotted btrfs_exclop_start_try_lock(), that could solve our problem
> here.

To me, the problem can be solved in a much simpler way, if it's
dev-replace for zoned device, let's write the whole stripe to the target
device, and wait for it.

For the btrfs_record_physical_zoned(), we can skip the OE things if
bbio::inode is NULL.

Would the following change solves the problem?

Thanks,
Qu

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index d7d8faf1978a..3fa480cd905e 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -1709,8 +1709,15 @@ static int flush_scrub_stripes(struct scrub_ctx
*sctx)

                         ASSERT(stripe->dev == fs_info->dev_replace.srcdev);

-                       bitmap_andnot(&good, &stripe->extent_sector_bitmap,
-                                     &stripe->error_bitmap,
stripe->nr_sectors);
+                       if (btrfs_is_zoned(fs_info))
+                               /*
+                                * For zoned case, we need to write the
whole
+                                * stripe back, no gaps allowed.
+                                */
+                               bitmap_set(&good, 0, stripe->nr_sectors);
+                       else
+                               bitmap_andnot(&good,
&stripe->extent_sector_bitmap,
+                                             &stripe->error_bitmap,
stripe->nr_sectors);
                         scrub_write_sectors(sctx, stripe, good, true);
                 }
         }
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 98d6b8cc3874..cced6aeff8d7 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1659,6 +1659,13 @@ void btrfs_record_physical_zoned(struct btrfs_bio
*bbio)
         const u64 physical = bbio->bio.bi_iter.bi_sector << SECTOR_SHIFT;
         struct btrfs_ordered_extent *ordered;

+       /*
+        * For scrub case we have no inode, and doesn't need to bother
ordered
+        * extents.
+        */
+       if (!bbio->inode)
+               return;
+
         ordered = btrfs_lookup_ordered_extent(bbio->inode,
bbio->file_offset);
         if (WARN_ON(!ordered))
                 return;