[dm-devel] [PATCH] dm: fix iterate_device sanity check

[dm-devel] [PATCH] dm: fix iterate_device sanity check

[Jeffle Xu]

According to the definition of dm_iterate_devices_fn:
 * This function must iterate through each section of device used by the
 * target until it encounters a non-zero return code, which it then returns.
 * Returns zero if no callout returned non-zero.

For some target type (e.g., dm-stripe), one call of iterate_devices() may
iterate multiple underlying devices internally, in which case a non-zero
return code returned by iterate_devices_callout_fn will stop the iteration
in advance.

Thus if we want to ensure that _all_ underlying devices support some kind of
attribute, the iteration structure like dm_table_supports_nowait() should be
used, while the input iterate_devices_callout_fn should handle the 'not
support' semantics. On the opposite, the iteration structure like
dm_table_any_device_attribute() should be used if _any_ underlying device
supporting this attibute is sufficient. In this case, the input
iterate_devices_callout_fn should handle the 'support' semantics.

Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
Cc: stable@vger.kernel.org
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 drivers/md/dm-table.c | 84 ++++++++++++++++++++++---------------------
 drivers/md/dm.c       |  2 +-
 drivers/md/dm.h       |  2 +-
 3 files changed, 46 insertions(+), 42 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 4acf2342f7ad..53dcbf75eda9 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -820,24 +820,24 @@ void dm_table_set_type(struct dm_table *t, enum dm_queue_mode type)
 EXPORT_SYMBOL_GPL(dm_table_set_type);
 
 /* validate the dax capability of the target device span */
-int device_supports_dax(struct dm_target *ti, struct dm_dev *dev,
+int device_not_dax_capable(struct dm_target *ti, struct dm_dev *dev,
 			sector_t start, sector_t len, void *data)
 {
 	int blocksize = *(int *) data, id;
 	bool rc;
 
 	id = dax_read_lock();
-	rc = dax_supported(dev->dax_dev, dev->bdev, blocksize, start, len);
+	rc = !dax_supported(dev->dax_dev, dev->bdev, blocksize, start, len);
 	dax_read_unlock(id);
 
 	return rc;
 }
 
 /* Check devices support synchronous DAX */
-static int device_dax_synchronous(struct dm_target *ti, struct dm_dev *dev,
+static int device_not_dax_synchronous_capable(struct dm_target *ti, struct dm_dev *dev,
 				  sector_t start, sector_t len, void *data)
 {
-	return dev->dax_dev && dax_synchronous(dev->dax_dev);
+	return !dev->dax_dev || !dax_synchronous(dev->dax_dev);
 }
 
 bool dm_table_supports_dax(struct dm_table *t,
@@ -854,7 +854,7 @@ bool dm_table_supports_dax(struct dm_table *t,
 			return false;
 
 		if (!ti->type->iterate_devices ||
-		    !ti->type->iterate_devices(ti, iterate_fn, blocksize))
+		    ti->type->iterate_devices(ti, iterate_fn, blocksize))
 			return false;
 	}
 
@@ -925,7 +925,7 @@ static int dm_table_determine_type(struct dm_table *t)
 verify_bio_based:
 		/* We must use this table as bio-based */
 		t->type = DM_TYPE_BIO_BASED;
-		if (dm_table_supports_dax(t, device_supports_dax, &page_size) ||
+		if (dm_table_supports_dax(t, device_not_dax_capable, &page_size) ||
 		    (list_empty(devices) && live_md_type == DM_TYPE_DAX_BIO_BASED)) {
 			t->type = DM_TYPE_DAX_BIO_BASED;
 		}
@@ -1595,12 +1595,12 @@ static int dm_table_supports_dax_write_cache(struct dm_table *t)
 	return false;
 }
 
-static int device_is_nonrot(struct dm_target *ti, struct dm_dev *dev,
+static int device_is_rot(struct dm_target *ti, struct dm_dev *dev,
 			    sector_t start, sector_t len, void *data)
 {
 	struct request_queue *q = bdev_get_queue(dev->bdev);
 
-	return q && blk_queue_nonrot(q);
+	return q && !blk_queue_nonrot(q);
 }
 
 static int device_is_not_random(struct dm_target *ti, struct dm_dev *dev,
@@ -1611,8 +1611,8 @@ static int device_is_not_random(struct dm_target *ti, struct dm_dev *dev,
 	return q && !blk_queue_add_random(q);
 }
 
-static bool dm_table_all_devices_attribute(struct dm_table *t,
-					   iterate_devices_callout_fn func)
+static bool dm_table_any_device_attribute(struct dm_table *t,
+					  iterate_devices_callout_fn func)
 {
 	struct dm_target *ti;
 	unsigned i;
@@ -1620,12 +1620,12 @@ static bool dm_table_all_devices_attribute(struct dm_table *t,
 	for (i = 0; i < dm_table_get_num_targets(t); i++) {
 		ti = dm_table_get_target(t, i);
 
-		if (!ti->type->iterate_devices ||
-		    !ti->type->iterate_devices(ti, func, NULL))
-			return false;
+		if (ti->type->iterate_devices &&
+		    ti->type->iterate_devices(ti, func, NULL))
+			return true;
 	}
 
-	return true;
+	return false;
 }
 
 static int device_not_write_same_capable(struct dm_target *ti, struct dm_dev *dev,
@@ -1780,26 +1780,25 @@ static int device_requires_stable_pages(struct dm_target *ti,
 }
 
 /*
- * If any underlying device requires stable pages, a table must require
- * them as well.  Only targets that support iterate_devices are considered:
- * don't want error, zero, etc to require stable pages.
+ * type->iterate_devices() should be called when the sanity check needs to
+ * iterate and check all underlying data devices. iterate_devices() will
+ * iterate all underlying data devices until it encounters a non-zero return
+ * code, returned by whether the input iterate_devices_callout_fn, or
+ * iterate_devices() itself internally.
+ *
+ * For some target type (e.g., dm-stripe), one call of iterate_devices() may
+ * iterate multiple underlying devices internally, in which case a non-zero
+ * return code returned by iterate_devices_callout_fn will stop the iteration
+ * in advance.
+ *
+ * Thus if we want to ensure that _all_ underlying devices support some kind of
+ * attribute, the iteration structure like dm_table_supports_nowait() should be
+ * used, while the input iterate_devices_callout_fn should handle the 'not
+ * support' semantics. On the opposite, the iteration structure like
+ * dm_table_any_device_attribute() should be used if _any_ underlying device
+ * supporting this attibute is sufficient. In this case, the input
+ * iterate_devices_callout_fn should handle the 'support' semantics.
  */
-static bool dm_table_requires_stable_pages(struct dm_table *t)
-{
-	struct dm_target *ti;
-	unsigned i;
-
-	for (i = 0; i < dm_table_get_num_targets(t); i++) {
-		ti = dm_table_get_target(t, i);
-
-		if (ti->type->iterate_devices &&
-		    ti->type->iterate_devices(ti, device_requires_stable_pages, NULL))
-			return true;
-	}
-
-	return false;
-}
-
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 			       struct queue_limits *limits)
 {
@@ -1837,9 +1836,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	}
 	blk_queue_write_cache(q, wc, fua);
 
-	if (dm_table_supports_dax(t, device_supports_dax, &page_size)) {
+	if (dm_table_supports_dax(t, device_not_dax_capable, &page_size)) {
 		blk_queue_flag_set(QUEUE_FLAG_DAX, q);
-		if (dm_table_supports_dax(t, device_dax_synchronous, NULL))
+		if (dm_table_supports_dax(t, device_not_dax_synchronous_capable, NULL))
 			set_dax_synchronous(t->md->dax_dev);
 	}
 	else
@@ -1849,10 +1848,10 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 		dax_write_cache(t->md->dax_dev, true);
 
 	/* Ensure that all underlying devices are non-rotational. */
-	if (dm_table_all_devices_attribute(t, device_is_nonrot))
-		blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
-	else
+	if (dm_table_any_device_attribute(t, device_is_rot))
 		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
+	else
+		blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
 
 	if (!dm_table_supports_write_same(t))
 		q->limits.max_write_same_sectors = 0;
@@ -1864,8 +1863,11 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	/*
 	 * Some devices don't use blk_integrity but still want stable pages
 	 * because they do their own checksumming.
+	 * If any underlying device requires stable pages, a table must require
+	 * them as well.  Only targets that support iterate_devices are considered:
+	 * don't want error, zero, etc to require stable pages.
 	 */
-	if (dm_table_requires_stable_pages(t))
+	if (dm_table_any_device_attribute(t, device_requires_stable_pages))
 		blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, q);
 	else
 		blk_queue_flag_clear(QUEUE_FLAG_STABLE_WRITES, q);
@@ -1876,8 +1878,10 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	 * Clear QUEUE_FLAG_ADD_RANDOM if any underlying device does not
 	 * have it set.
 	 */
-	if (blk_queue_add_random(q) && dm_table_all_devices_attribute(t, device_is_not_random))
+	if (dm_table_any_device_attribute(t, device_is_not_random))
 		blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, q);
+	else
+		blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);
 
 	/*
 	 * For a zoned target, the number of zones should be updated for the
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7bac564f3faa..8a3d73efb9dd 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1128,7 +1128,7 @@ static bool dm_dax_supported(struct dax_device *dax_dev, struct block_device *bd
 	if (!map)
 		goto out;
 
-	ret = dm_table_supports_dax(map, device_supports_dax, &blocksize);
+	ret = dm_table_supports_dax(map, device_not_dax_capable, &blocksize);
 
 out:
 	dm_put_live_table(md, srcu_idx);
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
index fffe1e289c53..b441ad772c18 100644
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -73,7 +73,7 @@ void dm_table_free_md_mempools(struct dm_table *t);
 struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
 bool dm_table_supports_dax(struct dm_table *t, iterate_devices_callout_fn fn,
 			   int *blocksize);
-int device_supports_dax(struct dm_target *ti, struct dm_dev *dev,
+int device_not_dax_capable(struct dm_target *ti, struct dm_dev *dev,
 			   sector_t start, sector_t len, void *data);
 
 void dm_lock_md_type(struct mapped_device *md);
-- 
2.27.0

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] dm: fix iterate_device sanity check

[JeffleXu]

dm_table_supports_dax_write_cache() is remained untouched, since I'm not
sure if the semantics requires that 'any underlying device' or 'all
underlying devices' supporting dax_write_cache. At least it seems that
'any underlying device' is sufficient from the current code.

On 2/2/21 11:35 AM, Jeffle Xu wrote:
> According to the definition of dm_iterate_devices_fn:
>  * This function must iterate through each section of device used by the
>  * target until it encounters a non-zero return code, which it then returns.
>  * Returns zero if no callout returned non-zero.
> 
> For some target type (e.g., dm-stripe), one call of iterate_devices() may
> iterate multiple underlying devices internally, in which case a non-zero
> return code returned by iterate_devices_callout_fn will stop the iteration
> in advance.
> 
> Thus if we want to ensure that _all_ underlying devices support some kind of
> attribute, the iteration structure like dm_table_supports_nowait() should be
> used, while the input iterate_devices_callout_fn should handle the 'not
> support' semantics. On the opposite, the iteration structure like
> dm_table_any_device_attribute() should be used if _any_ underlying device
> supporting this attibute is sufficient. In this case, the input
> iterate_devices_callout_fn should handle the 'support' semantics.
> 
> Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
> Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
> Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  drivers/md/dm-table.c | 84 ++++++++++++++++++++++---------------------
>  drivers/md/dm.c       |  2 +-
>  drivers/md/dm.h       |  2 +-
>  3 files changed, 46 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 4acf2342f7ad..53dcbf75eda9 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -820,24 +820,24 @@ void dm_table_set_type(struct dm_table *t, enum dm_queue_mode type)
>  EXPORT_SYMBOL_GPL(dm_table_set_type);
>  
>  /* validate the dax capability of the target device span */
> -int device_supports_dax(struct dm_target *ti, struct dm_dev *dev,
> +int device_not_dax_capable(struct dm_target *ti, struct dm_dev *dev,
>  			sector_t start, sector_t len, void *data)
>  {
>  	int blocksize = *(int *) data, id;
>  	bool rc;
>  
>  	id = dax_read_lock();
> -	rc = dax_supported(dev->dax_dev, dev->bdev, blocksize, start, len);
> +	rc = !dax_supported(dev->dax_dev, dev->bdev, blocksize, start, len);
>  	dax_read_unlock(id);
>  
>  	return rc;
>  }
>  
>  /* Check devices support synchronous DAX */
> -static int device_dax_synchronous(struct dm_target *ti, struct dm_dev *dev,
> +static int device_not_dax_synchronous_capable(struct dm_target *ti, struct dm_dev *dev,
>  				  sector_t start, sector_t len, void *data)
>  {
> -	return dev->dax_dev && dax_synchronous(dev->dax_dev);
> +	return !dev->dax_dev || !dax_synchronous(dev->dax_dev);
>  }
>  
>  bool dm_table_supports_dax(struct dm_table *t,
> @@ -854,7 +854,7 @@ bool dm_table_supports_dax(struct dm_table *t,
>  			return false;
>  
>  		if (!ti->type->iterate_devices ||
> -		    !ti->type->iterate_devices(ti, iterate_fn, blocksize))
> +		    ti->type->iterate_devices(ti, iterate_fn, blocksize))
>  			return false;
>  	}
>  
> @@ -925,7 +925,7 @@ static int dm_table_determine_type(struct dm_table *t)
>  verify_bio_based:
>  		/* We must use this table as bio-based */
>  		t->type = DM_TYPE_BIO_BASED;
> -		if (dm_table_supports_dax(t, device_supports_dax, &page_size) ||
> +		if (dm_table_supports_dax(t, device_not_dax_capable, &page_size) ||
>  		    (list_empty(devices) && live_md_type == DM_TYPE_DAX_BIO_BASED)) {
>  			t->type = DM_TYPE_DAX_BIO_BASED;
>  		}
> @@ -1595,12 +1595,12 @@ static int dm_table_supports_dax_write_cache(struct dm_table *t)
>  	return false;
>  }
>  
> -static int device_is_nonrot(struct dm_target *ti, struct dm_dev *dev,
> +static int device_is_rot(struct dm_target *ti, struct dm_dev *dev,
>  			    sector_t start, sector_t len, void *data)
>  {
>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>  
> -	return q && blk_queue_nonrot(q);
> +	return q && !blk_queue_nonrot(q);
>  }
>  
>  static int device_is_not_random(struct dm_target *ti, struct dm_dev *dev,
> @@ -1611,8 +1611,8 @@ static int device_is_not_random(struct dm_target *ti, struct dm_dev *dev,
>  	return q && !blk_queue_add_random(q);
>  }
>  
> -static bool dm_table_all_devices_attribute(struct dm_table *t,
> -					   iterate_devices_callout_fn func)
> +static bool dm_table_any_device_attribute(struct dm_table *t,
> +					  iterate_devices_callout_fn func)
>  {
>  	struct dm_target *ti;
>  	unsigned i;
> @@ -1620,12 +1620,12 @@ static bool dm_table_all_devices_attribute(struct dm_table *t,
>  	for (i = 0; i < dm_table_get_num_targets(t); i++) {
>  		ti = dm_table_get_target(t, i);
>  
> -		if (!ti->type->iterate_devices ||
> -		    !ti->type->iterate_devices(ti, func, NULL))
> -			return false;
> +		if (ti->type->iterate_devices &&
> +		    ti->type->iterate_devices(ti, func, NULL))
> +			return true;
>  	}
>  
> -	return true;
> +	return false;
>  }
>  
>  static int device_not_write_same_capable(struct dm_target *ti, struct dm_dev *dev,
> @@ -1780,26 +1780,25 @@ static int device_requires_stable_pages(struct dm_target *ti,
>  }
>  
>  /*
> - * If any underlying device requires stable pages, a table must require
> - * them as well.  Only targets that support iterate_devices are considered:
> - * don't want error, zero, etc to require stable pages.
> + * type->iterate_devices() should be called when the sanity check needs to
> + * iterate and check all underlying data devices. iterate_devices() will
> + * iterate all underlying data devices until it encounters a non-zero return
> + * code, returned by whether the input iterate_devices_callout_fn, or
> + * iterate_devices() itself internally.
> + *
> + * For some target type (e.g., dm-stripe), one call of iterate_devices() may
> + * iterate multiple underlying devices internally, in which case a non-zero
> + * return code returned by iterate_devices_callout_fn will stop the iteration
> + * in advance.
> + *
> + * Thus if we want to ensure that _all_ underlying devices support some kind of
> + * attribute, the iteration structure like dm_table_supports_nowait() should be
> + * used, while the input iterate_devices_callout_fn should handle the 'not
> + * support' semantics. On the opposite, the iteration structure like
> + * dm_table_any_device_attribute() should be used if _any_ underlying device
> + * supporting this attibute is sufficient. In this case, the input
> + * iterate_devices_callout_fn should handle the 'support' semantics.
>   */
> -static bool dm_table_requires_stable_pages(struct dm_table *t)
> -{
> -	struct dm_target *ti;
> -	unsigned i;
> -
> -	for (i = 0; i < dm_table_get_num_targets(t); i++) {
> -		ti = dm_table_get_target(t, i);
> -
> -		if (ti->type->iterate_devices &&
> -		    ti->type->iterate_devices(ti, device_requires_stable_pages, NULL))
> -			return true;
> -	}
> -
> -	return false;
> -}
> -
>  void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  			       struct queue_limits *limits)
>  {
> @@ -1837,9 +1836,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  	}
>  	blk_queue_write_cache(q, wc, fua);
>  
> -	if (dm_table_supports_dax(t, device_supports_dax, &page_size)) {
> +	if (dm_table_supports_dax(t, device_not_dax_capable, &page_size)) {
>  		blk_queue_flag_set(QUEUE_FLAG_DAX, q);
> -		if (dm_table_supports_dax(t, device_dax_synchronous, NULL))
> +		if (dm_table_supports_dax(t, device_not_dax_synchronous_capable, NULL))
>  			set_dax_synchronous(t->md->dax_dev);
>  	}
>  	else
> @@ -1849,10 +1848,10 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  		dax_write_cache(t->md->dax_dev, true);
>  
>  	/* Ensure that all underlying devices are non-rotational. */
> -	if (dm_table_all_devices_attribute(t, device_is_nonrot))
> -		blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
> -	else
> +	if (dm_table_any_device_attribute(t, device_is_rot))
>  		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
> +	else
> +		blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
>  
>  	if (!dm_table_supports_write_same(t))
>  		q->limits.max_write_same_sectors = 0;
> @@ -1864,8 +1863,11 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  	/*
>  	 * Some devices don't use blk_integrity but still want stable pages
>  	 * because they do their own checksumming.
> +	 * If any underlying device requires stable pages, a table must require
> +	 * them as well.  Only targets that support iterate_devices are considered:
> +	 * don't want error, zero, etc to require stable pages.
>  	 */
> -	if (dm_table_requires_stable_pages(t))
> +	if (dm_table_any_device_attribute(t, device_requires_stable_pages))
>  		blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, q);
>  	else
>  		blk_queue_flag_clear(QUEUE_FLAG_STABLE_WRITES, q);
> @@ -1876,8 +1878,10 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  	 * Clear QUEUE_FLAG_ADD_RANDOM if any underlying device does not
>  	 * have it set.
>  	 */
> -	if (blk_queue_add_random(q) && dm_table_all_devices_attribute(t, device_is_not_random))
> +	if (dm_table_any_device_attribute(t, device_is_not_random))
>  		blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, q);
> +	else
> +		blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);
>  
>  	/*
>  	 * For a zoned target, the number of zones should be updated for the
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 7bac564f3faa..8a3d73efb9dd 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1128,7 +1128,7 @@ static bool dm_dax_supported(struct dax_device *dax_dev, struct block_device *bd
>  	if (!map)
>  		goto out;
>  
> -	ret = dm_table_supports_dax(map, device_supports_dax, &blocksize);
> +	ret = dm_table_supports_dax(map, device_not_dax_capable, &blocksize);
>  
>  out:
>  	dm_put_live_table(md, srcu_idx);
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index fffe1e289c53..b441ad772c18 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -73,7 +73,7 @@ void dm_table_free_md_mempools(struct dm_table *t);
>  struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
>  bool dm_table_supports_dax(struct dm_table *t, iterate_devices_callout_fn fn,
>  			   int *blocksize);
> -int device_supports_dax(struct dm_target *ti, struct dm_dev *dev,
> +int device_not_dax_capable(struct dm_target *ti, struct dm_dev *dev,
>  			   sector_t start, sector_t len, void *data);
>  
>  void dm_lock_md_type(struct mapped_device *md);
> 

-- 
Thanks,
Jeffle

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] dm: fix iterate_device sanity check

[JeffleXu]

Any comment?

Thanks,
Jeffle


On 2/2/21 11:35 AM, Jeffle Xu wrote:
> According to the definition of dm_iterate_devices_fn:
>  * This function must iterate through each section of device used by the
>  * target until it encounters a non-zero return code, which it then returns.
>  * Returns zero if no callout returned non-zero.
> 
> For some target type (e.g., dm-stripe), one call of iterate_devices() may
> iterate multiple underlying devices internally, in which case a non-zero
> return code returned by iterate_devices_callout_fn will stop the iteration
> in advance.
> 
> Thus if we want to ensure that _all_ underlying devices support some kind of
> attribute, the iteration structure like dm_table_supports_nowait() should be
> used, while the input iterate_devices_callout_fn should handle the 'not
> support' semantics. On the opposite, the iteration structure like
> dm_table_any_device_attribute() should be used if _any_ underlying device
> supporting this attibute is sufficient. In this case, the input
> iterate_devices_callout_fn should handle the 'support' semantics.
> 
> Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
> Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
> Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  drivers/md/dm-table.c | 84 ++++++++++++++++++++++---------------------
>  drivers/md/dm.c       |  2 +-
>  drivers/md/dm.h       |  2 +-
>  3 files changed, 46 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 4acf2342f7ad..53dcbf75eda9 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -820,24 +820,24 @@ void dm_table_set_type(struct dm_table *t, enum dm_queue_mode type)
>  EXPORT_SYMBOL_GPL(dm_table_set_type);
>  
>  /* validate the dax capability of the target device span */
> -int device_supports_dax(struct dm_target *ti, struct dm_dev *dev,
> +int device_not_dax_capable(struct dm_target *ti, struct dm_dev *dev,
>  			sector_t start, sector_t len, void *data)
>  {
>  	int blocksize = *(int *) data, id;
>  	bool rc;
>  
>  	id = dax_read_lock();
> -	rc = dax_supported(dev->dax_dev, dev->bdev, blocksize, start, len);
> +	rc = !dax_supported(dev->dax_dev, dev->bdev, blocksize, start, len);
>  	dax_read_unlock(id);
>  
>  	return rc;
>  }
>  
>  /* Check devices support synchronous DAX */
> -static int device_dax_synchronous(struct dm_target *ti, struct dm_dev *dev,
> +static int device_not_dax_synchronous_capable(struct dm_target *ti, struct dm_dev *dev,
>  				  sector_t start, sector_t len, void *data)
>  {
> -	return dev->dax_dev && dax_synchronous(dev->dax_dev);
> +	return !dev->dax_dev || !dax_synchronous(dev->dax_dev);
>  }
>  
>  bool dm_table_supports_dax(struct dm_table *t,
> @@ -854,7 +854,7 @@ bool dm_table_supports_dax(struct dm_table *t,
>  			return false;
>  
>  		if (!ti->type->iterate_devices ||
> -		    !ti->type->iterate_devices(ti, iterate_fn, blocksize))
> +		    ti->type->iterate_devices(ti, iterate_fn, blocksize))
>  			return false;
>  	}
>  
> @@ -925,7 +925,7 @@ static int dm_table_determine_type(struct dm_table *t)
>  verify_bio_based:
>  		/* We must use this table as bio-based */
>  		t->type = DM_TYPE_BIO_BASED;
> -		if (dm_table_supports_dax(t, device_supports_dax, &page_size) ||
> +		if (dm_table_supports_dax(t, device_not_dax_capable, &page_size) ||
>  		    (list_empty(devices) && live_md_type == DM_TYPE_DAX_BIO_BASED)) {
>  			t->type = DM_TYPE_DAX_BIO_BASED;
>  		}
> @@ -1595,12 +1595,12 @@ static int dm_table_supports_dax_write_cache(struct dm_table *t)
>  	return false;
>  }
>  
> -static int device_is_nonrot(struct dm_target *ti, struct dm_dev *dev,
> +static int device_is_rot(struct dm_target *ti, struct dm_dev *dev,
>  			    sector_t start, sector_t len, void *data)
>  {
>  	struct request_queue *q = bdev_get_queue(dev->bdev);
>  
> -	return q && blk_queue_nonrot(q);
> +	return q && !blk_queue_nonrot(q);
>  }
>  
>  static int device_is_not_random(struct dm_target *ti, struct dm_dev *dev,
> @@ -1611,8 +1611,8 @@ static int device_is_not_random(struct dm_target *ti, struct dm_dev *dev,
>  	return q && !blk_queue_add_random(q);
>  }
>  
> -static bool dm_table_all_devices_attribute(struct dm_table *t,
> -					   iterate_devices_callout_fn func)
> +static bool dm_table_any_device_attribute(struct dm_table *t,
> +					  iterate_devices_callout_fn func)
>  {
>  	struct dm_target *ti;
>  	unsigned i;
> @@ -1620,12 +1620,12 @@ static bool dm_table_all_devices_attribute(struct dm_table *t,
>  	for (i = 0; i < dm_table_get_num_targets(t); i++) {
>  		ti = dm_table_get_target(t, i);
>  
> -		if (!ti->type->iterate_devices ||
> -		    !ti->type->iterate_devices(ti, func, NULL))
> -			return false;
> +		if (ti->type->iterate_devices &&
> +		    ti->type->iterate_devices(ti, func, NULL))
> +			return true;
>  	}
>  
> -	return true;
> +	return false;
>  }
>  
>  static int device_not_write_same_capable(struct dm_target *ti, struct dm_dev *dev,
> @@ -1780,26 +1780,25 @@ static int device_requires_stable_pages(struct dm_target *ti,
>  }
>  
>  /*
> - * If any underlying device requires stable pages, a table must require
> - * them as well.  Only targets that support iterate_devices are considered:
> - * don't want error, zero, etc to require stable pages.
> + * type->iterate_devices() should be called when the sanity check needs to
> + * iterate and check all underlying data devices. iterate_devices() will
> + * iterate all underlying data devices until it encounters a non-zero return
> + * code, returned by whether the input iterate_devices_callout_fn, or
> + * iterate_devices() itself internally.
> + *
> + * For some target type (e.g., dm-stripe), one call of iterate_devices() may
> + * iterate multiple underlying devices internally, in which case a non-zero
> + * return code returned by iterate_devices_callout_fn will stop the iteration
> + * in advance.
> + *
> + * Thus if we want to ensure that _all_ underlying devices support some kind of
> + * attribute, the iteration structure like dm_table_supports_nowait() should be
> + * used, while the input iterate_devices_callout_fn should handle the 'not
> + * support' semantics. On the opposite, the iteration structure like
> + * dm_table_any_device_attribute() should be used if _any_ underlying device
> + * supporting this attibute is sufficient. In this case, the input
> + * iterate_devices_callout_fn should handle the 'support' semantics.
>   */
> -static bool dm_table_requires_stable_pages(struct dm_table *t)
> -{
> -	struct dm_target *ti;
> -	unsigned i;
> -
> -	for (i = 0; i < dm_table_get_num_targets(t); i++) {
> -		ti = dm_table_get_target(t, i);
> -
> -		if (ti->type->iterate_devices &&
> -		    ti->type->iterate_devices(ti, device_requires_stable_pages, NULL))
> -			return true;
> -	}
> -
> -	return false;
> -}
> -
>  void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  			       struct queue_limits *limits)
>  {
> @@ -1837,9 +1836,9 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  	}
>  	blk_queue_write_cache(q, wc, fua);
>  
> -	if (dm_table_supports_dax(t, device_supports_dax, &page_size)) {
> +	if (dm_table_supports_dax(t, device_not_dax_capable, &page_size)) {
>  		blk_queue_flag_set(QUEUE_FLAG_DAX, q);
> -		if (dm_table_supports_dax(t, device_dax_synchronous, NULL))
> +		if (dm_table_supports_dax(t, device_not_dax_synchronous_capable, NULL))
>  			set_dax_synchronous(t->md->dax_dev);
>  	}
>  	else
> @@ -1849,10 +1848,10 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  		dax_write_cache(t->md->dax_dev, true);
>  
>  	/* Ensure that all underlying devices are non-rotational. */
> -	if (dm_table_all_devices_attribute(t, device_is_nonrot))
> -		blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
> -	else
> +	if (dm_table_any_device_attribute(t, device_is_rot))
>  		blk_queue_flag_clear(QUEUE_FLAG_NONROT, q);
> +	else
> +		blk_queue_flag_set(QUEUE_FLAG_NONROT, q);
>  
>  	if (!dm_table_supports_write_same(t))
>  		q->limits.max_write_same_sectors = 0;
> @@ -1864,8 +1863,11 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  	/*
>  	 * Some devices don't use blk_integrity but still want stable pages
>  	 * because they do their own checksumming.
> +	 * If any underlying device requires stable pages, a table must require
> +	 * them as well.  Only targets that support iterate_devices are considered:
> +	 * don't want error, zero, etc to require stable pages.
>  	 */
> -	if (dm_table_requires_stable_pages(t))
> +	if (dm_table_any_device_attribute(t, device_requires_stable_pages))
>  		blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, q);
>  	else
>  		blk_queue_flag_clear(QUEUE_FLAG_STABLE_WRITES, q);
> @@ -1876,8 +1878,10 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
>  	 * Clear QUEUE_FLAG_ADD_RANDOM if any underlying device does not
>  	 * have it set.
>  	 */
> -	if (blk_queue_add_random(q) && dm_table_all_devices_attribute(t, device_is_not_random))
> +	if (dm_table_any_device_attribute(t, device_is_not_random))
>  		blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, q);
> +	else
> +		blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);
>  
>  	/*
>  	 * For a zoned target, the number of zones should be updated for the
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 7bac564f3faa..8a3d73efb9dd 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1128,7 +1128,7 @@ static bool dm_dax_supported(struct dax_device *dax_dev, struct block_device *bd
>  	if (!map)
>  		goto out;
>  
> -	ret = dm_table_supports_dax(map, device_supports_dax, &blocksize);
> +	ret = dm_table_supports_dax(map, device_not_dax_capable, &blocksize);
>  
>  out:
>  	dm_put_live_table(md, srcu_idx);
> diff --git a/drivers/md/dm.h b/drivers/md/dm.h
> index fffe1e289c53..b441ad772c18 100644
> --- a/drivers/md/dm.h
> +++ b/drivers/md/dm.h
> @@ -73,7 +73,7 @@ void dm_table_free_md_mempools(struct dm_table *t);
>  struct dm_md_mempools *dm_table_get_md_mempools(struct dm_table *t);
>  bool dm_table_supports_dax(struct dm_table *t, iterate_devices_callout_fn fn,
>  			   int *blocksize);
> -int device_supports_dax(struct dm_target *ti, struct dm_dev *dev,
> +int device_not_dax_capable(struct dm_target *ti, struct dm_dev *dev,
>  			   sector_t start, sector_t len, void *data);
>  
>  void dm_lock_md_type(struct mapped_device *md);
> 

-- 
Thanks,
Jeffle

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm: fix iterate_device sanity check

[Mike Snitzer]

On Mon, Feb 01 2021 at 10:35pm -0500,
Jeffle Xu <jefflexu@linux.alibaba.com> wrote:

> According to the definition of dm_iterate_devices_fn:
>  * This function must iterate through each section of device used by the
>  * target until it encounters a non-zero return code, which it then returns.
>  * Returns zero if no callout returned non-zero.
> 
> For some target type (e.g., dm-stripe), one call of iterate_devices() may
> iterate multiple underlying devices internally, in which case a non-zero
> return code returned by iterate_devices_callout_fn will stop the iteration
> in advance.
> 
> Thus if we want to ensure that _all_ underlying devices support some kind of
> attribute, the iteration structure like dm_table_supports_nowait() should be
> used, while the input iterate_devices_callout_fn should handle the 'not
> support' semantics. On the opposite, the iteration structure like
> dm_table_any_device_attribute() should be used if _any_ underlying device
> supporting this attibute is sufficient. In this case, the input
> iterate_devices_callout_fn should handle the 'support' semantics.
> 
> Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
> Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
> Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>

Thanks for auditing and fixing this up.  It has been on my todo so
you've really helped me out -- your changes look correct to me.

I've staged it for 5.12, the stable fix will likely need manual fixups
depending on the stable tree... we'll just need to assist with
backport(s) as needed.

Thanks again,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm: fix iterate_device sanity check

[JeffleXu]

On 2/6/21 2:39 AM, Mike Snitzer wrote:
> On Mon, Feb 01 2021 at 10:35pm -0500,
> Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
> 
>> According to the definition of dm_iterate_devices_fn:
>>  * This function must iterate through each section of device used by the
>>  * target until it encounters a non-zero return code, which it then returns.
>>  * Returns zero if no callout returned non-zero.
>>
>> For some target type (e.g., dm-stripe), one call of iterate_devices() may
>> iterate multiple underlying devices internally, in which case a non-zero
>> return code returned by iterate_devices_callout_fn will stop the iteration
>> in advance.
>>
>> Thus if we want to ensure that _all_ underlying devices support some kind of
>> attribute, the iteration structure like dm_table_supports_nowait() should be
>> used, while the input iterate_devices_callout_fn should handle the 'not
>> support' semantics. On the opposite, the iteration structure like
>> dm_table_any_device_attribute() should be used if _any_ underlying device
>> supporting this attibute is sufficient. In this case, the input
>> iterate_devices_callout_fn should handle the 'support' semantics.
>>
>> Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
>> Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
>> Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> 
> Thanks for auditing and fixing this up.  It has been on my todo so
> you've really helped me out -- your changes look correct to me.
> 
> I've staged it for 5.12, the stable fix will likely need manual fixups
> depending on the stable tree... we'll just need to assist with
> backport(s) as needed.
> 

Thanks.

Actually, I have one v2 version patch, fixing more issues, and some
explanation comment included. Maybe I should send the v2?

-- 
Thanks,
Jeffle

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm: fix iterate_device sanity check

[JeffleXu]

On 2/6/21 2:39 AM, Mike Snitzer wrote:
> On Mon, Feb 01 2021 at 10:35pm -0500,
> Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
> 
>> According to the definition of dm_iterate_devices_fn:
>>  * This function must iterate through each section of device used by the
>>  * target until it encounters a non-zero return code, which it then returns.
>>  * Returns zero if no callout returned non-zero.
>>
>> For some target type (e.g., dm-stripe), one call of iterate_devices() may
>> iterate multiple underlying devices internally, in which case a non-zero
>> return code returned by iterate_devices_callout_fn will stop the iteration
>> in advance.
>>
>> Thus if we want to ensure that _all_ underlying devices support some kind of
>> attribute, the iteration structure like dm_table_supports_nowait() should be
>> used, while the input iterate_devices_callout_fn should handle the 'not
>> support' semantics. On the opposite, the iteration structure like
>> dm_table_any_device_attribute() should be used if _any_ underlying device
>> supporting this attibute is sufficient. In this case, the input
>> iterate_devices_callout_fn should handle the 'support' semantics.
>>
>> Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
>> Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
>> Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> 
> Thanks for auditing and fixing this up.  It has been on my todo so
> you've really helped me out -- your changes look correct to me.
> 
> I've staged it for 5.12, the stable fix will likely need manual fixups
> depending on the stable tree... we'll just need to assist with
> backport(s) as needed.

I'm glad to help offer the stable backport. But I don't know which
kernel version the stable kernel is still being maintained. Also which
mailing list I should send to when I finished backporting?

-- 
Thanks,
Jeffle

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm: fix iterate_device sanity check

[Mike Snitzer]

On Fri, Feb 05 2021 at  9:03pm -0500,
JeffleXu <jefflexu@linux.alibaba.com> wrote:

> 
> 
> On 2/6/21 2:39 AM, Mike Snitzer wrote:
> > On Mon, Feb 01 2021 at 10:35pm -0500,
> > Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
> > 
> >> According to the definition of dm_iterate_devices_fn:
> >>  * This function must iterate through each section of device used by the
> >>  * target until it encounters a non-zero return code, which it then returns.
> >>  * Returns zero if no callout returned non-zero.
> >>
> >> For some target type (e.g., dm-stripe), one call of iterate_devices() may
> >> iterate multiple underlying devices internally, in which case a non-zero
> >> return code returned by iterate_devices_callout_fn will stop the iteration
> >> in advance.
> >>
> >> Thus if we want to ensure that _all_ underlying devices support some kind of
> >> attribute, the iteration structure like dm_table_supports_nowait() should be
> >> used, while the input iterate_devices_callout_fn should handle the 'not
> >> support' semantics. On the opposite, the iteration structure like
> >> dm_table_any_device_attribute() should be used if _any_ underlying device
> >> supporting this attibute is sufficient. In this case, the input
> >> iterate_devices_callout_fn should handle the 'support' semantics.
> >>
> >> Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
> >> Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
> >> Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> > 
> > Thanks for auditing and fixing this up.  It has been on my todo so
> > you've really helped me out -- your changes look correct to me.
> > 
> > I've staged it for 5.12, the stable fix will likely need manual fixups
> > depending on the stable tree... we'll just need to assist with
> > backport(s) as needed.
> 
> I'm glad to help offer the stable backport. But I don't know which
> kernel version the stable kernel is still being maintained. Also which
> mailing list I should send to when I finished backporting?

All your v2 changes speak to needing more discipline in crafting
individual stable@ fixes that are applicable to various kernels.. when
all applied to mainline then it'd be the equivalent of your single
monolithic patch.

But without splitting the changes into separate patches, for stable@'s
benefit, we'll have a much more difficult time of shepherding the
applicable changes into the disparate stable@ kernels.

I'll have a look at splitting your v2 up accordingly.

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm: fix iterate_device sanity check

[Mike Snitzer]

On Mon, Feb 08 2021 at 10:17am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Fri, Feb 05 2021 at  9:03pm -0500,
> JeffleXu <jefflexu@linux.alibaba.com> wrote:
> 
> > 
> > 
> > On 2/6/21 2:39 AM, Mike Snitzer wrote:
> > > On Mon, Feb 01 2021 at 10:35pm -0500,
> > > Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
> > > 
> > >> According to the definition of dm_iterate_devices_fn:
> > >>  * This function must iterate through each section of device used by the
> > >>  * target until it encounters a non-zero return code, which it then returns.
> > >>  * Returns zero if no callout returned non-zero.
> > >>
> > >> For some target type (e.g., dm-stripe), one call of iterate_devices() may
> > >> iterate multiple underlying devices internally, in which case a non-zero
> > >> return code returned by iterate_devices_callout_fn will stop the iteration
> > >> in advance.
> > >>
> > >> Thus if we want to ensure that _all_ underlying devices support some kind of
> > >> attribute, the iteration structure like dm_table_supports_nowait() should be
> > >> used, while the input iterate_devices_callout_fn should handle the 'not
> > >> support' semantics. On the opposite, the iteration structure like
> > >> dm_table_any_device_attribute() should be used if _any_ underlying device
> > >> supporting this attibute is sufficient. In this case, the input
> > >> iterate_devices_callout_fn should handle the 'support' semantics.
> > >>
> > >> Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
> > >> Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
> > >> Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
> > >> Cc: stable@vger.kernel.org
> > >> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> > > 
> > > Thanks for auditing and fixing this up.  It has been on my todo so
> > > you've really helped me out -- your changes look correct to me.
> > > 
> > > I've staged it for 5.12, the stable fix will likely need manual fixups
> > > depending on the stable tree... we'll just need to assist with
> > > backport(s) as needed.
> > 
> > I'm glad to help offer the stable backport. But I don't know which
> > kernel version the stable kernel is still being maintained. Also which
> > mailing list I should send to when I finished backporting?
> 
> All your v2 changes speak to needing more discipline in crafting
> individual stable@ fixes that are applicable to various kernels.. when
> all applied to mainline then it'd be the equivalent of your single
> monolithic patch.
> 
> But without splitting the changes into separate patches, for stable@'s
> benefit, we'll have a much more difficult time of shepherding the
> applicable changes into the disparate stable@ kernels.
> 
> I'll have a look at splitting your v2 up accordingly.

Hi, please see these commits that I've staged in linux-next via:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next

1141b9133777 dm table: fix iterate_devices based device capability checks
0224c5e6fd07 dm table: fix DAX iterate_devices based device capability checks
76b0e14be03f dm table: fix zoned iterate_devices based device capability checks
55cdd7435e97 dm table: remove needless request_queue NULL pointer checks

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm: fix iterate_device sanity check

[JeffleXu]

On 2/9/21 1:29 PM, Mike Snitzer wrote:
> On Mon, Feb 08 2021 at 10:17am -0500,
> Mike Snitzer <snitzer@redhat.com> wrote:
> 
>> On Fri, Feb 05 2021 at  9:03pm -0500,
>> JeffleXu <jefflexu@linux.alibaba.com> wrote:
>>
>>>
>>>
>>> On 2/6/21 2:39 AM, Mike Snitzer wrote:
>>>> On Mon, Feb 01 2021 at 10:35pm -0500,
>>>> Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
>>>>
>>>>> According to the definition of dm_iterate_devices_fn:
>>>>>  * This function must iterate through each section of device used by the
>>>>>  * target until it encounters a non-zero return code, which it then returns.
>>>>>  * Returns zero if no callout returned non-zero.
>>>>>
>>>>> For some target type (e.g., dm-stripe), one call of iterate_devices() may
>>>>> iterate multiple underlying devices internally, in which case a non-zero
>>>>> return code returned by iterate_devices_callout_fn will stop the iteration
>>>>> in advance.
>>>>>
>>>>> Thus if we want to ensure that _all_ underlying devices support some kind of
>>>>> attribute, the iteration structure like dm_table_supports_nowait() should be
>>>>> used, while the input iterate_devices_callout_fn should handle the 'not
>>>>> support' semantics. On the opposite, the iteration structure like
>>>>> dm_table_any_device_attribute() should be used if _any_ underlying device
>>>>> supporting this attibute is sufficient. In this case, the input
>>>>> iterate_devices_callout_fn should handle the 'support' semantics.
>>>>>
>>>>> Fixes: 545ed20e6df6 ("dm: add infrastructure for DAX support")
>>>>> Fixes: c3c4555edd10 ("dm table: clear add_random unless all devices have it set")
>>>>> Fixes: 4693c9668fdc ("dm table: propagate non rotational flag")
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>>>>
>>>> Thanks for auditing and fixing this up.  It has been on my todo so
>>>> you've really helped me out -- your changes look correct to me.
>>>>
>>>> I've staged it for 5.12, the stable fix will likely need manual fixups
>>>> depending on the stable tree... we'll just need to assist with
>>>> backport(s) as needed.
>>>
>>> I'm glad to help offer the stable backport. But I don't know which
>>> kernel version the stable kernel is still being maintained. Also which
>>> mailing list I should send to when I finished backporting?
>>
>> All your v2 changes speak to needing more discipline in crafting
>> individual stable@ fixes that are applicable to various kernels.. when
>> all applied to mainline then it'd be the equivalent of your single
>> monolithic patch.
>>
>> But without splitting the changes into separate patches, for stable@'s
>> benefit, we'll have a much more difficult time of shepherding the
>> applicable changes into the disparate stable@ kernels.
>>
>> I'll have a look at splitting your v2 up accordingly.
> 
> Hi, please see these commits that I've staged in linux-next via:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next
> 
> 1141b9133777 dm table: fix iterate_devices based device capability checks
> 0224c5e6fd07 dm table: fix DAX iterate_devices based device capability checks
> 76b0e14be03f dm table: fix zoned iterate_devices based device capability checks
> 55cdd7435e97 dm table: remove needless request_queue NULL pointer checks
> 

Thanks. This series looks good to me.

I suddenly find that the semantics of patch 1 (1141b9133777 dm table:
fix iterate_devices based device capability checks) is a little
different with the original context.

-	if (blk_queue_add_random(q) && dm_table_all_devices_attribute(t,
device_is_not_random))
+	if (dm_table_any_dev_attr(t, device_is_not_random))
 		blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, q);
+	else
+		blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);

In the original context, QUEUE_FLAG_ADD_RANDOM will only be cleared, it
won't be set, while it could be set after patch 1. But I could see no
harm of setting QUEUE_FLAG_ADD_RANDOM flag though.

FYI. Currently only scsi devices are still using QUEUE_FLAG_ADD_RANDOM
flag, as all non-rotational devices should not set this flag since
commit b277da0a8a59 ("block: disable entropy contributions for nonrot
devices").


-- 
Thanks,
Jeffle

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm: fix iterate_device sanity check

[Mike Snitzer]

On Tue, Feb 09 2021 at  2:06am -0500,
JeffleXu <jefflexu@linux.alibaba.com> wrote:

> 
> 
> On 2/9/21 1:29 PM, Mike Snitzer wrote:
> > 
> > Hi, please see these commits that I've staged in linux-next via:
> > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=for-next
> > 
> > 1141b9133777 dm table: fix iterate_devices based device capability checks
> > 0224c5e6fd07 dm table: fix DAX iterate_devices based device capability checks
> > 76b0e14be03f dm table: fix zoned iterate_devices based device capability checks
> > 55cdd7435e97 dm table: remove needless request_queue NULL pointer checks
> > 
> 
> Thanks. This series looks good to me.
> 
> I suddenly find that the semantics of patch 1 (1141b9133777 dm table:
> fix iterate_devices based device capability checks) is a little
> different with the original context.
> 
> -	if (blk_queue_add_random(q) && dm_table_all_devices_attribute(t,
> device_is_not_random))
> +	if (dm_table_any_dev_attr(t, device_is_not_random))
>  		blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, q);
> +	else
> +		blk_queue_flag_set(QUEUE_FLAG_ADD_RANDOM, q);
> 
> In the original context, QUEUE_FLAG_ADD_RANDOM will only be cleared, it
> won't be set, while it could be set after patch 1. But I could see no
> harm of setting QUEUE_FLAG_ADD_RANDOM flag though.
> 
> FYI. Currently only scsi devices are still using QUEUE_FLAG_ADD_RANDOM
> flag, as all non-rotational devices should not set this flag since
> commit b277da0a8a59 ("block: disable entropy contributions for nonrot
> devices").

I fixed it, thanks.

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel