| Message ID | 4d5e42d343318979a254f7dbdd96aa1c48908ed8.1651157034.git.naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned: fixes for zone finishing | expand |
On Fri, Apr 29, 2022 at 12:02:15AM +0900, Naohiro Aota wrote: > 1 file changed, 54 insertions(+), 73 deletions(-) > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index 997a96d7a3d5..9cddafe78fb1 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -1879,20 +1879,14 @@ bool btrfs_zone_activate(struct btrfs_block_group *block_group) > return ret; > } > > -int btrfs_zone_finish(struct btrfs_block_group *block_group) > +static int __btrfs_zone_finish(struct btrfs_block_group *block_group, bool nowait) Can you please avoid using __ for functions? Eg. the main function can be btrfs_zone_finish(taking 2 parameters) and the exported one being btrfs_zone_finish_nowait reflecting the preset parameter and also making the 'nowait' semantics clear. > { > struct btrfs_fs_info *fs_info = block_group->fs_info; > struct map_lookup *map; > - struct btrfs_device *device; > - u64 physical; > + bool need_zone_finish; > int ret = 0; > int i; > > - if (!btrfs_is_zoned(fs_info)) > - return 0; > - > - map = block_group->physical_map; > - > spin_lock(&block_group->lock); > if (!block_group->zone_is_active) { > spin_unlock(&block_group->lock); > @@ -1906,36 +1900,42 @@ int btrfs_zone_finish(struct btrfs_block_group *block_group) > spin_unlock(&block_group->lock); > return -EAGAIN; > } > - spin_unlock(&block_group->lock); > > - ret = btrfs_inc_block_group_ro(block_group, false); > - if (ret) > - return ret; > + if (!nowait) { > + spin_unlock(&block_group->lock); > > - /* Ensure all writes in this block group finish */ > - btrfs_wait_block_group_reservations(block_group); > - /* No need to wait for NOCOW writers. Zoned mode does not allow that. */ > - btrfs_wait_ordered_roots(fs_info, U64_MAX, block_group->start, > - block_group->length); > + ret = btrfs_inc_block_group_ro(block_group, false); > + if (ret) > + return ret; > > - spin_lock(&block_group->lock); > + /* Ensure all writes in this block group finish */ > + btrfs_wait_block_group_reservations(block_group); > + /* No need to wait for NOCOW writers. Zoned mode does not allow that. */ > + btrfs_wait_ordered_roots(fs_info, U64_MAX, block_group->start, > + block_group->length); > > - /* > - * Bail out if someone already deactivated the block group, or > - * allocated space is left in the block group. > - */ > - if (!block_group->zone_is_active) { > - spin_unlock(&block_group->lock); > - btrfs_dec_block_group_ro(block_group); > - return 0; > - } > + spin_lock(&block_group->lock); > > - if (block_group->reserved) { > - spin_unlock(&block_group->lock); > - btrfs_dec_block_group_ro(block_group); > - return -EAGAIN; > + /* > + * Bail out if someone already deactivated the block group, or > + * allocated space is left in the block group. > + */ > + if (!block_group->zone_is_active) { > + spin_unlock(&block_group->lock); > + btrfs_dec_block_group_ro(block_group); > + return 0; > + } > + > + if (block_group->reserved) { > + spin_unlock(&block_group->lock); > + btrfs_dec_block_group_ro(block_group); > + return -EAGAIN; > + } > } > > + /* There is unwritten space left. Need to finish the underlying zones. */ > + need_zone_finish = (block_group->zone_capacity - block_group->alloc_offset) > 0; This could be simplified to 'bg->zone_capacity > bg->alloc_offset', but maybe should be behind a helper as the expression appears more times. > + > block_group->zone_is_active = 0; > block_group->alloc_offset = block_group->zone_capacity; > block_group->free_space_ctl->free_space = 0; > @@ -1943,24 +1943,29 @@ int btrfs_zone_finish(struct btrfs_block_group *block_group) > btrfs_clear_data_reloc_bg(block_group); > spin_unlock(&block_group->lock); > > + map = block_group->physical_map; > for (i = 0; i < map->num_stripes; i++) { > - device = map->stripes[i].dev; > - physical = map->stripes[i].physical; > + struct btrfs_device *device = map->stripes[i].dev; > + const u64 physical = map->stripes[i].physical; > > if (device->zone_info->max_active_zones == 0) > continue; > > - ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH, > - physical >> SECTOR_SHIFT, > - device->zone_info->zone_size >> SECTOR_SHIFT, > - GFP_NOFS); > + if (need_zone_finish) { > + ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH, > + physical >> SECTOR_SHIFT, > + device->zone_info->zone_size >> SECTOR_SHIFT, > + GFP_NOFS); > > - if (ret) > - return ret; > + if (ret) > + return ret; > + } > > btrfs_dev_clear_active_zone(device, physical); > } > - btrfs_dec_block_group_ro(block_group); > + > + if (!nowait) > + btrfs_dec_block_group_ro(block_group); > > spin_lock(&fs_info->zone_active_bgs_lock); > ASSERT(!list_empty(&block_group->active_bg_list));
On Thu, Apr 28, 2022 at 06:11:03PM +0200, David Sterba wrote: > On Fri, Apr 29, 2022 at 12:02:15AM +0900, Naohiro Aota wrote: > > 1 file changed, 54 insertions(+), 73 deletions(-) > > > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > > index 997a96d7a3d5..9cddafe78fb1 100644 > > --- a/fs/btrfs/zoned.c > > +++ b/fs/btrfs/zoned.c > > @@ -1879,20 +1879,14 @@ bool btrfs_zone_activate(struct btrfs_block_group *block_group) > > return ret; > > } > > > > -int btrfs_zone_finish(struct btrfs_block_group *block_group) > > +static int __btrfs_zone_finish(struct btrfs_block_group *block_group, bool nowait) > > Can you please avoid using __ for functions? Eg. the main function can > be btrfs_zone_finish(taking 2 parameters) and the exported one being > btrfs_zone_finish_nowait reflecting the preset parameter and also making > the 'nowait' semantics clear. But, we exports both btrfs_zone_finish() (the waiting variant) and btrfs_zone_finish_endio() (the nowait variant + checking the left space after write). How about "do_zone_finish(block_group, bool nowait)" as a main function and "btrfs_zone_finish(block_group)" and "btrfs_zone_finish_endio(block_group)" ? > > { > > struct btrfs_fs_info *fs_info = block_group->fs_info; > > struct map_lookup *map; > > - struct btrfs_device *device; > > - u64 physical; > > + bool need_zone_finish; > > int ret = 0; > > int i; > > > > - if (!btrfs_is_zoned(fs_info)) > > - return 0; > > - > > - map = block_group->physical_map; > > - > > spin_lock(&block_group->lock); > > if (!block_group->zone_is_active) { > > spin_unlock(&block_group->lock); > > @@ -1906,36 +1900,42 @@ int btrfs_zone_finish(struct btrfs_block_group *block_group) > > spin_unlock(&block_group->lock); > > return -EAGAIN; > > } > > - spin_unlock(&block_group->lock); > > > > - ret = btrfs_inc_block_group_ro(block_group, false); > > - if (ret) > > - return ret; > > + if (!nowait) { > > + spin_unlock(&block_group->lock); > > > > - /* Ensure all writes in this block group finish */ > > - btrfs_wait_block_group_reservations(block_group); > > - /* No need to wait for NOCOW writers. Zoned mode does not allow that. */ > > - btrfs_wait_ordered_roots(fs_info, U64_MAX, block_group->start, > > - block_group->length); > > + ret = btrfs_inc_block_group_ro(block_group, false); > > + if (ret) > > + return ret; > > > > - spin_lock(&block_group->lock); > > + /* Ensure all writes in this block group finish */ > > + btrfs_wait_block_group_reservations(block_group); > > + /* No need to wait for NOCOW writers. Zoned mode does not allow that. */ > > + btrfs_wait_ordered_roots(fs_info, U64_MAX, block_group->start, > > + block_group->length); > > > > - /* > > - * Bail out if someone already deactivated the block group, or > > - * allocated space is left in the block group. > > - */ > > - if (!block_group->zone_is_active) { > > - spin_unlock(&block_group->lock); > > - btrfs_dec_block_group_ro(block_group); > > - return 0; > > - } > > + spin_lock(&block_group->lock); > > > > - if (block_group->reserved) { > > - spin_unlock(&block_group->lock); > > - btrfs_dec_block_group_ro(block_group); > > - return -EAGAIN; > > + /* > > + * Bail out if someone already deactivated the block group, or > > + * allocated space is left in the block group. > > + */ > > + if (!block_group->zone_is_active) { > > + spin_unlock(&block_group->lock); > > + btrfs_dec_block_group_ro(block_group); > > + return 0; > > + } > > + > > + if (block_group->reserved) { > > + spin_unlock(&block_group->lock); > > + btrfs_dec_block_group_ro(block_group); > > + return -EAGAIN; > > + } > > } > > > > + /* There is unwritten space left. Need to finish the underlying zones. */ > > + need_zone_finish = (block_group->zone_capacity - block_group->alloc_offset) > 0; > > This could be simplified to 'bg->zone_capacity > bg->alloc_offset', > but maybe should be behind a helper as the expression appears more > times. Yep. True. I think extent-tree.c has some of this. Let me check. > > + > > block_group->zone_is_active = 0; > > block_group->alloc_offset = block_group->zone_capacity; > > block_group->free_space_ctl->free_space = 0; > > @@ -1943,24 +1943,29 @@ int btrfs_zone_finish(struct btrfs_block_group *block_group) > > btrfs_clear_data_reloc_bg(block_group); > > spin_unlock(&block_group->lock); > > > > + map = block_group->physical_map; > > for (i = 0; i < map->num_stripes; i++) { > > - device = map->stripes[i].dev; > > - physical = map->stripes[i].physical; > > + struct btrfs_device *device = map->stripes[i].dev; > > + const u64 physical = map->stripes[i].physical; > > > > if (device->zone_info->max_active_zones == 0) > > continue; > > > > - ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH, > > - physical >> SECTOR_SHIFT, > > - device->zone_info->zone_size >> SECTOR_SHIFT, > > - GFP_NOFS); > > + if (need_zone_finish) { > > + ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH, > > + physical >> SECTOR_SHIFT, > > + device->zone_info->zone_size >> SECTOR_SHIFT, > > + GFP_NOFS); > > > > - if (ret) > > - return ret; > > + if (ret) > > + return ret; > > + } > > > > btrfs_dev_clear_active_zone(device, physical); > > } > > - btrfs_dec_block_group_ro(block_group); > > + > > + if (!nowait) > > + btrfs_dec_block_group_ro(block_group); > > > > spin_lock(&fs_info->zone_active_bgs_lock); > > ASSERT(!list_empty(&block_group->active_bg_list));
On Fri, Apr 29, 2022 at 04:56:33AM +0000, Naohiro Aota wrote: > On Thu, Apr 28, 2022 at 06:11:03PM +0200, David Sterba wrote: > > On Fri, Apr 29, 2022 at 12:02:15AM +0900, Naohiro Aota wrote: > > > 1 file changed, 54 insertions(+), 73 deletions(-) > > > > > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > > > index 997a96d7a3d5..9cddafe78fb1 100644 > > > --- a/fs/btrfs/zoned.c > > > +++ b/fs/btrfs/zoned.c > > > @@ -1879,20 +1879,14 @@ bool btrfs_zone_activate(struct btrfs_block_group *block_group) > > > return ret; > > > } > > > > > > -int btrfs_zone_finish(struct btrfs_block_group *block_group) > > > +static int __btrfs_zone_finish(struct btrfs_block_group *block_group, bool nowait) > > > > Can you please avoid using __ for functions? Eg. the main function can > > be btrfs_zone_finish(taking 2 parameters) and the exported one being > > btrfs_zone_finish_nowait reflecting the preset parameter and also making > > the 'nowait' semantics clear. > > But, we exports both btrfs_zone_finish() (the waiting variant) and > btrfs_zone_finish_endio() (the nowait variant + checking the left space > after write). How about "do_zone_finish(block_group, bool nowait)" as a > main function and "btrfs_zone_finish(block_group)" and > "btrfs_zone_finish_endio(block_group)" ? Yeah, do_zone_finish works as well.
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c index 997a96d7a3d5..9cddafe78fb1 100644 --- a/fs/btrfs/zoned.c +++ b/fs/btrfs/zoned.c @@ -1879,20 +1879,14 @@ bool btrfs_zone_activate(struct btrfs_block_group *block_group) return ret; } -int btrfs_zone_finish(struct btrfs_block_group *block_group) +static int __btrfs_zone_finish(struct btrfs_block_group *block_group, bool nowait) { struct btrfs_fs_info *fs_info = block_group->fs_info; struct map_lookup *map; - struct btrfs_device *device; - u64 physical; + bool need_zone_finish; int ret = 0; int i; - if (!btrfs_is_zoned(fs_info)) - return 0; - - map = block_group->physical_map; - spin_lock(&block_group->lock); if (!block_group->zone_is_active) { spin_unlock(&block_group->lock); @@ -1906,36 +1900,42 @@ int btrfs_zone_finish(struct btrfs_block_group *block_group) spin_unlock(&block_group->lock); return -EAGAIN; } - spin_unlock(&block_group->lock); - ret = btrfs_inc_block_group_ro(block_group, false); - if (ret) - return ret; + if (!nowait) { + spin_unlock(&block_group->lock); - /* Ensure all writes in this block group finish */ - btrfs_wait_block_group_reservations(block_group); - /* No need to wait for NOCOW writers. Zoned mode does not allow that. */ - btrfs_wait_ordered_roots(fs_info, U64_MAX, block_group->start, - block_group->length); + ret = btrfs_inc_block_group_ro(block_group, false); + if (ret) + return ret; - spin_lock(&block_group->lock); + /* Ensure all writes in this block group finish */ + btrfs_wait_block_group_reservations(block_group); + /* No need to wait for NOCOW writers. Zoned mode does not allow that. */ + btrfs_wait_ordered_roots(fs_info, U64_MAX, block_group->start, + block_group->length); - /* - * Bail out if someone already deactivated the block group, or - * allocated space is left in the block group. - */ - if (!block_group->zone_is_active) { - spin_unlock(&block_group->lock); - btrfs_dec_block_group_ro(block_group); - return 0; - } + spin_lock(&block_group->lock); - if (block_group->reserved) { - spin_unlock(&block_group->lock); - btrfs_dec_block_group_ro(block_group); - return -EAGAIN; + /* + * Bail out if someone already deactivated the block group, or + * allocated space is left in the block group. + */ + if (!block_group->zone_is_active) { + spin_unlock(&block_group->lock); + btrfs_dec_block_group_ro(block_group); + return 0; + } + + if (block_group->reserved) { + spin_unlock(&block_group->lock); + btrfs_dec_block_group_ro(block_group); + return -EAGAIN; + } } + /* There is unwritten space left. Need to finish the underlying zones. */ + need_zone_finish = (block_group->zone_capacity - block_group->alloc_offset) > 0; + block_group->zone_is_active = 0; block_group->alloc_offset = block_group->zone_capacity; block_group->free_space_ctl->free_space = 0; @@ -1943,24 +1943,29 @@ int btrfs_zone_finish(struct btrfs_block_group *block_group) btrfs_clear_data_reloc_bg(block_group); spin_unlock(&block_group->lock); + map = block_group->physical_map; for (i = 0; i < map->num_stripes; i++) { - device = map->stripes[i].dev; - physical = map->stripes[i].physical; + struct btrfs_device *device = map->stripes[i].dev; + const u64 physical = map->stripes[i].physical; if (device->zone_info->max_active_zones == 0) continue; - ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH, - physical >> SECTOR_SHIFT, - device->zone_info->zone_size >> SECTOR_SHIFT, - GFP_NOFS); + if (need_zone_finish) { + ret = blkdev_zone_mgmt(device->bdev, REQ_OP_ZONE_FINISH, + physical >> SECTOR_SHIFT, + device->zone_info->zone_size >> SECTOR_SHIFT, + GFP_NOFS); - if (ret) - return ret; + if (ret) + return ret; + } btrfs_dev_clear_active_zone(device, physical); } - btrfs_dec_block_group_ro(block_group); + + if (!nowait) + btrfs_dec_block_group_ro(block_group); spin_lock(&fs_info->zone_active_bgs_lock); ASSERT(!list_empty(&block_group->active_bg_list)); @@ -1973,6 +1978,14 @@ int btrfs_zone_finish(struct btrfs_block_group *block_group) return 0; } +int btrfs_zone_finish(struct btrfs_block_group *block_group) +{ + if (!btrfs_is_zoned(block_group->fs_info)) + return 0; + + return __btrfs_zone_finish(block_group, false); +} + bool btrfs_can_activate_zone(struct btrfs_fs_devices *fs_devices, u64 flags) { struct btrfs_fs_info *fs_info = fs_devices->fs_info; @@ -2004,9 +2017,6 @@ bool btrfs_can_activate_zone(struct btrfs_fs_devices *fs_devices, u64 flags) void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical, u64 length) { struct btrfs_block_group *block_group; - struct map_lookup *map; - struct btrfs_device *device; - u64 physical; if (!btrfs_is_zoned(fs_info)) return; @@ -2017,36 +2027,7 @@ void btrfs_zone_finish_endio(struct btrfs_fs_info *fs_info, u64 logical, u64 len if (logical + length < block_group->start + block_group->zone_capacity) goto out; - spin_lock(&block_group->lock); - - if (!block_group->zone_is_active) { - spin_unlock(&block_group->lock); - goto out; - } - - block_group->zone_is_active = 0; - /* We should have consumed all the free space */ - ASSERT(block_group->alloc_offset == block_group->zone_capacity); - ASSERT(block_group->free_space_ctl->free_space == 0); - btrfs_clear_treelog_bg(block_group); - btrfs_clear_data_reloc_bg(block_group); - spin_unlock(&block_group->lock); - - map = block_group->physical_map; - device = map->stripes[0].dev; - physical = map->stripes[0].physical; - - if (!device->zone_info->max_active_zones) - goto out; - - btrfs_dev_clear_active_zone(device, physical); - - spin_lock(&fs_info->zone_active_bgs_lock); - ASSERT(!list_empty(&block_group->active_bg_list)); - list_del_init(&block_group->active_bg_list); - spin_unlock(&fs_info->zone_active_bgs_lock); - - btrfs_put_block_group(block_group); + __btrfs_zone_finish(block_group, true); out: btrfs_put_block_group(block_group);
btrfs_zone_finish() and btrfs_zone_finish_endio() have similar code. Introduce __btrfs_zone_finish() to consolidate them. Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- fs/btrfs/zoned.c | 127 ++++++++++++++++++++--------------------------- 1 file changed, 54 insertions(+), 73 deletions(-)