| Message ID | af6c527cbd8bdc782e50bd33996ee83acc3a16fb.1671221596.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixup uninitialized warnings and enable extra checks | expand |
On Fri, Dec 16, 2022 at 03:15:56PM -0500, Josef Bacik wrote: > There's a special case for loading the device zone info if we have the > zone cache which is a fair bit of code. Extract this out into it's own > helper to clean up the code a little bit, and as a side effect it fixes > an uninitialized error we get with -Wmaybe-uninitialized where it > thought zno may have been uninitialized. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> I'm going to rewrite the code around here with the following WIP branch, to improve the zone caching. https://github.com/naota/linux/commits/feature/zone-cache Specifically, this commit removes the for-loop and the "if (i == *nr_zones)" block you moved in this patch. So, the resulting code will be small enough to keep it there. https://github.com/naota/linux/commit/8d592ac744111bb2f51595a1608beecadb2c5d03 Could you wait for a while for me to clean-up and send the series? I'll also check the series with -Wmaybe-uninitialized. > --- > fs/btrfs/zoned.c | 73 +++++++++++++++++++++++++++++------------------- > 1 file changed, 45 insertions(+), 28 deletions(-) > > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index a759668477bb..f3640ab95e5e 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -216,11 +216,46 @@ static int emulate_report_zones(struct btrfs_device *device, u64 pos, > return i; > } > > +static int load_zones_from_cache(struct btrfs_zoned_device_info *zinfo, u64 pos, > + struct blk_zone *zones, unsigned int *nr_zones) > +{ > + unsigned int i; > + u32 zno; > + > + if (!zinfo->zone_cache) > + return -ENOENT; > + > + ASSERT(IS_ALIGNED(pos, zinfo->zone_size)); > + zno = pos >> zinfo->zone_size_shift; > + > + /* > + * We cannot report zones beyond the zone end. So, it is OK to > + * cap *nr_zones to at the end. > + */ > + *nr_zones = min_t(u32, *nr_zones, zinfo->nr_zones - zno); > + > + for (i = 0; i < *nr_zones; i++) { > + struct blk_zone *zone_info; > + > + zone_info = &zinfo->zone_cache[zno + i]; > + if (!zone_info->len) > + break; > + } > + > + if (i == *nr_zones) { > + /* Cache hit on all the zones */ > + memcpy(zones, zinfo->zone_cache + zno, > + sizeof(*zinfo->zone_cache) * *nr_zones); > + return 0; > + } > + > + return -ENOENT; > +} > + > static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos, > struct blk_zone *zones, unsigned int *nr_zones) > { > struct btrfs_zoned_device_info *zinfo = device->zone_info; > - u32 zno; > int ret; > > if (!*nr_zones) > @@ -233,32 +268,8 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos, > } > > /* Check cache */ > - if (zinfo->zone_cache) { > - unsigned int i; > - > - ASSERT(IS_ALIGNED(pos, zinfo->zone_size)); > - zno = pos >> zinfo->zone_size_shift; > - /* > - * We cannot report zones beyond the zone end. So, it is OK to > - * cap *nr_zones to at the end. > - */ > - *nr_zones = min_t(u32, *nr_zones, zinfo->nr_zones - zno); > - > - for (i = 0; i < *nr_zones; i++) { > - struct blk_zone *zone_info; > - > - zone_info = &zinfo->zone_cache[zno + i]; > - if (!zone_info->len) > - break; > - } > - > - if (i == *nr_zones) { > - /* Cache hit on all the zones */ > - memcpy(zones, zinfo->zone_cache + zno, > - sizeof(*zinfo->zone_cache) * *nr_zones); > - return 0; > - } > - } > + if (!load_zones_from_cache(zinfo, pos, zones, nr_zones)) > + return 0; > > ret = blkdev_report_zones(device->bdev, pos >> SECTOR_SHIFT, *nr_zones, > copy_zone_info_cb, zones); > @@ -274,9 +285,15 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos, > return -EIO; > > /* Populate cache */ > - if (zinfo->zone_cache) > + if (zinfo->zone_cache) { > + u32 zno; > + > + ASSERT(IS_ALIGNED(pos, zinfo->zone_size)); > + zno = pos >> zinfo->zone_size_shift; > + > memcpy(zinfo->zone_cache + zno, zones, > sizeof(*zinfo->zone_cache) * *nr_zones); > + } > > return 0; > } > -- > 2.26.3 >
On Mon, Dec 19, 2022 at 07:05:15AM +0000, Naohiro Aota wrote: > On Fri, Dec 16, 2022 at 03:15:56PM -0500, Josef Bacik wrote: > > There's a special case for loading the device zone info if we have the > > zone cache which is a fair bit of code. Extract this out into it's own > > helper to clean up the code a little bit, and as a side effect it fixes > > an uninitialized error we get with -Wmaybe-uninitialized where it > > thought zno may have been uninitialized. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > I'm going to rewrite the code around here with the following WIP branch, to > improve the zone caching. > > https://github.com/naota/linux/commits/feature/zone-cache > > Specifically, this commit removes the for-loop and the "if (i == > *nr_zones)" block you moved in this patch. So, the resulting code will be > small enough to keep it there. > > https://github.com/naota/linux/commit/8d592ac744111bb2f51595a1608beecadb2c5d03 > > Could you wait for a while for me to clean-up and send the series? I'll > also check the series with -Wmaybe-uninitialized. I'd like to have the warnigs fixes first but if there is a shorter fix that would not move the code then it can go in now and you wouldn't have to redo your changes.
On Tue, Dec 20, 2022 at 08:24:56PM +0100, David Sterba wrote: > On Mon, Dec 19, 2022 at 07:05:15AM +0000, Naohiro Aota wrote: > > On Fri, Dec 16, 2022 at 03:15:56PM -0500, Josef Bacik wrote: > > > There's a special case for loading the device zone info if we have the > > > zone cache which is a fair bit of code. Extract this out into it's own > > > helper to clean up the code a little bit, and as a side effect it fixes > > > an uninitialized error we get with -Wmaybe-uninitialized where it > > > thought zno may have been uninitialized. > > > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > > I'm going to rewrite the code around here with the following WIP branch, to > > improve the zone caching. > > > > https://github.com/naota/linux/commits/feature/zone-cache > > > > Specifically, this commit removes the for-loop and the "if (i == > > *nr_zones)" block you moved in this patch. So, the resulting code will be > > small enough to keep it there. > > > > https://github.com/naota/linux/commit/8d592ac744111bb2f51595a1608beecadb2c5d03 > > > > Could you wait for a while for me to clean-up and send the series? I'll > > also check the series with -Wmaybe-uninitialized. > > I'd like to have the warnigs fixes first but if there is a shorter fix > that would not move the code then it can go in now and you wouldn't have > to redo your changes. Sure, how about this then? From 50bc2858dc58301ca1b7d61bb72ca2566e59032c Mon Sep 17 00:00:00 2001 From: Naohiro Aota <naohiro.aota@wdc.com> Date: Thu, 22 Dec 2022 01:17:59 +0900 Subject: [PATCH] btrfs: zoned: fix uninit warning in btrfs_get_dev_zones Fix an uninitialized error we get with -Wmaybe-uninitialized where it thought zno may have been uninitialized. Since there will be a rewrite of the code, avoid moving the code around and do the small enough fix. Reported-by: Josef Bacik <josef@toxicpanda.com> Link: https://lore.kernel.org/linux-btrfs/af6c527cbd8bdc782e50bd33996ee83acc3a16fb.1671221596.git.josef@toxicpanda.com/ Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- fs/btrfs/zoned.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c index a759668477bb..ab63f8443e0a 100644 --- a/fs/btrfs/zoned.c +++ b/fs/btrfs/zoned.c @@ -220,7 +220,6 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos, struct blk_zone *zones, unsigned int *nr_zones) { struct btrfs_zoned_device_info *zinfo = device->zone_info; - u32 zno; int ret; if (!*nr_zones) @@ -235,6 +234,7 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos, /* Check cache */ if (zinfo->zone_cache) { unsigned int i; + u32 zno; ASSERT(IS_ALIGNED(pos, zinfo->zone_size)); zno = pos >> zinfo->zone_size_shift; @@ -274,9 +274,12 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos, return -EIO; /* Populate cache */ - if (zinfo->zone_cache) + if (zinfo->zone_cache) { + u32 zno = pos >> zinfo->zone_size_shift; + memcpy(zinfo->zone_cache + zno, zones, sizeof(*zinfo->zone_cache) * *nr_zones); + } return 0; }
On Wed, Dec 21, 2022 at 04:47:45PM +0000, Naohiro Aota wrote:
> Sure, how about this then?
That looks good, added to misc-next, thanks.
diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c index a759668477bb..f3640ab95e5e 100644 --- a/fs/btrfs/zoned.c +++ b/fs/btrfs/zoned.c @@ -216,11 +216,46 @@ static int emulate_report_zones(struct btrfs_device *device, u64 pos, return i; } +static int load_zones_from_cache(struct btrfs_zoned_device_info *zinfo, u64 pos, + struct blk_zone *zones, unsigned int *nr_zones) +{ + unsigned int i; + u32 zno; + + if (!zinfo->zone_cache) + return -ENOENT; + + ASSERT(IS_ALIGNED(pos, zinfo->zone_size)); + zno = pos >> zinfo->zone_size_shift; + + /* + * We cannot report zones beyond the zone end. So, it is OK to + * cap *nr_zones to at the end. + */ + *nr_zones = min_t(u32, *nr_zones, zinfo->nr_zones - zno); + + for (i = 0; i < *nr_zones; i++) { + struct blk_zone *zone_info; + + zone_info = &zinfo->zone_cache[zno + i]; + if (!zone_info->len) + break; + } + + if (i == *nr_zones) { + /* Cache hit on all the zones */ + memcpy(zones, zinfo->zone_cache + zno, + sizeof(*zinfo->zone_cache) * *nr_zones); + return 0; + } + + return -ENOENT; +} + static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos, struct blk_zone *zones, unsigned int *nr_zones) { struct btrfs_zoned_device_info *zinfo = device->zone_info; - u32 zno; int ret; if (!*nr_zones) @@ -233,32 +268,8 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos, } /* Check cache */ - if (zinfo->zone_cache) { - unsigned int i; - - ASSERT(IS_ALIGNED(pos, zinfo->zone_size)); - zno = pos >> zinfo->zone_size_shift; - /* - * We cannot report zones beyond the zone end. So, it is OK to - * cap *nr_zones to at the end. - */ - *nr_zones = min_t(u32, *nr_zones, zinfo->nr_zones - zno); - - for (i = 0; i < *nr_zones; i++) { - struct blk_zone *zone_info; - - zone_info = &zinfo->zone_cache[zno + i]; - if (!zone_info->len) - break; - } - - if (i == *nr_zones) { - /* Cache hit on all the zones */ - memcpy(zones, zinfo->zone_cache + zno, - sizeof(*zinfo->zone_cache) * *nr_zones); - return 0; - } - } + if (!load_zones_from_cache(zinfo, pos, zones, nr_zones)) + return 0; ret = blkdev_report_zones(device->bdev, pos >> SECTOR_SHIFT, *nr_zones, copy_zone_info_cb, zones); @@ -274,9 +285,15 @@ static int btrfs_get_dev_zones(struct btrfs_device *device, u64 pos, return -EIO; /* Populate cache */ - if (zinfo->zone_cache) + if (zinfo->zone_cache) { + u32 zno; + + ASSERT(IS_ALIGNED(pos, zinfo->zone_size)); + zno = pos >> zinfo->zone_size_shift; + memcpy(zinfo->zone_cache + zno, zones, sizeof(*zinfo->zone_cache) * *nr_zones); + } return 0; }
There's a special case for loading the device zone info if we have the zone cache which is a fair bit of code. Extract this out into it's own helper to clean up the code a little bit, and as a side effect it fixes an uninitialized error we get with -Wmaybe-uninitialized where it thought zno may have been uninitialized. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/zoned.c | 73 +++++++++++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 28 deletions(-)