On Fri, Apr 16, 2021 at 10:14 AM Johannes Thumshirn <johannes.thumsh...@wdc.com> wrote: > > On 16/04/2021 11:12, Filipe Manana wrote: > > On Thu, Apr 15, 2021 at 3:00 PM Johannes Thumshirn > > <johannes.thumsh...@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.thumsh...@wdc.com > >> Signed-off-by: Johannes Thumshirn <johannes.thumsh...@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 <fdman...@suse.com> > > > > Thanks -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
