| Message ID | 94803d18b1c4ce208b6a93e37998718e61ea37d5.1679278088.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: scrub: use a more reader friendly code to implement scrub_simple_mirror() | expand |
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. 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)
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) >
On Tue, Mar 21, 2023 at 03:25:18PM +0800, Qu Wenruo wrote: > >> + 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. It's a one time allocation and short lived so some ineffectivity is tollerable. > At most an ASSERT() like "ASSERT(PAGE_SIZE >= BTRFS_SUPER_INFO_SIZE);". Yeah that's possible, but all current architectures have page size at least 4K, I doubt the assert makes sense.
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) { 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; + /* 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); - - return 0; + __free_page(page); + return ret; } static void scrub_workers_put(struct btrfs_fs_info *fs_info)
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. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/scrub.c | 54 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 8 deletions(-)