| Message ID | 20211015020740.506425-1-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix partition check for host-aware zoned block devices | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Fri, Oct 15, 2021 at 11:07:40AM +0900, Shin'ichiro Kawasaki wrote: > To fix the issues, call the helper function disk_has_partitions() in > place of disk->part_tbl empty check. Since the function was removed with > the commit a33df75c6328, reimplement it to walk through entries in the > xarray disk->part_tbl. Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> Matthew, we talked about possiblig adding a xa_nr_entries helper a while ago. This would be a good place for it, as we could just check xa_nr_entries() > 1 for example. > > Fixes: a33df75c6328 ("block: use an xarray for disk->part_tbl") > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Cc: stable@vger.kernel.org # v5.14+ > --- > block/blk-settings.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/block/blk-settings.c b/block/blk-settings.c > index a7c857ad7d10..b880c70e22e4 100644 > --- a/block/blk-settings.c > +++ b/block/blk-settings.c > @@ -842,6 +842,24 @@ bool blk_queue_can_use_dma_map_merging(struct request_queue *q, > } > EXPORT_SYMBOL_GPL(blk_queue_can_use_dma_map_merging); > > +static bool disk_has_partitions(struct gendisk *disk) > +{ > + unsigned long idx; > + struct block_device *part; > + bool ret = false; > + > + rcu_read_lock(); > + xa_for_each(&disk->part_tbl, idx, part) { > + if (bdev_is_partition(part)) { > + ret = true; > + break; > + } > + } > + rcu_read_unlock(); > + > + return ret; > +} > + > /** > * blk_queue_set_zoned - configure a disk queue zoned model. > * @disk: the gendisk of the queue to configure > @@ -876,7 +894,7 @@ void blk_queue_set_zoned(struct gendisk *disk, enum blk_zoned_model model) > * we do nothing special as far as the block layer is concerned. > */ > if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) || > - !xa_empty(&disk->part_tbl)) > + disk_has_partitions(disk)) > model = BLK_ZONED_NONE; > break; > case BLK_ZONED_NONE: > -- > 2.31.1 ---end quoted text---
On Sat, Oct 16, 2021 at 06:34:50AM +0200, Christoph Hellwig wrote: > On Fri, Oct 15, 2021 at 11:07:40AM +0900, Shin'ichiro Kawasaki wrote: > > To fix the issues, call the helper function disk_has_partitions() in > > place of disk->part_tbl empty check. Since the function was removed with > > the commit a33df75c6328, reimplement it to walk through entries in the > > xarray disk->part_tbl. > > Looks good, > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Matthew, > > we talked about possiblig adding a xa_nr_entries helper a while ago. > This would be a good place for it, as we could just check > xa_nr_entries() > 1 for example. Do I understand the problem correctly, that you don't actually want to know whether there's more than one entry in the array, but rather that there's an entry at an index other than 0? If so, that's an easy question to answer, we just don't have a helper for it yet. Something like this should do: static inline bool xa_is_trivial(const struct xarray *xa) { void *entry = READ_ONCE(xa->xa_head); return entry || !xa_is_node(entry); } Probably needs a better name than that. > > > > Fixes: a33df75c6328 ("block: use an xarray for disk->part_tbl") > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > Cc: stable@vger.kernel.org # v5.14+ > > --- > > block/blk-settings.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/block/blk-settings.c b/block/blk-settings.c > > index a7c857ad7d10..b880c70e22e4 100644 > > --- a/block/blk-settings.c > > +++ b/block/blk-settings.c > > @@ -842,6 +842,24 @@ bool blk_queue_can_use_dma_map_merging(struct request_queue *q, > > } > > EXPORT_SYMBOL_GPL(blk_queue_can_use_dma_map_merging); > > > > +static bool disk_has_partitions(struct gendisk *disk) > > +{ > > + unsigned long idx; > > + struct block_device *part; > > + bool ret = false; > > + > > + rcu_read_lock(); > > + xa_for_each(&disk->part_tbl, idx, part) { > > + if (bdev_is_partition(part)) { > > + ret = true; > > + break; > > + } > > + } > > + rcu_read_unlock(); > > + > > + return ret; > > +} > > + > > /** > > * blk_queue_set_zoned - configure a disk queue zoned model. > > * @disk: the gendisk of the queue to configure > > @@ -876,7 +894,7 @@ void blk_queue_set_zoned(struct gendisk *disk, enum blk_zoned_model model) > > * we do nothing special as far as the block layer is concerned. > > */ > > if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) || > > - !xa_empty(&disk->part_tbl)) > > + disk_has_partitions(disk)) > > model = BLK_ZONED_NONE; > > break; > > case BLK_ZONED_NONE: > > -- > > 2.31.1 > ---end quoted text---
On Sat, Oct 16, 2021 at 03:26:18PM +0100, Matthew Wilcox wrote: > Do I understand the problem correctly, that you don't actually want to > know whether there's more than one entry in the array, but rather that > there's an entry at an index other than 0? In this case both checks are equivalemt because index 0 is always present once the disk is life.
On Oct 16, 2021 / 15:26, Matthew Wilcox wrote: > On Sat, Oct 16, 2021 at 06:34:50AM +0200, Christoph Hellwig wrote: > > On Fri, Oct 15, 2021 at 11:07:40AM +0900, Shin'ichiro Kawasaki wrote: > > > To fix the issues, call the helper function disk_has_partitions() in > > > place of disk->part_tbl empty check. Since the function was removed with > > > the commit a33df75c6328, reimplement it to walk through entries in the > > > xarray disk->part_tbl. > > > > Looks good, > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > Matthew, > > > > we talked about possiblig adding a xa_nr_entries helper a while ago. > > This would be a good place for it, as we could just check > > xa_nr_entries() > 1 for example. > > Do I understand the problem correctly, that you don't actually want to > know whether there's more than one entry in the array, but rather that > there's an entry at an index other than 0? > > If so, that's an easy question to answer, we just don't have a helper > for it yet. Something like this should do: > > static inline bool xa_is_trivial(const struct xarray *xa) > { > void *entry = READ_ONCE(xa->xa_head); > > return entry || !xa_is_node(entry); > } > > Probably needs a better name than that. Thanks for the discussion. Based on the code above, I tried following hunk below, and confirmed the new helper function can be used to fix the issue I found. Good. To make it work, I needed to change the logical operator in the function from OR to AND. As for the function name, my mere suggestion is xa_has_single_entry(), but this may not be the best. I would like to ask advice on the next action for the fix. If my original patch can go to upstream first, I think the changes for this new xarray helper can be done later. This approach would be good if the new helper will not be propagated to the stable branches. Another approach is to do both of this new helper introduction and the issue fix at this moment. Which approach is the better? diff --git a/include/linux/xarray.h b/include/linux/xarray.h index a91e3d90df8a..7a31c9423d01 100644 --- a/include/linux/xarray.h +++ b/include/linux/xarray.h @@ -1238,6 +1238,20 @@ static inline unsigned long xa_to_sibling(const void *entry) return xa_to_internal(entry); } +/** + * xa_has_single_entry() - Does it have an entry at an index other than 0? + * @entry: XArray entry. + * + * Context: Any context. + * Return: %true if there is no entry at an index other than 0. + */ +static inline bool xa_has_single_entry(const struct xarray *xa) +{ + const void *entry = READ_ONCE(xa->xa_head); + + return entry && !xa_is_node(entry); +} + /** * xa_is_sibling() - Is the entry a sibling entry? * @entry: Entry retrieved from the XArray
diff --git a/block/blk-settings.c b/block/blk-settings.c index a7c857ad7d10..b880c70e22e4 100644 --- a/block/blk-settings.c +++ b/block/blk-settings.c @@ -842,6 +842,24 @@ bool blk_queue_can_use_dma_map_merging(struct request_queue *q, } EXPORT_SYMBOL_GPL(blk_queue_can_use_dma_map_merging); +static bool disk_has_partitions(struct gendisk *disk) +{ + unsigned long idx; + struct block_device *part; + bool ret = false; + + rcu_read_lock(); + xa_for_each(&disk->part_tbl, idx, part) { + if (bdev_is_partition(part)) { + ret = true; + break; + } + } + rcu_read_unlock(); + + return ret; +} + /** * blk_queue_set_zoned - configure a disk queue zoned model. * @disk: the gendisk of the queue to configure @@ -876,7 +894,7 @@ void blk_queue_set_zoned(struct gendisk *disk, enum blk_zoned_model model) * we do nothing special as far as the block layer is concerned. */ if (!IS_ENABLED(CONFIG_BLK_DEV_ZONED) || - !xa_empty(&disk->part_tbl)) + disk_has_partitions(disk)) model = BLK_ZONED_NONE; break; case BLK_ZONED_NONE:
Commit a33df75c6328 ("block: use an xarray for disk->part_tbl") modified the method to check partition existence in host-aware zoned block devices from disk_has_partitions() helper function call to empty check of xarray disk->part_tbl. However, disk->part_tbl always has single entry for disk->part0 and never becomes empty. This resulted in the host-aware zoned devices always judged to have partitions, and it made the sysfs queue/zoned attribute to be "none" instead of "host-aware" regardless of partition existence in the devices. This also caused DEBUG_LOCKS_WARN_ON(lock->magic != lock) for sdkp->rev_mutex in scsi layer when the kernel detects host-aware zoned device. Since block layer handled the host-aware zoned devices as non- zoned devices, scsi layer did not have chance to initialize the mutex for zone revalidation. Therefore, the warning was triggered. To fix the issues, call the helper function disk_has_partitions() in place of disk->part_tbl empty check. Since the function was removed with the commit a33df75c6328, reimplement it to walk through entries in the xarray disk->part_tbl. Fixes: a33df75c6328 ("block: use an xarray for disk->part_tbl") Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Cc: stable@vger.kernel.org # v5.14+ --- block/blk-settings.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)