Re: new scrub code vs zoned file systems

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

On 2023/6/1 06:25, Qu Wenruo wrote:
> 
> 
> 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);

In fact this is not even needed.

The scrub_write_sectors() already have all the fill_writer_pointer_gap() 
calls for the block group.

Meaning we can pass the existing @good bitmap, even with gaps, and 
scrub_write_sectors() would handle it properly.
Only the NULL inode pointer check is needed.

Thus the initial problem of that crash is really not about the write gaps.

[   53.691003] nvme3n1: Zone Management Append(0x7d) @ LBA 65536, 4 
blocks, Zone Is Full (sct 0x1 / sc 0xb9) DNR
[   53.694996] I/O error, dev nvme3n1, sector 786432 op 
0xd:(ZONE_APPEND) flags 0x4000 phys_seg 3 prio class 2

Any clue on why the target zone is full during replace?

Thanks,
Qu
> +                       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;