| Message ID | 20210804184854.10696-4-mpdesouza@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Use btrfs_find_item whenever possible | expand |
On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote: > calculate_emulated_zone_size can be simplified by using btrfs_find_item, which > executes btrfs_search_slot and calls btrfs_next_leaf if needed. > > No functional changes. > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> But since we're here, some unrelated comment inlined below. > --- > fs/btrfs/zoned.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index 47af1ab3bf12..d344baa26de0 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info) > struct btrfs_key key; > struct extent_buffer *leaf; > struct btrfs_dev_extent *dext; > - int ret = 0; > - > - key.objectid = 1; Not related to this patch itself, but this immediate number is not that straightforward. In fact for DEV_EXTENT_KEY, the objectid means device number. For current zoned device support IIRC we only support single device, thus it's fixed to 1. It would be better to have some extra comment/ASSERT() for it. > - key.type = BTRFS_DEV_EXTENT_KEY; > - key.offset = 0; Normally for DEV_EXTENT_KEY, the offset is the dev physical offset, which normally is not 0 (as we reserve 0~1M for each device) So this is a special zoned on-disk format? Thanks, Qu > + int ret; > > path = btrfs_alloc_path(); > if (!path) > return -ENOMEM; > > - ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); > + ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key); > if (ret < 0) > goto out; > > - if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) { > - ret = btrfs_next_leaf(root, path); > - if (ret < 0) > - goto out; > - /* No dev extents at all? Not good */ > - if (ret > 0) { > - ret = -EUCLEAN; > - goto out; > - } > + /* No dev extents at all? Not good */ > + else if (ret > 0) { > + ret = -EUCLEAN; > + goto out; > } > > leaf = path->nodes[0]; >
On Thu, Aug 05, 2021 at 02:39:09PM +0800, Qu Wenruo wrote: > > > On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote: > > calculate_emulated_zone_size can be simplified by using btrfs_find_item, which > > executes btrfs_search_slot and calls btrfs_next_leaf if needed. > > > > No functional changes. > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > But since we're here, some unrelated comment inlined below. > > --- > > fs/btrfs/zoned.c | 21 ++++++--------------- > > 1 file changed, 6 insertions(+), 15 deletions(-) > > > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > > index 47af1ab3bf12..d344baa26de0 100644 > > --- a/fs/btrfs/zoned.c > > +++ b/fs/btrfs/zoned.c > > @@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info) > > struct btrfs_key key; > > struct extent_buffer *leaf; > > struct btrfs_dev_extent *dext; > > - int ret = 0; > > - > > - key.objectid = 1; > > Not related to this patch itself, but this immediate number is not that > straightforward. > > In fact for DEV_EXTENT_KEY, the objectid means device number. > > For current zoned device support IIRC we only support single device, > thus it's fixed to 1. To be precise, we can have multiple devices, but only support single profile. > It would be better to have some extra comment/ASSERT() for it. > > > > - key.type = BTRFS_DEV_EXTENT_KEY; > > - key.offset = 0; > > Normally for DEV_EXTENT_KEY, the offset is the dev physical offset, > which normally is not 0 (as we reserve 0~1M for each device) > > So this is a special zoned on-disk format? We also reserve the first two zones on a disk for superblock log zones, so on typical SMR drive, 0-512M is reserved. We can use any DEV_EXTENT item here to determine the emulated zone size. So, it's trying to find the first one. > Thanks, > Qu > > > + int ret; > > > > path = btrfs_alloc_path(); > > if (!path) > > return -ENOMEM; > > > > - ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); > > + ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key); > > if (ret < 0) > > goto out; > > > > - if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) { > > - ret = btrfs_next_leaf(root, path); > > - if (ret < 0) > > - goto out; > > - /* No dev extents at all? Not good */ > > - if (ret > 0) { > > - ret = -EUCLEAN; > > - goto out; > > - } > > + /* No dev extents at all? Not good */ > > + else if (ret > 0) { > > + ret = -EUCLEAN; > > + goto out; > > } > > > > leaf = path->nodes[0]; > >
On 2021/8/6 下午1:52, Naohiro Aota wrote: > On Thu, Aug 05, 2021 at 02:39:09PM +0800, Qu Wenruo wrote: >> >> >> On 2021/8/5 上午2:48, Marcos Paulo de Souza wrote: >>> calculate_emulated_zone_size can be simplified by using btrfs_find_item, which >>> executes btrfs_search_slot and calls btrfs_next_leaf if needed. >>> >>> No functional changes. >>> >>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> >> >> Reviewed-by: Qu Wenruo <wqu@suse.com> >> >> But since we're here, some unrelated comment inlined below. >>> --- >>> fs/btrfs/zoned.c | 21 ++++++--------------- >>> 1 file changed, 6 insertions(+), 15 deletions(-) >>> >>> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c >>> index 47af1ab3bf12..d344baa26de0 100644 >>> --- a/fs/btrfs/zoned.c >>> +++ b/fs/btrfs/zoned.c >>> @@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info) >>> struct btrfs_key key; >>> struct extent_buffer *leaf; >>> struct btrfs_dev_extent *dext; >>> - int ret = 0; >>> - >>> - key.objectid = 1; >> >> Not related to this patch itself, but this immediate number is not that >> straightforward. >> >> In fact for DEV_EXTENT_KEY, the objectid means device number. >> >> For current zoned device support IIRC we only support single device, >> thus it's fixed to 1. > > To be precise, we can have multiple devices, but only support single > profile. So it means we could have cases where we only have devid 2 (device add, then remove the original device). The original code is fine, it's just searching the for the first device extent. But the new code is no longer doing the same thing. My reviewed-by tag is wrong now.... > >> It would be better to have some extra comment/ASSERT() for it. >> >> >>> - key.type = BTRFS_DEV_EXTENT_KEY; >>> - key.offset = 0; >> >> Normally for DEV_EXTENT_KEY, the offset is the dev physical offset, >> which normally is not 0 (as we reserve 0~1M for each device) >> >> So this is a special zoned on-disk format? > > We also reserve the first two zones on a disk for superblock log > zones, so on typical SMR drive, 0-512M is reserved. > > We can use any DEV_EXTENT item here to determine the emulated zone > size. So, it's trying to find the first one. Then the patch is changing the behavior. Now it can't handle cases where we don't have devid 1. And since we're search for the first DEV_EXTENT_KEY, I don't believe btrfs_find_item() is the proper function here to call. Please drop my reviewed-by tag. Thanks, Qu > >> Thanks, >> Qu >> >>> + int ret; >>> >>> path = btrfs_alloc_path(); >>> if (!path) >>> return -ENOMEM; >>> >>> - ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); >>> + ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key); >>> if (ret < 0) >>> goto out; >>> >>> - if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) { >>> - ret = btrfs_next_leaf(root, path); >>> - if (ret < 0) >>> - goto out; >>> - /* No dev extents at all? Not good */ >>> - if (ret > 0) { >>> - ret = -EUCLEAN; >>> - goto out; >>> - } >>> + /* No dev extents at all? Not good */ >>> + else if (ret > 0) { >>> + ret = -EUCLEAN; >>> + goto out; >>> } >>> >>> leaf = path->nodes[0];
On Wed, Aug 04, 2021 at 03:48:50PM -0300, Marcos Paulo de Souza wrote: > calculate_emulated_zone_size can be simplified by using btrfs_find_item, which > executes btrfs_search_slot and calls btrfs_next_leaf if needed. > > No functional changes. > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > --- > fs/btrfs/zoned.c | 21 ++++++--------------- > 1 file changed, 6 insertions(+), 15 deletions(-) > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index 47af1ab3bf12..d344baa26de0 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info) > struct btrfs_key key; > struct extent_buffer *leaf; > struct btrfs_dev_extent *dext; > - int ret = 0; > - > - key.objectid = 1; > - key.type = BTRFS_DEV_EXTENT_KEY; > - key.offset = 0; > + int ret; > > path = btrfs_alloc_path(); > if (!path) > return -ENOMEM; > > - ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); > + ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key); > if (ret < 0) > goto out; > > - if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) { > - ret = btrfs_next_leaf(root, path); > - if (ret < 0) > - goto out; > - /* No dev extents at all? Not good */ > - if (ret > 0) { > - ret = -EUCLEAN; > - goto out; > - } > + /* No dev extents at all? Not good */ > + else if (ret > 0) { > + ret = -EUCLEAN; > + goto out; > } As I wrote in a reply to Qu, we want to find the minimal DEV_EXTENT item here (somewhat like btrfs_verify_dev_extents()). btrfs_find_item() returns 1 if the objectid is different. So, it cause -EUCLEAN when a device with devid = 1 is removed. > > leaf = path->nodes[0]; > -- > 2.31.1 >
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c index 47af1ab3bf12..d344baa26de0 100644 --- a/fs/btrfs/zoned.c +++ b/fs/btrfs/zoned.c @@ -230,29 +230,20 @@ static int calculate_emulated_zone_size(struct btrfs_fs_info *fs_info) struct btrfs_key key; struct extent_buffer *leaf; struct btrfs_dev_extent *dext; - int ret = 0; - - key.objectid = 1; - key.type = BTRFS_DEV_EXTENT_KEY; - key.offset = 0; + int ret; path = btrfs_alloc_path(); if (!path) return -ENOMEM; - ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); + ret = btrfs_find_item(root, path, 1, BTRFS_DEV_EXTENT_KEY, 0, &key); if (ret < 0) goto out; - if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) { - ret = btrfs_next_leaf(root, path); - if (ret < 0) - goto out; - /* No dev extents at all? Not good */ - if (ret > 0) { - ret = -EUCLEAN; - goto out; - } + /* No dev extents at all? Not good */ + else if (ret > 0) { + ret = -EUCLEAN; + goto out; } leaf = path->nodes[0];
calculate_emulated_zone_size can be simplified by using btrfs_find_item, which executes btrfs_search_slot and calls btrfs_next_leaf if needed. No functional changes. Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> --- fs/btrfs/zoned.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)