| Message ID | d3aec3e168a547dcc39a764f242d1df9d3489928.1618494550.git.johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned: automatic BG reclaim | expand |
On 4/15/21 9:58 AM, Johannes Thumshirn wrote: > When relocating a block group the freed up space is not discarded in one > big block, but each extent is discarded on it's own with -odisard=sync. > > For a zoned filesystem we need to discard the whole block group at once, > so btrfs_discard_extent() will translate the discard into a > REQ_OP_ZONE_RESET operation, which then resets the device's zone. > > Link: https://lore.kernel.org/linux-btrfs/459e2932c48e12e883dcfd3dda828d9da251d5b5.1617962110.git.johannes.thumshirn@wdc.com > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> What would be cool is if we could disable discard per bg so we don't discard at all during the relocation, and then discard the whole block group no matter if we have zoned or not. However not really something you need to do, just thinking out loud Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
On 15/04/2021 20:26, Josef Bacik wrote: > On 4/15/21 9:58 AM, Johannes Thumshirn wrote: >> When relocating a block group the freed up space is not discarded in one >> big block, but each extent is discarded on it's own with -odisard=sync. >> >> For a zoned filesystem we need to discard the whole block group at once, >> so btrfs_discard_extent() will translate the discard into a >> REQ_OP_ZONE_RESET operation, which then resets the device's zone. >> >> Link: https://lore.kernel.org/linux-btrfs/459e2932c48e12e883dcfd3dda828d9da251d5b5.1617962110.git.johannes.thumshirn@wdc.com >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > What would be cool is if we could disable discard per bg so we don't discard at > all during the relocation, and then discard the whole block group no matter if > we have zoned or not. However not really something you need to do, just > thinking out loud I could say I'll add it to my queue, but that's already so long, I have no idea when that's gonna happen. > Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks
On Thu, Apr 15, 2021 at 3:00 PM Johannes Thumshirn <johannes.thumshirn@wdc.com> wrote: > > When relocating a block group the freed up space is not discarded in one > big block, but each extent is discarded on it's own with -odisard=sync. > > For a zoned filesystem we need to discard the whole block group at once, > so btrfs_discard_extent() will translate the discard into a > REQ_OP_ZONE_RESET operation, which then resets the device's zone. > > Link: https://lore.kernel.org/linux-btrfs/459e2932c48e12e883dcfd3dda828d9da251d5b5.1617962110.git.johannes.thumshirn@wdc.com > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > fs/btrfs/volumes.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 6d9b2369f17a..b1bab75ec12a 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -3103,6 +3103,7 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) > struct btrfs_root *root = fs_info->chunk_root; > struct btrfs_trans_handle *trans; > struct btrfs_block_group *block_group; > + u64 length; > int ret; > > /* > @@ -3130,8 +3131,24 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) > if (!block_group) > return -ENOENT; > btrfs_discard_cancel_work(&fs_info->discard_ctl, block_group); > + length = block_group->length; > btrfs_put_block_group(block_group); > > + /* > + * Step two, delete the device extents and the chunk tree entries. > + * > + * On a zoned file system, discard the whole block group, this will > + * trigger a REQ_OP_ZONE_RESET operation on the device zone. If > + * resetting the zone fails, don't treat it as a fatal problem from the > + * filesystem's point of view. > + */ > + if (btrfs_is_zoned(fs_info)) { > + ret = btrfs_discard_extent(fs_info, chunk_offset, length, NULL); > + if (ret) > + btrfs_info(fs_info, "failed to reset zone %llu", > + chunk_offset); > + } > + > trans = btrfs_start_trans_remove_block_group(root->fs_info, > chunk_offset); > if (IS_ERR(trans)) { > @@ -3140,10 +3157,6 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) > return ret; > } > > - /* > - * step two, delete the device extents and the > - * chunk tree entries > - */ This hunk seems unrelated and unintentional. Not that the comment adds any value, but if the removal was intentional, I would suggest a separate patch for that. Other than that, it looks good, thanks. Reviewed-by: Filipe Manana <fdmanana@suse.com> > ret = btrfs_remove_chunk(trans, chunk_offset); > btrfs_end_transaction(trans); > return ret; > -- > 2.30.0 >
On 16/04/2021 11:12, Filipe Manana wrote: > On Thu, Apr 15, 2021 at 3:00 PM Johannes Thumshirn > <johannes.thumshirn@wdc.com> wrote: >> >> When relocating a block group the freed up space is not discarded in one >> big block, but each extent is discarded on it's own with -odisard=sync. >> >> For a zoned filesystem we need to discard the whole block group at once, >> so btrfs_discard_extent() will translate the discard into a >> REQ_OP_ZONE_RESET operation, which then resets the device's zone. >> >> Link: https://lore.kernel.org/linux-btrfs/459e2932c48e12e883dcfd3dda828d9da251d5b5.1617962110.git.johannes.thumshirn@wdc.com >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> --- >> fs/btrfs/volumes.c | 21 +++++++++++++++++---- >> 1 file changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 6d9b2369f17a..b1bab75ec12a 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -3103,6 +3103,7 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) >> struct btrfs_root *root = fs_info->chunk_root; >> struct btrfs_trans_handle *trans; >> struct btrfs_block_group *block_group; >> + u64 length; >> int ret; >> >> /* >> @@ -3130,8 +3131,24 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) >> if (!block_group) >> return -ENOENT; >> btrfs_discard_cancel_work(&fs_info->discard_ctl, block_group); >> + length = block_group->length; >> btrfs_put_block_group(block_group); >> >> + /* >> + * Step two, delete the device extents and the chunk tree entries. >> + * >> + * On a zoned file system, discard the whole block group, this will >> + * trigger a REQ_OP_ZONE_RESET operation on the device zone. If >> + * resetting the zone fails, don't treat it as a fatal problem from the >> + * filesystem's point of view. >> + */ >> + if (btrfs_is_zoned(fs_info)) { >> + ret = btrfs_discard_extent(fs_info, chunk_offset, length, NULL); >> + if (ret) >> + btrfs_info(fs_info, "failed to reset zone %llu", >> + chunk_offset); >> + } >> + >> trans = btrfs_start_trans_remove_block_group(root->fs_info, >> chunk_offset); >> if (IS_ERR(trans)) { >> @@ -3140,10 +3157,6 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) >> return ret; >> } >> >> - /* >> - * step two, delete the device extents and the >> - * chunk tree entries >> - */ > > This hunk seems unrelated and unintentional. > Not that the comment adds any value, but if the removal was > intentional, I would suggest a separate patch for that. It's moved upwards + /* + * Step two, delete the device extents and the chunk tree entries. + * + * On a zoned file system, discard the whole block group, this will + * trigger a REQ_OP_ZONE_RESET operation on the device zone. If + * resetting the zone fails, don't treat it as a fatal problem from the + * filesystem's point of view. + */ 'cause technically the "delete extents" step starts with the discard now. But I can leave it where it was. > Other than that, it looks good, thanks. > > Reviewed-by: Filipe Manana <fdmanana@suse.com> > Thanks
On Fri, Apr 16, 2021 at 10:14 AM Johannes Thumshirn <Johannes.Thumshirn@wdc.com> wrote: > > On 16/04/2021 11:12, Filipe Manana wrote: > > On Thu, Apr 15, 2021 at 3:00 PM Johannes Thumshirn > > <johannes.thumshirn@wdc.com> wrote: > >> > >> When relocating a block group the freed up space is not discarded in one > >> big block, but each extent is discarded on it's own with -odisard=sync. > >> > >> For a zoned filesystem we need to discard the whole block group at once, > >> so btrfs_discard_extent() will translate the discard into a > >> REQ_OP_ZONE_RESET operation, which then resets the device's zone. > >> > >> Link: https://lore.kernel.org/linux-btrfs/459e2932c48e12e883dcfd3dda828d9da251d5b5.1617962110.git.johannes.thumshirn@wdc.com > >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > >> --- > >> fs/btrfs/volumes.c | 21 +++++++++++++++++---- > >> 1 file changed, 17 insertions(+), 4 deletions(-) > >> > >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > >> index 6d9b2369f17a..b1bab75ec12a 100644 > >> --- a/fs/btrfs/volumes.c > >> +++ b/fs/btrfs/volumes.c > >> @@ -3103,6 +3103,7 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) > >> struct btrfs_root *root = fs_info->chunk_root; > >> struct btrfs_trans_handle *trans; > >> struct btrfs_block_group *block_group; > >> + u64 length; > >> int ret; > >> > >> /* > >> @@ -3130,8 +3131,24 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) > >> if (!block_group) > >> return -ENOENT; > >> btrfs_discard_cancel_work(&fs_info->discard_ctl, block_group); > >> + length = block_group->length; > >> btrfs_put_block_group(block_group); > >> > >> + /* > >> + * Step two, delete the device extents and the chunk tree entries. > >> + * > >> + * On a zoned file system, discard the whole block group, this will > >> + * trigger a REQ_OP_ZONE_RESET operation on the device zone. If > >> + * resetting the zone fails, don't treat it as a fatal problem from the > >> + * filesystem's point of view. > >> + */ > >> + if (btrfs_is_zoned(fs_info)) { > >> + ret = btrfs_discard_extent(fs_info, chunk_offset, length, NULL); > >> + if (ret) > >> + btrfs_info(fs_info, "failed to reset zone %llu", > >> + chunk_offset); > >> + } > >> + > >> trans = btrfs_start_trans_remove_block_group(root->fs_info, > >> chunk_offset); > >> if (IS_ERR(trans)) { > >> @@ -3140,10 +3157,6 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) > >> return ret; > >> } > >> > >> - /* > >> - * step two, delete the device extents and the > >> - * chunk tree entries > >> - */ > > > > This hunk seems unrelated and unintentional. > > Not that the comment adds any value, but if the removal was > > intentional, I would suggest a separate patch for that. > > It's moved upwards > > + /* > + * Step two, delete the device extents and the chunk tree entries. > + * > + * On a zoned file system, discard the whole block group, this will > + * trigger a REQ_OP_ZONE_RESET operation on the device zone. If > + * resetting the zone fails, don't treat it as a fatal problem from the > + * filesystem's point of view. > + */ > > 'cause technically the "delete extents" step starts with the discard now. But I > can leave it where it was. The comment refers to deleting the device extent items from the device tree, not really extents on disk. I.e. it's all about the items from the block group in the chunk and device trees. > > > Other than that, it looks good, thanks. > > > > Reviewed-by: Filipe Manana <fdmanana@suse.com> > > > > Thanks
On 16/04/2021 11:30, Filipe Manana wrote: > The comment refers to deleting the device extent items from the device > tree, not really extents on disk. > I.e. it's all about the items from the block group in the chunk and > device trees. OK I'll move it back where it was.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 6d9b2369f17a..b1bab75ec12a 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -3103,6 +3103,7 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) struct btrfs_root *root = fs_info->chunk_root; struct btrfs_trans_handle *trans; struct btrfs_block_group *block_group; + u64 length; int ret; /* @@ -3130,8 +3131,24 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) if (!block_group) return -ENOENT; btrfs_discard_cancel_work(&fs_info->discard_ctl, block_group); + length = block_group->length; btrfs_put_block_group(block_group); + /* + * Step two, delete the device extents and the chunk tree entries. + * + * On a zoned file system, discard the whole block group, this will + * trigger a REQ_OP_ZONE_RESET operation on the device zone. If + * resetting the zone fails, don't treat it as a fatal problem from the + * filesystem's point of view. + */ + if (btrfs_is_zoned(fs_info)) { + ret = btrfs_discard_extent(fs_info, chunk_offset, length, NULL); + if (ret) + btrfs_info(fs_info, "failed to reset zone %llu", + chunk_offset); + } + trans = btrfs_start_trans_remove_block_group(root->fs_info, chunk_offset); if (IS_ERR(trans)) { @@ -3140,10 +3157,6 @@ static int btrfs_relocate_chunk(struct btrfs_fs_info *fs_info, u64 chunk_offset) return ret; } - /* - * step two, delete the device extents and the - * chunk tree entries - */ ret = btrfs_remove_chunk(trans, chunk_offset); btrfs_end_transaction(trans); return ret;
When relocating a block group the freed up space is not discarded in one big block, but each extent is discarded on it's own with -odisard=sync. For a zoned filesystem we need to discard the whole block group at once, so btrfs_discard_extent() will translate the discard into a REQ_OP_ZONE_RESET operation, which then resets the device's zone. Link: https://lore.kernel.org/linux-btrfs/459e2932c48e12e883dcfd3dda828d9da251d5b5.1617962110.git.johannes.thumshirn@wdc.com Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/btrfs/volumes.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)