Re: [PATCH v3 01/12] btrfs: scrub: use dedicated super block verification function to scrub one super block

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

On 2023/3/21 13:22, Anand Jain wrote:
> On 20/03/2023 10:12, Qu Wenruo wrote:
>> There is really no need to go through the super complex scrub_sectors()
>> to just handle super blocks.
>>
>> This patch will introduce a dedicated function (less than 50 lines) to
>> handle super block scrubing.
>>
>> This new function will introduce a behavior change, instead of using the
>> complex but concurrent scrub_bio system, here we just go
>> submit-and-wait.
>>
>> There is really not much sense to care the performance of super block
>> scrubbing. It only has 3 super blocks at most, and they are all scattered
>> around the devices already.
>>
> 
> Looks good
> 
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Reviewed-by: Anand Jain <anand.jain@oracle.com>
> 
> nits below:
> 
>> ---
>>   fs/btrfs/scrub.c | 54 +++++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 46 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>> index 3cdf73277e7e..e765eb8b8bcf 100644
>> --- a/fs/btrfs/scrub.c
>> +++ b/fs/btrfs/scrub.c
>> @@ -4243,18 +4243,59 @@ int scrub_enumerate_chunks(struct scrub_ctx 
>> *sctx,
>>       return ret;
>>   }
>> +static int scrub_one_super(struct scrub_ctx *sctx, struct 
>> btrfs_device *dev,
>> +               struct page *page, u64 physical, u64 generation)
>> +{
>> +    struct btrfs_fs_info *fs_info = sctx->fs_info; > +    struct 
>> bio_vec bvec;
>> +    struct bio bio;
>> +    struct btrfs_super_block *sb = page_address(page);
>> +    int ret;
>> +
>> +    bio_init(&bio, dev->bdev, &bvec, 1, REQ_OP_READ);
>> +    bio.bi_iter.bi_sector = physical >> SECTOR_SHIFT;
>> +    bio_add_page(&bio, page, BTRFS_SUPER_INFO_SIZE, 0);
>> +    ret = submit_bio_wait(&bio);
>> +    bio_uninit(&bio);
>> +
>> +    if (ret < 0)
>> +        return ret;
>> +    ret = btrfs_check_super_csum(fs_info, sb);
>> +    if (ret != 0) {
>> +        btrfs_err_rl(fs_info,
>> +            "super block at physical %llu devid %llu has bad csum",
>> +            physical, dev->devid);
>> +        return -EIO;
>> +    }
>> +    if (btrfs_super_generation(sb) != generation) {
>> +        btrfs_err_rl(fs_info,
>> +"super block at physical %llu devid %llu has bad generation, has %llu 
>> expect %llu",
>> +                 physical, dev->devid,
>> +                 btrfs_super_generation(sb), generation);
>> +        return -EUCLEAN;
>> +    }
>> +
>> +    ret = btrfs_validate_super(fs_info, sb, -1);
>> +    return ret;
>> +}
>> +
>>   static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx,
>>                          struct btrfs_device *scrub_dev)
>>   {
> 
>   scrub_supers() no longer requires struct scrub_ctx * as a parameter,
>   but this should modify from scrub_supers().
> 
>   A separate patch submitted.
> 
>>       int    i;
>>       u64    bytenr;
>>       u64    gen;
>> -    int    ret;
>> +    int    ret = 0;
>> +    struct page *page;
>>       struct btrfs_fs_info *fs_info = sctx->fs_info;
>>       if (BTRFS_FS_ERROR(fs_info))
>>           return -EROFS;
>> +    page = alloc_page(GFP_KERNEL);
>> +    if (!page)
>> +        return -ENOMEM;
>> +
> 
>   Over allocation for PAGESIZE>4K is unoptimized for SB, which is
>   acceptable. Add a comment to clarify.

For this, I'm not sure if it's that unoptimized.

The "alternative" is to just allocate 4K memory, but bio needs a page 
pointer, not a memory pointer (it can be converted, but not simple if 
not aligned).

The PAGESIZE > 4K one is only not ideal for memory usage, which I'd say 
doesn't worthy a full comment.
At most an ASSERT() like "ASSERT(PAGE_SIZE >= BTRFS_SUPER_INFO_SIZE);".

Thanks,
Qu

> 
> Thanks, Anand
> 
>>       /* Seed devices of a new filesystem has their own generation. */
>>       if (scrub_dev->fs_devices != fs_info->fs_devices)
>>           gen = scrub_dev->generation;
>> @@ -4269,15 +4310,12 @@ static noinline_for_stack int 
>> scrub_supers(struct scrub_ctx *sctx,
>>           if (!btrfs_check_super_location(scrub_dev, bytenr))
>>               continue;
>> -        ret = scrub_sectors(sctx, bytenr, BTRFS_SUPER_INFO_SIZE, bytenr,
>> -                    scrub_dev, BTRFS_EXTENT_FLAG_SUPER, gen, i,
>> -                    NULL, bytenr);
>> +        ret = scrub_one_super(sctx, scrub_dev, page, bytenr, gen);
>>           if (ret)
>> -            return ret;
>> +            break;
>>       }
> 
>> -    wait_event(sctx->list_wait, atomic_read(&sctx->bios_in_flight) == 
>> 0);
> 
>   Nice.  :-)
> 
> Thanks, Anand
> 
>> -
>> -    return 0;
>> +    __free_page(page);
>> +    return ret;
>>   }
>>   static void scrub_workers_put(struct btrfs_fs_info *fs_info)
>