| Message ID | 10d0f55b196d4dc949f0ac29f2f0af023eaa7523.1679376183.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] btrfs: remove struct scrub_stx for superblock scrubbing | expand |
On 2023/3/21 13:23, Anand Jain wrote: > Following the patchset that implements reader-friendly scrub code > made the struct scrub_stx is no longer required for scrubbing superblocks. > > btrfs: scrub: use a more reader friendly code to implement scrub_simple_mirror() > > Therefore, scrub_ctx does not need to be passed as a parameter, > (unless there are other plans for it). > > This patch cleans up the code and is built on top of the above patchset. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> Looks good, if you're fine I can fold this into the offending patch in the next update. Thanks, Qu > --- > fs/btrfs/scrub.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index beccf763ae64..bc87277559d3 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -4909,12 +4909,12 @@ 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) > +static int scrub_one_super(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_fs_info *fs_info = dev->fs_info; > struct btrfs_super_block *sb = page_address(page); > int ret; > > @@ -4945,15 +4945,14 @@ static int scrub_one_super(struct scrub_ctx *sctx, struct btrfs_device *dev, > return ret; > } > > -static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx, > - struct btrfs_device *scrub_dev) > +static noinline_for_stack int scrub_supers(struct btrfs_device *scrub_dev) > { > int i; > u64 bytenr; > u64 gen; > int ret = 0; > struct page *page; > - struct btrfs_fs_info *fs_info = sctx->fs_info; > + struct btrfs_fs_info *fs_info = scrub_dev->fs_info; > > if (BTRFS_FS_ERROR(fs_info)) > return -EROFS; > @@ -4976,7 +4975,7 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx, > if (!btrfs_check_super_location(scrub_dev, bytenr)) > continue; > > - ret = scrub_one_super(sctx, scrub_dev, page, bytenr, gen); > + ret = scrub_one_super(scrub_dev, page, bytenr, gen); > if (ret) > break; > } > @@ -5172,7 +5171,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start, > * kick off writing super in log tree sync. > */ > mutex_lock(&fs_info->fs_devices->device_list_mutex); > - ret = scrub_supers(sctx, dev); > + ret = scrub_supers(dev); > mutex_unlock(&fs_info->fs_devices->device_list_mutex); > > spin_lock(&sctx->stat_lock);
On 3/21/23 15:26, Qu Wenruo wrote: > > > On 2023/3/21 13:23, Anand Jain wrote: >> Following the patchset that implements reader-friendly scrub code >> made the struct scrub_stx is no longer required for scrubbing >> superblocks. >> >> btrfs: scrub: use a more reader friendly code to implement >> scrub_simple_mirror() >> >> Therefore, scrub_ctx does not need to be passed as a parameter, >> (unless there are other plans for it). >> >> This patch cleans up the code and is built on top of the above patchset. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > Looks good, if you're fine I can fold this into the offending patch in > the next update. > IMO, this patch is a cleanup rather than a bug fix, so there isn't offending patch. If it is folded to the patch 1/12, it may be too many objectives in one patch. Nonetheless, I have no objections if you still decide to fold it. Thanks, Anand > Thanks, > Qu > >> --- >> fs/btrfs/scrub.c | 15 +++++++-------- >> 1 file changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >> index beccf763ae64..bc87277559d3 100644 >> --- a/fs/btrfs/scrub.c >> +++ b/fs/btrfs/scrub.c >> @@ -4909,12 +4909,12 @@ 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) >> +static int scrub_one_super(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_fs_info *fs_info = dev->fs_info; >> struct btrfs_super_block *sb = page_address(page); >> int ret; >> @@ -4945,15 +4945,14 @@ static int scrub_one_super(struct scrub_ctx >> *sctx, struct btrfs_device *dev, >> return ret; >> } >> -static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx, >> - struct btrfs_device *scrub_dev) >> +static noinline_for_stack int scrub_supers(struct btrfs_device >> *scrub_dev) >> { >> int i; >> u64 bytenr; >> u64 gen; >> int ret = 0; >> struct page *page; >> - struct btrfs_fs_info *fs_info = sctx->fs_info; >> + struct btrfs_fs_info *fs_info = scrub_dev->fs_info; >> if (BTRFS_FS_ERROR(fs_info)) >> return -EROFS; >> @@ -4976,7 +4975,7 @@ static noinline_for_stack int >> scrub_supers(struct scrub_ctx *sctx, >> if (!btrfs_check_super_location(scrub_dev, bytenr)) >> continue; >> - ret = scrub_one_super(sctx, scrub_dev, page, bytenr, gen); >> + ret = scrub_one_super(scrub_dev, page, bytenr, gen); >> if (ret) >> break; >> } >> @@ -5172,7 +5171,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info >> *fs_info, u64 devid, u64 start, >> * kick off writing super in log tree sync. >> */ >> mutex_lock(&fs_info->fs_devices->device_list_mutex); >> - ret = scrub_supers(sctx, dev); >> + ret = scrub_supers(dev); >> mutex_unlock(&fs_info->fs_devices->device_list_mutex); >> spin_lock(&sctx->stat_lock);
On 2023/3/21 18:11, Anand Jain wrote: > On 3/21/23 15:26, Qu Wenruo wrote: >> >> >> On 2023/3/21 13:23, Anand Jain wrote: >>> Following the patchset that implements reader-friendly scrub code >>> made the struct scrub_stx is no longer required for scrubbing >>> superblocks. >>> >>> btrfs: scrub: use a more reader friendly code to implement >>> scrub_simple_mirror() >>> >>> Therefore, scrub_ctx does not need to be passed as a parameter, >>> (unless there are other plans for it). >>> >>> This patch cleans up the code and is built on top of the above patchset. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> >> Looks good, if you're fine I can fold this into the offending patch in >> the next update. >> > > IMO, this patch is a cleanup rather than a bug fix, so there > isn't offending patch. If it is folded to the patch 1/12, it > may be too many objectives in one patch. The cleanup is only possible because of the patch "btrfs: scrub: use dedicated super block verification function to scrub one super block". As the old code relies on the scrub_sectors() function, thus needing the @sctx parameter. And it's indeed my fault not fully cleaning up the parameters. Thus I believe it's better to fold it into the mentioned patch. Thanks, Qu > > Nonetheless, I have no objections if you still decide to fold it. > > Thanks, Anand > > >> Thanks, >> Qu >> >>> --- >>> fs/btrfs/scrub.c | 15 +++++++-------- >>> 1 file changed, 7 insertions(+), 8 deletions(-) >>> >>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >>> index beccf763ae64..bc87277559d3 100644 >>> --- a/fs/btrfs/scrub.c >>> +++ b/fs/btrfs/scrub.c >>> @@ -4909,12 +4909,12 @@ 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) >>> +static int scrub_one_super(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_fs_info *fs_info = dev->fs_info; >>> struct btrfs_super_block *sb = page_address(page); >>> int ret; >>> @@ -4945,15 +4945,14 @@ static int scrub_one_super(struct scrub_ctx >>> *sctx, struct btrfs_device *dev, >>> return ret; >>> } >>> -static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx, >>> - struct btrfs_device *scrub_dev) >>> +static noinline_for_stack int scrub_supers(struct btrfs_device >>> *scrub_dev) >>> { >>> int i; >>> u64 bytenr; >>> u64 gen; >>> int ret = 0; >>> struct page *page; >>> - struct btrfs_fs_info *fs_info = sctx->fs_info; >>> + struct btrfs_fs_info *fs_info = scrub_dev->fs_info; >>> if (BTRFS_FS_ERROR(fs_info)) >>> return -EROFS; >>> @@ -4976,7 +4975,7 @@ static noinline_for_stack int >>> scrub_supers(struct scrub_ctx *sctx, >>> if (!btrfs_check_super_location(scrub_dev, bytenr)) >>> continue; >>> - ret = scrub_one_super(sctx, scrub_dev, page, bytenr, gen); >>> + ret = scrub_one_super(scrub_dev, page, bytenr, gen); >>> if (ret) >>> break; >>> } >>> @@ -5172,7 +5171,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info >>> *fs_info, u64 devid, u64 start, >>> * kick off writing super in log tree sync. >>> */ >>> mutex_lock(&fs_info->fs_devices->device_list_mutex); >>> - ret = scrub_supers(sctx, dev); >>> + ret = scrub_supers(dev); >>> mutex_unlock(&fs_info->fs_devices->device_list_mutex); >>> spin_lock(&sctx->stat_lock); >
On 23/03/2023 09:13, Qu Wenruo wrote: > > > On 2023/3/21 18:11, Anand Jain wrote: >> On 3/21/23 15:26, Qu Wenruo wrote: >>> >>> >>> On 2023/3/21 13:23, Anand Jain wrote: >>>> Following the patchset that implements reader-friendly scrub code >>>> made the struct scrub_stx is no longer required for scrubbing >>>> superblocks. >>>> >>>> btrfs: scrub: use a more reader friendly code to implement >>>> scrub_simple_mirror() >>>> >>>> Therefore, scrub_ctx does not need to be passed as a parameter, >>>> (unless there are other plans for it). >>>> >>>> This patch cleans up the code and is built on top of the above >>>> patchset. >>>> >>>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> >>> Looks good, if you're fine I can fold this into the offending patch >>> in the next update. >>> >> >> IMO, this patch is a cleanup rather than a bug fix, so there >> isn't offending patch. If it is folded to the patch 1/12, it >> may be too many objectives in one patch. > > The cleanup is only possible because of the patch "btrfs: scrub: use > dedicated super block verification function to scrub one super block". > > As the old code relies on the scrub_sectors() function, thus needing the > @sctx parameter. > > And it's indeed my fault not fully cleaning up the parameters. > > Thus I believe it's better to fold it into the mentioned patch. Yes. Please go ahead. Thanks, Anand > Thanks, > Qu >> >> Nonetheless, I have no objections if you still decide to fold it. >> >> Thanks, Anand >> >> >>> Thanks, >>> Qu >>> >>>> --- >>>> fs/btrfs/scrub.c | 15 +++++++-------- >>>> 1 file changed, 7 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >>>> index beccf763ae64..bc87277559d3 100644 >>>> --- a/fs/btrfs/scrub.c >>>> +++ b/fs/btrfs/scrub.c >>>> @@ -4909,12 +4909,12 @@ 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) >>>> +static int scrub_one_super(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_fs_info *fs_info = dev->fs_info; >>>> struct btrfs_super_block *sb = page_address(page); >>>> int ret; >>>> @@ -4945,15 +4945,14 @@ static int scrub_one_super(struct scrub_ctx >>>> *sctx, struct btrfs_device *dev, >>>> return ret; >>>> } >>>> -static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx, >>>> - struct btrfs_device *scrub_dev) >>>> +static noinline_for_stack int scrub_supers(struct btrfs_device >>>> *scrub_dev) >>>> { >>>> int i; >>>> u64 bytenr; >>>> u64 gen; >>>> int ret = 0; >>>> struct page *page; >>>> - struct btrfs_fs_info *fs_info = sctx->fs_info; >>>> + struct btrfs_fs_info *fs_info = scrub_dev->fs_info; >>>> if (BTRFS_FS_ERROR(fs_info)) >>>> return -EROFS; >>>> @@ -4976,7 +4975,7 @@ static noinline_for_stack int >>>> scrub_supers(struct scrub_ctx *sctx, >>>> if (!btrfs_check_super_location(scrub_dev, bytenr)) >>>> continue; >>>> - ret = scrub_one_super(sctx, scrub_dev, page, bytenr, gen); >>>> + ret = scrub_one_super(scrub_dev, page, bytenr, gen); >>>> if (ret) >>>> break; >>>> } >>>> @@ -5172,7 +5171,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info >>>> *fs_info, u64 devid, u64 start, >>>> * kick off writing super in log tree sync. >>>> */ >>>> mutex_lock(&fs_info->fs_devices->device_list_mutex); >>>> - ret = scrub_supers(sctx, dev); >>>> + ret = scrub_supers(dev); >>>> mutex_unlock(&fs_info->fs_devices->device_list_mutex); >>>> spin_lock(&sctx->stat_lock); >>
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index beccf763ae64..bc87277559d3 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -4909,12 +4909,12 @@ 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) +static int scrub_one_super(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_fs_info *fs_info = dev->fs_info; struct btrfs_super_block *sb = page_address(page); int ret; @@ -4945,15 +4945,14 @@ static int scrub_one_super(struct scrub_ctx *sctx, struct btrfs_device *dev, return ret; } -static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx, - struct btrfs_device *scrub_dev) +static noinline_for_stack int scrub_supers(struct btrfs_device *scrub_dev) { int i; u64 bytenr; u64 gen; int ret = 0; struct page *page; - struct btrfs_fs_info *fs_info = sctx->fs_info; + struct btrfs_fs_info *fs_info = scrub_dev->fs_info; if (BTRFS_FS_ERROR(fs_info)) return -EROFS; @@ -4976,7 +4975,7 @@ static noinline_for_stack int scrub_supers(struct scrub_ctx *sctx, if (!btrfs_check_super_location(scrub_dev, bytenr)) continue; - ret = scrub_one_super(sctx, scrub_dev, page, bytenr, gen); + ret = scrub_one_super(scrub_dev, page, bytenr, gen); if (ret) break; } @@ -5172,7 +5171,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start, * kick off writing super in log tree sync. */ mutex_lock(&fs_info->fs_devices->device_list_mutex); - ret = scrub_supers(sctx, dev); + ret = scrub_supers(dev); mutex_unlock(&fs_info->fs_devices->device_list_mutex); spin_lock(&sctx->stat_lock);
Following the patchset that implements reader-friendly scrub code made the struct scrub_stx is no longer required for scrubbing superblocks. btrfs: scrub: use a more reader friendly code to implement scrub_simple_mirror() Therefore, scrub_ctx does not need to be passed as a parameter, (unless there are other plans for it). This patch cleans up the code and is built on top of the above patchset. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/scrub.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)