clean up the blk-ia-ranges.c code a bit

clean up the blk-ia-ranges.c code a bit

[Christoph Hellwig]

Hi Jens, hi Damien,

this is a little drive by cleanup for the ia-ranges code, including
moving the data structure to the gendisk as it is only used for
non-passthrough access.

I don't have hardware to test this on, so it would be good to make this
go through Damien's rig.

Diffstat:
 block/blk-ia-ranges.c  |   65 +++++++++++++++----------------------------------
 block/blk-sysfs.c      |    2 -
 block/blk.h            |    3 --
 include/linux/blkdev.h |   12 ++++-----
 4 files changed, 28 insertions(+), 54 deletions(-)

[PATCH 1/2] block: move ->ia_ranges from the request_queue to the gendisk

[Christoph Hellwig]

Independent access ranges only matter for file system I/O and are only
valid with a registered gendisk, so move them there.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-ia-ranges.c  | 18 +++++++++---------
 include/linux/blkdev.h | 12 ++++++------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/block/blk-ia-ranges.c b/block/blk-ia-ranges.c
index 47c89e65b57fa..c1bf14bcd15f4 100644
--- a/block/blk-ia-ranges.c
+++ b/block/blk-ia-ranges.c
@@ -106,7 +106,7 @@ static struct kobj_type blk_ia_ranges_ktype = {
  *
  * Register with sysfs a set of independent access ranges for @disk.
  * If @new_iars is not NULL, this set of ranges is registered and the old set
- * specified by q->ia_ranges is unregistered. Otherwise, q->ia_ranges is
+ * specified by disk->ia_ranges is unregistered. Otherwise, disk->ia_ranges is
  * registered if it is not already.
  */
 int disk_register_independent_access_ranges(struct gendisk *disk,
@@ -121,12 +121,12 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
 
 	/* If a new range set is specified, unregister the old one */
 	if (new_iars) {
-		if (q->ia_ranges)
+		if (disk->ia_ranges)
 			disk_unregister_independent_access_ranges(disk);
-		q->ia_ranges = new_iars;
+		disk->ia_ranges = new_iars;
 	}
 
-	iars = q->ia_ranges;
+	iars = disk->ia_ranges;
 	if (!iars)
 		return 0;
 
@@ -138,7 +138,7 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
 	ret = kobject_init_and_add(&iars->kobj, &blk_ia_ranges_ktype,
 				   &q->kobj, "%s", "independent_access_ranges");
 	if (ret) {
-		q->ia_ranges = NULL;
+		disk->ia_ranges = NULL;
 		kobject_put(&iars->kobj);
 		return ret;
 	}
@@ -164,7 +164,7 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
 void disk_unregister_independent_access_ranges(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
-	struct blk_independent_access_ranges *iars = q->ia_ranges;
+	struct blk_independent_access_ranges *iars = disk->ia_ranges;
 	int i;
 
 	lockdep_assert_held(&q->sysfs_dir_lock);
@@ -182,7 +182,7 @@ void disk_unregister_independent_access_ranges(struct gendisk *disk)
 		kfree(iars);
 	}
 
-	q->ia_ranges = NULL;
+	disk->ia_ranges = NULL;
 }
 
 static struct blk_independent_access_range *
@@ -242,7 +242,7 @@ static bool disk_check_ia_ranges(struct gendisk *disk,
 static bool disk_ia_ranges_changed(struct gendisk *disk,
 				   struct blk_independent_access_ranges *new)
 {
-	struct blk_independent_access_ranges *old = disk->queue->ia_ranges;
+	struct blk_independent_access_ranges *old = disk->ia_ranges;
 	int i;
 
 	if (!old)
@@ -331,7 +331,7 @@ void disk_set_independent_access_ranges(struct gendisk *disk,
 	if (blk_queue_registered(q)) {
 		disk_register_independent_access_ranges(disk, iars);
 	} else {
-		swap(q->ia_ranges, iars);
+		swap(disk->ia_ranges, iars);
 		kfree(iars);
 	}
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 22b12531aeb71..b9a94c53c6cd3 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -171,6 +171,12 @@ struct gendisk {
 	struct badblocks *bb;
 	struct lockdep_map lockdep_map;
 	u64 diskseq;
+
+	/*
+	 * Independent sector access ranges. This is always NULL for
+	 * devices that do not have multiple independent access ranges.
+	 */
+	struct blk_independent_access_ranges *ia_ranges;
 };
 
 static inline bool disk_live(struct gendisk *disk)
@@ -539,12 +545,6 @@ struct request_queue {
 
 	bool			mq_sysfs_init_done;
 
-	/*
-	 * Independent sector access ranges. This is always NULL for
-	 * devices that do not have multiple independent access ranges.
-	 */
-	struct blk_independent_access_ranges *ia_ranges;
-
 	/**
 	 * @srcu: Sleepable RCU. Use as lock when type of the request queue
 	 * is blocking (BLK_MQ_F_BLOCKING). Must be the last member
-- 
2.30.2

[PATCH 2/2] block: simplify disk_set_independent_access_ranges

[Christoph Hellwig]

Lift setting disk->ia_ranges from disk_register_independent_access_ranges
into disk_set_independent_access_ranges, and make the behavior the same
for the registered vs non-registered queue cases.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-ia-ranges.c | 57 ++++++++++++-------------------------------
 block/blk-sysfs.c     |  2 +-
 block/blk.h           |  3 +--
 3 files changed, 18 insertions(+), 44 deletions(-)

diff --git a/block/blk-ia-ranges.c b/block/blk-ia-ranges.c
index c1bf14bcd15f4..2bd1d311033b5 100644
--- a/block/blk-ia-ranges.c
+++ b/block/blk-ia-ranges.c
@@ -102,31 +102,18 @@ static struct kobj_type blk_ia_ranges_ktype = {
  * disk_register_independent_access_ranges - register with sysfs a set of
  *		independent access ranges
  * @disk:	Target disk
- * @new_iars:	New set of independent access ranges
  *
  * Register with sysfs a set of independent access ranges for @disk.
- * If @new_iars is not NULL, this set of ranges is registered and the old set
- * specified by disk->ia_ranges is unregistered. Otherwise, disk->ia_ranges is
- * registered if it is not already.
  */
-int disk_register_independent_access_ranges(struct gendisk *disk,
-				struct blk_independent_access_ranges *new_iars)
+int disk_register_independent_access_ranges(struct gendisk *disk)
 {
+	struct blk_independent_access_ranges *iars = disk->ia_ranges;
 	struct request_queue *q = disk->queue;
-	struct blk_independent_access_ranges *iars;
 	int i, ret;
 
 	lockdep_assert_held(&q->sysfs_dir_lock);
 	lockdep_assert_held(&q->sysfs_lock);
 
-	/* If a new range set is specified, unregister the old one */
-	if (new_iars) {
-		if (disk->ia_ranges)
-			disk_unregister_independent_access_ranges(disk);
-		disk->ia_ranges = new_iars;
-	}
-
-	iars = disk->ia_ranges;
 	if (!iars)
 		return 0;
 
@@ -210,6 +197,9 @@ static bool disk_check_ia_ranges(struct gendisk *disk,
 	sector_t sector = 0;
 	int i;
 
+	if (WARN_ON_ONCE(!iars->nr_ia_ranges))
+		return false;
+
 	/*
 	 * While sorting the ranges in increasing LBA order, check that the
 	 * ranges do not overlap, that there are no sector holes and that all
@@ -298,25 +288,15 @@ void disk_set_independent_access_ranges(struct gendisk *disk,
 {
 	struct request_queue *q = disk->queue;
 
-	if (WARN_ON_ONCE(iars && !iars->nr_ia_ranges)) {
+	mutex_lock(&q->sysfs_dir_lock);
+	mutex_lock(&q->sysfs_lock);
+	if (iars && !disk_check_ia_ranges(disk, iars)) {
 		kfree(iars);
 		iars = NULL;
 	}
-
-	mutex_lock(&q->sysfs_dir_lock);
-	mutex_lock(&q->sysfs_lock);
-
-	if (iars) {
-		if (!disk_check_ia_ranges(disk, iars)) {
-			kfree(iars);
-			iars = NULL;
-			goto reg;
-		}
-
-		if (!disk_ia_ranges_changed(disk, iars)) {
-			kfree(iars);
-			goto unlock;
-		}
+	if (iars && !disk_ia_ranges_changed(disk, iars)) {
+		kfree(iars);
+		goto unlock;
 	}
 
 	/*
@@ -324,17 +304,12 @@ void disk_set_independent_access_ranges(struct gendisk *disk,
 	 * revalidation. If that is the case, we need to unregister the old
 	 * set of independent access ranges and register the new set. If the
 	 * queue is not registered, registration of the device request queue
-	 * will register the independent access ranges, so only swap in the
-	 * new set and free the old one.
+	 * will register the independent access ranges.
 	 */
-reg:
-	if (blk_queue_registered(q)) {
-		disk_register_independent_access_ranges(disk, iars);
-	} else {
-		swap(disk->ia_ranges, iars);
-		kfree(iars);
-	}
-
+	disk_unregister_independent_access_ranges(disk);
+	disk->ia_ranges = iars;
+	if (blk_queue_registered(q))
+		disk_register_independent_access_ranges(disk);
 unlock:
 	mutex_unlock(&q->sysfs_lock);
 	mutex_unlock(&q->sysfs_dir_lock);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 85ea43eff094c..58cb9cb9f48cd 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -832,7 +832,7 @@ int blk_register_queue(struct gendisk *disk)
 		blk_mq_debugfs_register(q);
 	mutex_unlock(&q->debugfs_mutex);
 
-	ret = disk_register_independent_access_ranges(disk, NULL);
+	ret = disk_register_independent_access_ranges(disk);
 	if (ret)
 		goto put_dev;
 
diff --git a/block/blk.h b/block/blk.h
index 74d59435870cb..58ad50cacd2d5 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -459,8 +459,7 @@ long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg);
 
 extern const struct address_space_operations def_blk_aops;
 
-int disk_register_independent_access_ranges(struct gendisk *disk,
-				struct blk_independent_access_ranges *new_iars);
+int disk_register_independent_access_ranges(struct gendisk *disk);
 void disk_unregister_independent_access_ranges(struct gendisk *disk);
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST
-- 
2.30.2

Re: clean up the blk-ia-ranges.c code a bit

[Damien Le Moal]

On 6/29/22 15:20, Christoph Hellwig wrote:
> Hi Jens, hi Damien,
> 
> this is a little drive by cleanup for the ia-ranges code, including
> moving the data structure to the gendisk as it is only used for
> non-passthrough access.
> 
> I don't have hardware to test this on, so it would be good to make this
> go through Damien's rig.

What branch is this based on ?

> 
> Diffstat:
>  block/blk-ia-ranges.c  |   65 +++++++++++++++----------------------------------
>  block/blk-sysfs.c      |    2 -
>  block/blk.h            |    3 --
>  include/linux/blkdev.h |   12 ++++-----
>  4 files changed, 28 insertions(+), 54 deletions(-)


-- 
Damien Le Moal
Western Digital Research

Re: clean up the blk-ia-ranges.c code a bit

[Christoph Hellwig]

On Wed, Jun 29, 2022 at 03:32:36PM +0900, Damien Le Moal wrote:
> On 6/29/22 15:20, Christoph Hellwig wrote:
> > Hi Jens, hi Damien,
> > 
> > this is a little drive by cleanup for the ia-ranges code, including
> > moving the data structure to the gendisk as it is only used for
> > non-passthrough access.
> > 
> > I don't have hardware to test this on, so it would be good to make this
> > go through Damien's rig.
> 
> What branch is this based on ?

for-5.20/block

Re: clean up the blk-ia-ranges.c code a bit

[Damien Le Moal]

On 6/29/22 15:44, Christoph Hellwig wrote:
> On Wed, Jun 29, 2022 at 03:32:36PM +0900, Damien Le Moal wrote:
>> On 6/29/22 15:20, Christoph Hellwig wrote:
>>> Hi Jens, hi Damien,
>>>
>>> this is a little drive by cleanup for the ia-ranges code, including
>>> moving the data structure to the gendisk as it is only used for
>>> non-passthrough access.
>>>
>>> I don't have hardware to test this on, so it would be good to make this
>>> go through Damien's rig.
>>
>> What branch is this based on ?
> 
> for-5.20/block

OK. Testing.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] block: move ->ia_ranges from the request_queue to the gendisk

[Damien Le Moal]

On 6/29/22 15:20, Christoph Hellwig wrote:
> Independent access ranges only matter for file system I/O and are only
> valid with a registered gendisk, so move them there.

Would this potentially affect the use of ranges in DM ? E.g. exposing a
dm-linear device targets as ranges. I do not think so but I did not check
the details.

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-ia-ranges.c  | 18 +++++++++---------
>  include/linux/blkdev.h | 12 ++++++------
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/block/blk-ia-ranges.c b/block/blk-ia-ranges.c
> index 47c89e65b57fa..c1bf14bcd15f4 100644
> --- a/block/blk-ia-ranges.c
> +++ b/block/blk-ia-ranges.c
> @@ -106,7 +106,7 @@ static struct kobj_type blk_ia_ranges_ktype = {
>   *
>   * Register with sysfs a set of independent access ranges for @disk.
>   * If @new_iars is not NULL, this set of ranges is registered and the old set
> - * specified by q->ia_ranges is unregistered. Otherwise, q->ia_ranges is
> + * specified by disk->ia_ranges is unregistered. Otherwise, disk->ia_ranges is
>   * registered if it is not already.
>   */
>  int disk_register_independent_access_ranges(struct gendisk *disk,
> @@ -121,12 +121,12 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
>  
>  	/* If a new range set is specified, unregister the old one */
>  	if (new_iars) {
> -		if (q->ia_ranges)
> +		if (disk->ia_ranges)
>  			disk_unregister_independent_access_ranges(disk);
> -		q->ia_ranges = new_iars;
> +		disk->ia_ranges = new_iars;
>  	}
>  
> -	iars = q->ia_ranges;
> +	iars = disk->ia_ranges;
>  	if (!iars)
>  		return 0;
>  
> @@ -138,7 +138,7 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
>  	ret = kobject_init_and_add(&iars->kobj, &blk_ia_ranges_ktype,
>  				   &q->kobj, "%s", "independent_access_ranges");
>  	if (ret) {
> -		q->ia_ranges = NULL;
> +		disk->ia_ranges = NULL;
>  		kobject_put(&iars->kobj);
>  		return ret;
>  	}
> @@ -164,7 +164,7 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
>  void disk_unregister_independent_access_ranges(struct gendisk *disk)
>  {
>  	struct request_queue *q = disk->queue;
> -	struct blk_independent_access_ranges *iars = q->ia_ranges;
> +	struct blk_independent_access_ranges *iars = disk->ia_ranges;
>  	int i;
>  
>  	lockdep_assert_held(&q->sysfs_dir_lock);
> @@ -182,7 +182,7 @@ void disk_unregister_independent_access_ranges(struct gendisk *disk)
>  		kfree(iars);
>  	}
>  
> -	q->ia_ranges = NULL;
> +	disk->ia_ranges = NULL;
>  }
>  
>  static struct blk_independent_access_range *
> @@ -242,7 +242,7 @@ static bool disk_check_ia_ranges(struct gendisk *disk,
>  static bool disk_ia_ranges_changed(struct gendisk *disk,
>  				   struct blk_independent_access_ranges *new)
>  {
> -	struct blk_independent_access_ranges *old = disk->queue->ia_ranges;
> +	struct blk_independent_access_ranges *old = disk->ia_ranges;
>  	int i;
>  
>  	if (!old)
> @@ -331,7 +331,7 @@ void disk_set_independent_access_ranges(struct gendisk *disk,
>  	if (blk_queue_registered(q)) {
>  		disk_register_independent_access_ranges(disk, iars);
>  	} else {
> -		swap(q->ia_ranges, iars);
> +		swap(disk->ia_ranges, iars);
>  		kfree(iars);
>  	}
>  
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 22b12531aeb71..b9a94c53c6cd3 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -171,6 +171,12 @@ struct gendisk {
>  	struct badblocks *bb;
>  	struct lockdep_map lockdep_map;
>  	u64 diskseq;
> +
> +	/*
> +	 * Independent sector access ranges. This is always NULL for
> +	 * devices that do not have multiple independent access ranges.
> +	 */
> +	struct blk_independent_access_ranges *ia_ranges;
>  };
>  
>  static inline bool disk_live(struct gendisk *disk)
> @@ -539,12 +545,6 @@ struct request_queue {
>  
>  	bool			mq_sysfs_init_done;
>  
> -	/*
> -	 * Independent sector access ranges. This is always NULL for
> -	 * devices that do not have multiple independent access ranges.
> -	 */
> -	struct blk_independent_access_ranges *ia_ranges;
> -
>  	/**
>  	 * @srcu: Sleepable RCU. Use as lock when type of the request queue
>  	 * is blocking (BLK_MQ_F_BLOCKING). Must be the last member


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] block: move ->ia_ranges from the request_queue to the gendisk

[Christoph Hellwig]

On Wed, Jun 29, 2022 at 04:08:29PM +0900, Damien Le Moal wrote:
> On 6/29/22 15:20, Christoph Hellwig wrote:
> > Independent access ranges only matter for file system I/O and are only
> > valid with a registered gendisk, so move them there.
> 
> Would this potentially affect the use of ranges in DM ? E.g. exposing a
> dm-linear device targets as ranges. I do not think so but I did not check
> the details.

Device mapper only ever does file system I/O.  Passthrough I/O must
be initiated by the actual driver like scsi or nvme.

Re: [PATCH 1/2] block: move ->ia_ranges from the request_queue to the gendisk

[Damien Le Moal]

On 6/29/22 15:20, Christoph Hellwig wrote:
> Independent access ranges only matter for file system I/O and are only
> valid with a registered gendisk, so move them there.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  block/blk-ia-ranges.c  | 18 +++++++++---------
>  include/linux/blkdev.h | 12 ++++++------
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/block/blk-ia-ranges.c b/block/blk-ia-ranges.c
> index 47c89e65b57fa..c1bf14bcd15f4 100644
> --- a/block/blk-ia-ranges.c
> +++ b/block/blk-ia-ranges.c
> @@ -106,7 +106,7 @@ static struct kobj_type blk_ia_ranges_ktype = {
>   *
>   * Register with sysfs a set of independent access ranges for @disk.
>   * If @new_iars is not NULL, this set of ranges is registered and the old set
> - * specified by q->ia_ranges is unregistered. Otherwise, q->ia_ranges is
> + * specified by disk->ia_ranges is unregistered. Otherwise, disk->ia_ranges is
>   * registered if it is not already.
>   */
>  int disk_register_independent_access_ranges(struct gendisk *disk,
> @@ -121,12 +121,12 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
>  
>  	/* If a new range set is specified, unregister the old one */
>  	if (new_iars) {
> -		if (q->ia_ranges)
> +		if (disk->ia_ranges)
>  			disk_unregister_independent_access_ranges(disk);
> -		q->ia_ranges = new_iars;
> +		disk->ia_ranges = new_iars;
>  	}
>  
> -	iars = q->ia_ranges;
> +	iars = disk->ia_ranges;
>  	if (!iars)
>  		return 0;
>  
> @@ -138,7 +138,7 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
>  	ret = kobject_init_and_add(&iars->kobj, &blk_ia_ranges_ktype,
>  				   &q->kobj, "%s", "independent_access_ranges");
>  	if (ret) {
> -		q->ia_ranges = NULL;
> +		disk->ia_ranges = NULL;
>  		kobject_put(&iars->kobj);
>  		return ret;
>  	}
> @@ -164,7 +164,7 @@ int disk_register_independent_access_ranges(struct gendisk *disk,
>  void disk_unregister_independent_access_ranges(struct gendisk *disk)
>  {
>  	struct request_queue *q = disk->queue;
> -	struct blk_independent_access_ranges *iars = q->ia_ranges;
> +	struct blk_independent_access_ranges *iars = disk->ia_ranges;
>  	int i;
>  
>  	lockdep_assert_held(&q->sysfs_dir_lock);
> @@ -182,7 +182,7 @@ void disk_unregister_independent_access_ranges(struct gendisk *disk)
>  		kfree(iars);
>  	}
>  
> -	q->ia_ranges = NULL;
> +	disk->ia_ranges = NULL;
>  }
>  
>  static struct blk_independent_access_range *
> @@ -242,7 +242,7 @@ static bool disk_check_ia_ranges(struct gendisk *disk,
>  static bool disk_ia_ranges_changed(struct gendisk *disk,
>  				   struct blk_independent_access_ranges *new)
>  {
> -	struct blk_independent_access_ranges *old = disk->queue->ia_ranges;
> +	struct blk_independent_access_ranges *old = disk->ia_ranges;
>  	int i;
>  
>  	if (!old)
> @@ -331,7 +331,7 @@ void disk_set_independent_access_ranges(struct gendisk *disk,
>  	if (blk_queue_registered(q)) {
>  		disk_register_independent_access_ranges(disk, iars);
>  	} else {
> -		swap(q->ia_ranges, iars);
> +		swap(disk->ia_ranges, iars);
>  		kfree(iars);
>  	}
>  
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 22b12531aeb71..b9a94c53c6cd3 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -171,6 +171,12 @@ struct gendisk {
>  	struct badblocks *bb;
>  	struct lockdep_map lockdep_map;
>  	u64 diskseq;
> +
> +	/*
> +	 * Independent sector access ranges. This is always NULL for
> +	 * devices that do not have multiple independent access ranges.
> +	 */
> +	struct blk_independent_access_ranges *ia_ranges;
>  };
>  
>  static inline bool disk_live(struct gendisk *disk)
> @@ -539,12 +545,6 @@ struct request_queue {
>  
>  	bool			mq_sysfs_init_done;
>  
> -	/*
> -	 * Independent sector access ranges. This is always NULL for
> -	 * devices that do not have multiple independent access ranges.
> -	 */
> -	struct blk_independent_access_ranges *ia_ranges;
> -
>  	/**
>  	 * @srcu: Sleepable RCU. Use as lock when type of the request queue
>  	 * is blocking (BLK_MQ_F_BLOCKING). Must be the last member


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] block: simplify disk_set_independent_access_ranges

[Damien Le Moal]

On 6/29/22 15:20, Christoph Hellwig wrote:
> Lift setting disk->ia_ranges from disk_register_independent_access_ranges
> into disk_set_independent_access_ranges, and make the behavior the same
> for the registered vs non-registered queue cases.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  block/blk-ia-ranges.c | 57 ++++++++++++-------------------------------
>  block/blk-sysfs.c     |  2 +-
>  block/blk.h           |  3 +--
>  3 files changed, 18 insertions(+), 44 deletions(-)
> 
> diff --git a/block/blk-ia-ranges.c b/block/blk-ia-ranges.c
> index c1bf14bcd15f4..2bd1d311033b5 100644
> --- a/block/blk-ia-ranges.c
> +++ b/block/blk-ia-ranges.c
> @@ -102,31 +102,18 @@ static struct kobj_type blk_ia_ranges_ktype = {
>   * disk_register_independent_access_ranges - register with sysfs a set of
>   *		independent access ranges
>   * @disk:	Target disk
> - * @new_iars:	New set of independent access ranges
>   *
>   * Register with sysfs a set of independent access ranges for @disk.
> - * If @new_iars is not NULL, this set of ranges is registered and the old set
> - * specified by disk->ia_ranges is unregistered. Otherwise, disk->ia_ranges is
> - * registered if it is not already.
>   */
> -int disk_register_independent_access_ranges(struct gendisk *disk,
> -				struct blk_independent_access_ranges *new_iars)
> +int disk_register_independent_access_ranges(struct gendisk *disk)
>  {
> +	struct blk_independent_access_ranges *iars = disk->ia_ranges;
>  	struct request_queue *q = disk->queue;
> -	struct blk_independent_access_ranges *iars;
>  	int i, ret;
>  
>  	lockdep_assert_held(&q->sysfs_dir_lock);
>  	lockdep_assert_held(&q->sysfs_lock);
>  
> -	/* If a new range set is specified, unregister the old one */
> -	if (new_iars) {
> -		if (disk->ia_ranges)
> -			disk_unregister_independent_access_ranges(disk);
> -		disk->ia_ranges = new_iars;
> -	}
> -
> -	iars = disk->ia_ranges;
>  	if (!iars)
>  		return 0;
>  
> @@ -210,6 +197,9 @@ static bool disk_check_ia_ranges(struct gendisk *disk,
>  	sector_t sector = 0;
>  	int i;
>  
> +	if (WARN_ON_ONCE(!iars->nr_ia_ranges))
> +		return false;
> +
>  	/*
>  	 * While sorting the ranges in increasing LBA order, check that the
>  	 * ranges do not overlap, that there are no sector holes and that all
> @@ -298,25 +288,15 @@ void disk_set_independent_access_ranges(struct gendisk *disk,
>  {
>  	struct request_queue *q = disk->queue;
>  
> -	if (WARN_ON_ONCE(iars && !iars->nr_ia_ranges)) {
> +	mutex_lock(&q->sysfs_dir_lock);
> +	mutex_lock(&q->sysfs_lock);
> +	if (iars && !disk_check_ia_ranges(disk, iars)) {
>  		kfree(iars);
>  		iars = NULL;
>  	}
> -
> -	mutex_lock(&q->sysfs_dir_lock);
> -	mutex_lock(&q->sysfs_lock);
> -
> -	if (iars) {
> -		if (!disk_check_ia_ranges(disk, iars)) {
> -			kfree(iars);
> -			iars = NULL;
> -			goto reg;
> -		}
> -
> -		if (!disk_ia_ranges_changed(disk, iars)) {
> -			kfree(iars);
> -			goto unlock;
> -		}
> +	if (iars && !disk_ia_ranges_changed(disk, iars)) {
> +		kfree(iars);
> +		goto unlock;
>  	}
>  
>  	/*
> @@ -324,17 +304,12 @@ void disk_set_independent_access_ranges(struct gendisk *disk,
>  	 * revalidation. If that is the case, we need to unregister the old
>  	 * set of independent access ranges and register the new set. If the
>  	 * queue is not registered, registration of the device request queue
> -	 * will register the independent access ranges, so only swap in the
> -	 * new set and free the old one.
> +	 * will register the independent access ranges.
>  	 */
> -reg:
> -	if (blk_queue_registered(q)) {
> -		disk_register_independent_access_ranges(disk, iars);
> -	} else {
> -		swap(disk->ia_ranges, iars);
> -		kfree(iars);
> -	}
> -
> +	disk_unregister_independent_access_ranges(disk);
> +	disk->ia_ranges = iars;
> +	if (blk_queue_registered(q))
> +		disk_register_independent_access_ranges(disk);
>  unlock:
>  	mutex_unlock(&q->sysfs_lock);
>  	mutex_unlock(&q->sysfs_dir_lock);
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 85ea43eff094c..58cb9cb9f48cd 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -832,7 +832,7 @@ int blk_register_queue(struct gendisk *disk)
>  		blk_mq_debugfs_register(q);
>  	mutex_unlock(&q->debugfs_mutex);
>  
> -	ret = disk_register_independent_access_ranges(disk, NULL);
> +	ret = disk_register_independent_access_ranges(disk);
>  	if (ret)
>  		goto put_dev;
>  
> diff --git a/block/blk.h b/block/blk.h
> index 74d59435870cb..58ad50cacd2d5 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -459,8 +459,7 @@ long compat_blkdev_ioctl(struct file *file, unsigned cmd, unsigned long arg);
>  
>  extern const struct address_space_operations def_blk_aops;
>  
> -int disk_register_independent_access_ranges(struct gendisk *disk,
> -				struct blk_independent_access_ranges *new_iars);
> +int disk_register_independent_access_ranges(struct gendisk *disk);
>  void disk_unregister_independent_access_ranges(struct gendisk *disk);
>  
>  #ifdef CONFIG_FAIL_MAKE_REQUEST


-- 
Damien Le Moal
Western Digital Research

Re: clean up the blk-ia-ranges.c code a bit

[Damien Le Moal]

On 6/29/22 15:20, Christoph Hellwig wrote:
> Hi Jens, hi Damien,
> 
> this is a little drive by cleanup for the ia-ranges code, including
> moving the data structure to the gendisk as it is only used for
> non-passthrough access.
> 
> I don't have hardware to test this on, so it would be good to make this
> go through Damien's rig.
> 
> Diffstat:
>  block/blk-ia-ranges.c  |   65 +++++++++++++++----------------------------------
>  block/blk-sysfs.c      |    2 -
>  block/blk.h            |    3 --
>  include/linux/blkdev.h |   12 ++++-----
>  4 files changed, 28 insertions(+), 54 deletions(-)

For the series:

Tested-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

-- 
Damien Le Moal
Western Digital Research

Re: clean up the blk-ia-ranges.c code a bit

[Jens Axboe]

On Wed, 29 Jun 2022 08:20:11 +0200, Christoph Hellwig wrote:
> this is a little drive by cleanup for the ia-ranges code, including
> moving the data structure to the gendisk as it is only used for
> non-passthrough access.
> 
> I don't have hardware to test this on, so it would be good to make this
> go through Damien's rig.
> 
> [...]

Applied, thanks!

[1/2] block: move ->ia_ranges from the request_queue to the gendisk
      commit: 6a27d28c81bc5843de2490688a04ee5baa6615e7
[2/2] block: simplify disk_set_independent_access_ranges
      commit: 22d0c4080fe49299640d9d6c43154c49794c2825

Best regards,
-- 
Jens Axboe