[PATCH] block: Fix partition check for host-aware zoned block devices

[PATCH] block: Fix partition check for host-aware zoned block devices

[Shin'ichiro Kawasaki]

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(-)

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

Re: [PATCH] block: Fix partition check for host-aware zoned block devices

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH] block: Fix partition check for host-aware zoned block devices

[Christoph Hellwig]

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---

Re: [PATCH] block: Fix partition check for host-aware zoned block devices

[Matthew Wilcox]

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---

Re: [PATCH] block: Fix partition check for host-aware zoned block devices

[Christoph Hellwig]

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.

Re: [PATCH] block: Fix partition check for host-aware zoned block devices

[Shinichiro Kawasaki]

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


-- 
Best Regards,
Shin'ichiro Kawasaki