disk revalidation cleanups and fixlets

disk revalidation cleanups and fixlets

[Christoph Hellwig]

Hi Jens and Jan,

this series takes the disk size change detection and revalidations
from Jan a step further and fully integrate the code path for
partitioned vs non-partitioned devices.  It also fixes up a few
bits where we have unintentionally differing behavior.

[PATCH 1/5] block: refactor rescan_partitions

[Christoph Hellwig]

Split out a helper that adds one single partition, and another one
calling that dealing with the parsed_partitions state.  This makes
it much more obvious how we clean up all state and start again when
using the rescan label.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/partition-generic.c | 176 +++++++++++++++++++++-----------------
 1 file changed, 96 insertions(+), 80 deletions(-)

diff --git a/block/partition-generic.c b/block/partition-generic.c
index aee643ce13d1..f113be069b40 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -509,26 +509,77 @@ static bool part_zone_aligned(struct gendisk *disk,
 	return true;
 }
 
-int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
+static bool blk_add_partition(struct gendisk *disk, struct block_device *bdev,
+		struct parsed_partitions *state, int p)
 {
-	struct parsed_partitions *state = NULL;
+	sector_t size = state->parts[p].size;
+	sector_t from = state->parts[p].from;
 	struct hd_struct *part;
-	int p, highest, res;
-rescan:
-	if (state && !IS_ERR(state)) {
-		free_partitions(state);
-		state = NULL;
+
+	if (!size)
+		return true;
+
+	if (from >= get_capacity(disk)) {
+		printk(KERN_WARNING
+		       "%s: p%d start %llu is beyond EOD, ",
+		       disk->disk_name, p, (unsigned long long) from);
+		if (disk_unlock_native_capacity(disk))
+			return false;
+		return true;
 	}
 
-	res = drop_partitions(disk, bdev);
-	if (res)
-		return res;
+	if (from + size > get_capacity(disk)) {
+		printk(KERN_WARNING
+		       "%s: p%d size %llu extends beyond EOD, ",
+		       disk->disk_name, p, (unsigned long long) size);
 
-	if (disk->fops->revalidate_disk)
-		disk->fops->revalidate_disk(disk);
-	check_disk_size_change(disk, bdev, true);
-	bdev->bd_invalidated = 0;
-	if (!get_capacity(disk) || !(state = check_partition(disk, bdev)))
+		if (disk_unlock_native_capacity(disk))
+			return false;
+
+		/*
+		 * We can not ignore partitions of broken tables created by for
+		 * example camera firmware, but we limit them to the end of the
+		 * disk to avoid creating invalid block devices.
+		 */
+		size = get_capacity(disk) - from;
+	}
+
+	/*
+	 * On a zoned block device, partitions should be aligned on the device
+	 * zone size (i.e. zone boundary crossing not allowed).  Otherwise,
+	 * resetting the write pointer of the last zone of one partition may
+	 * impact the following partition.
+	 */
+	if (bdev_is_zoned(bdev) && !part_zone_aligned(disk, bdev, from, size)) {
+		printk(KERN_WARNING
+		       "%s: p%d start %llu+%llu is not zone aligned\n",
+		       disk->disk_name, p, (unsigned long long) from,
+		       (unsigned long long) size);
+		return true;
+	}
+
+	part = add_partition(disk, p, from, size, state->parts[p].flags,
+			     &state->parts[p].info);
+	if (IS_ERR(part)) {
+		printk(KERN_ERR " %s: p%d could not be added: %ld\n",
+		       disk->disk_name, p, -PTR_ERR(part));
+		return true;
+	}
+
+#ifdef CONFIG_BLK_DEV_MD
+	if (state->parts[p].flags & ADDPART_FLAG_RAID)
+		md_autodetect_dev(part_to_dev(part)->devt);
+#endif
+	return true;
+}
+
+static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
+{
+	struct parsed_partitions *state;
+	int ret = -EAGAIN, p, highest;
+
+	state = check_partition(disk, bdev);
+	if (!state)
 		return 0;
 	if (IS_ERR(state)) {
 		/*
@@ -540,7 +591,7 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 			printk(KERN_WARNING "%s: partition table beyond EOD, ",
 			       disk->disk_name);
 			if (disk_unlock_native_capacity(disk))
-				goto rescan;
+				return -EAGAIN;
 		}
 		return -EIO;
 	}
@@ -554,7 +605,7 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 		       "%s: partition table partially beyond EOD, ",
 		       disk->disk_name);
 		if (disk_unlock_native_capacity(disk))
-			goto rescan;
+			goto out_free_state;
 	}
 
 	/* tell userspace that the media / partition table may have changed */
@@ -571,72 +622,37 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 	disk_expand_part_tbl(disk, highest);
 
 	/* add partitions */
-	for (p = 1; p < state->limit; p++) {
-		sector_t size, from;
-
-		size = state->parts[p].size;
-		if (!size)
-			continue;
-
-		from = state->parts[p].from;
-		if (from >= get_capacity(disk)) {
-			printk(KERN_WARNING
-			       "%s: p%d start %llu is beyond EOD, ",
-			       disk->disk_name, p, (unsigned long long) from);
-			if (disk_unlock_native_capacity(disk))
-				goto rescan;
-			continue;
-		}
+	for (p = 1; p < state->limit; p++)
+		if (!blk_add_partition(disk, bdev, state, p))
+			goto out_free_state;
 
-		if (from + size > get_capacity(disk)) {
-			printk(KERN_WARNING
-			       "%s: p%d size %llu extends beyond EOD, ",
-			       disk->disk_name, p, (unsigned long long) size);
-
-			if (disk_unlock_native_capacity(disk)) {
-				/* free state and restart */
-				goto rescan;
-			} else {
-				/*
-				 * we can not ignore partitions of broken tables
-				 * created by for example camera firmware, but
-				 * we limit them to the end of the disk to avoid
-				 * creating invalid block devices
-				 */
-				size = get_capacity(disk) - from;
-			}
-		}
+	ret = 0;
+out_free_state:
+	free_partitions(state);
+	return ret;
+}
 
-		/*
-		 * On a zoned block device, partitions should be aligned on the
-		 * device zone size (i.e. zone boundary crossing not allowed).
-		 * Otherwise, resetting the write pointer of the last zone of
-		 * one partition may impact the following partition.
-		 */
-		if (bdev_is_zoned(bdev) &&
-		    !part_zone_aligned(disk, bdev, from, size)) {
-			printk(KERN_WARNING
-			       "%s: p%d start %llu+%llu is not zone aligned\n",
-			       disk->disk_name, p, (unsigned long long) from,
-			       (unsigned long long) size);
-			continue;
-		}
+int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
+{
+	int ret;
 
-		part = add_partition(disk, p, from, size,
-				     state->parts[p].flags,
-				     &state->parts[p].info);
-		if (IS_ERR(part)) {
-			printk(KERN_ERR " %s: p%d could not be added: %ld\n",
-			       disk->disk_name, p, -PTR_ERR(part));
-			continue;
-		}
-#ifdef CONFIG_BLK_DEV_MD
-		if (state->parts[p].flags & ADDPART_FLAG_RAID)
-			md_autodetect_dev(part_to_dev(part)->devt);
-#endif
-	}
-	free_partitions(state);
-	return 0;
+rescan:
+	ret = drop_partitions(disk, bdev);
+	if (ret)
+		return ret;
+
+	if (disk->fops->revalidate_disk)
+		disk->fops->revalidate_disk(disk);
+	check_disk_size_change(disk, bdev, true);
+	bdev->bd_invalidated = 0;
+
+	if (!get_capacity(disk))
+		return 0;
+	
+	ret = blk_add_partitions(disk, bdev);
+	if (ret == -EAGAIN)
+		goto rescan;
+	return ret;
 }
 
 int invalidate_partitions(struct gendisk *disk, struct block_device *bdev)
-- 
2.20.1

Re: [PATCH 1/5] block: refactor rescan_partitions

[Hannes Reinecke]

On 11/6/19 4:14 PM, Christoph Hellwig wrote:
> Split out a helper that adds one single partition, and another one
> calling that dealing with the parsed_partitions state.  This makes
> it much more obvious how we clean up all state and start again when
> using the rescan label.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/partition-generic.c | 176 +++++++++++++++++++++-----------------
>  1 file changed, 96 insertions(+), 80 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH 1/5] block: refactor rescan_partitions

[Jan Kara]

On Wed 06-11-19 16:14:35, Christoph Hellwig wrote:
> Split out a helper that adds one single partition, and another one
> calling that dealing with the parsed_partitions state.  This makes
> it much more obvious how we clean up all state and start again when
> using the rescan label.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/partition-generic.c | 176 +++++++++++++++++++++-----------------
>  1 file changed, 96 insertions(+), 80 deletions(-)
> 
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index aee643ce13d1..f113be069b40 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -509,26 +509,77 @@ static bool part_zone_aligned(struct gendisk *disk,
>  	return true;
>  }
>  
> -int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
> +static bool blk_add_partition(struct gendisk *disk, struct block_device *bdev,
> +		struct parsed_partitions *state, int p)
>  {
> -	struct parsed_partitions *state = NULL;
> +	sector_t size = state->parts[p].size;
> +	sector_t from = state->parts[p].from;
>  	struct hd_struct *part;
> -	int p, highest, res;
> -rescan:
> -	if (state && !IS_ERR(state)) {
> -		free_partitions(state);
> -		state = NULL;
> +
> +	if (!size)
> +		return true;
> +
> +	if (from >= get_capacity(disk)) {
> +		printk(KERN_WARNING
> +		       "%s: p%d start %llu is beyond EOD, ",
> +		       disk->disk_name, p, (unsigned long long) from);
> +		if (disk_unlock_native_capacity(disk))
> +			return false;
> +		return true;
>  	}
>  
> -	res = drop_partitions(disk, bdev);
> -	if (res)
> -		return res;
> +	if (from + size > get_capacity(disk)) {
> +		printk(KERN_WARNING
> +		       "%s: p%d size %llu extends beyond EOD, ",
> +		       disk->disk_name, p, (unsigned long long) size);
>  
> -	if (disk->fops->revalidate_disk)
> -		disk->fops->revalidate_disk(disk);
> -	check_disk_size_change(disk, bdev, true);
> -	bdev->bd_invalidated = 0;
> -	if (!get_capacity(disk) || !(state = check_partition(disk, bdev)))
> +		if (disk_unlock_native_capacity(disk))
> +			return false;
> +
> +		/*
> +		 * We can not ignore partitions of broken tables created by for
> +		 * example camera firmware, but we limit them to the end of the
> +		 * disk to avoid creating invalid block devices.
> +		 */
> +		size = get_capacity(disk) - from;
> +	}
> +
> +	/*
> +	 * On a zoned block device, partitions should be aligned on the device
> +	 * zone size (i.e. zone boundary crossing not allowed).  Otherwise,
> +	 * resetting the write pointer of the last zone of one partition may
> +	 * impact the following partition.
> +	 */
> +	if (bdev_is_zoned(bdev) && !part_zone_aligned(disk, bdev, from, size)) {
> +		printk(KERN_WARNING
> +		       "%s: p%d start %llu+%llu is not zone aligned\n",
> +		       disk->disk_name, p, (unsigned long long) from,
> +		       (unsigned long long) size);
> +		return true;
> +	}
> +
> +	part = add_partition(disk, p, from, size, state->parts[p].flags,
> +			     &state->parts[p].info);
> +	if (IS_ERR(part)) {
> +		printk(KERN_ERR " %s: p%d could not be added: %ld\n",
> +		       disk->disk_name, p, -PTR_ERR(part));
> +		return true;
> +	}
> +
> +#ifdef CONFIG_BLK_DEV_MD
> +	if (state->parts[p].flags & ADDPART_FLAG_RAID)
> +		md_autodetect_dev(part_to_dev(part)->devt);
> +#endif
> +	return true;
> +}
> +
> +static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
> +{
> +	struct parsed_partitions *state;
> +	int ret = -EAGAIN, p, highest;
> +
> +	state = check_partition(disk, bdev);
> +	if (!state)
>  		return 0;
>  	if (IS_ERR(state)) {
>  		/*
> @@ -540,7 +591,7 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
>  			printk(KERN_WARNING "%s: partition table beyond EOD, ",
>  			       disk->disk_name);
>  			if (disk_unlock_native_capacity(disk))
> -				goto rescan;
> +				return -EAGAIN;
>  		}
>  		return -EIO;
>  	}
> @@ -554,7 +605,7 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
>  		       "%s: partition table partially beyond EOD, ",
>  		       disk->disk_name);
>  		if (disk_unlock_native_capacity(disk))
> -			goto rescan;
> +			goto out_free_state;
>  	}
>  
>  	/* tell userspace that the media / partition table may have changed */
> @@ -571,72 +622,37 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
>  	disk_expand_part_tbl(disk, highest);
>  
>  	/* add partitions */
> -	for (p = 1; p < state->limit; p++) {
> -		sector_t size, from;
> -
> -		size = state->parts[p].size;
> -		if (!size)
> -			continue;
> -
> -		from = state->parts[p].from;
> -		if (from >= get_capacity(disk)) {
> -			printk(KERN_WARNING
> -			       "%s: p%d start %llu is beyond EOD, ",
> -			       disk->disk_name, p, (unsigned long long) from);
> -			if (disk_unlock_native_capacity(disk))
> -				goto rescan;
> -			continue;
> -		}
> +	for (p = 1; p < state->limit; p++)
> +		if (!blk_add_partition(disk, bdev, state, p))
> +			goto out_free_state;
>  
> -		if (from + size > get_capacity(disk)) {
> -			printk(KERN_WARNING
> -			       "%s: p%d size %llu extends beyond EOD, ",
> -			       disk->disk_name, p, (unsigned long long) size);
> -
> -			if (disk_unlock_native_capacity(disk)) {
> -				/* free state and restart */
> -				goto rescan;
> -			} else {
> -				/*
> -				 * we can not ignore partitions of broken tables
> -				 * created by for example camera firmware, but
> -				 * we limit them to the end of the disk to avoid
> -				 * creating invalid block devices
> -				 */
> -				size = get_capacity(disk) - from;
> -			}
> -		}
> +	ret = 0;
> +out_free_state:
> +	free_partitions(state);
> +	return ret;
> +}
>  
> -		/*
> -		 * On a zoned block device, partitions should be aligned on the
> -		 * device zone size (i.e. zone boundary crossing not allowed).
> -		 * Otherwise, resetting the write pointer of the last zone of
> -		 * one partition may impact the following partition.
> -		 */
> -		if (bdev_is_zoned(bdev) &&
> -		    !part_zone_aligned(disk, bdev, from, size)) {
> -			printk(KERN_WARNING
> -			       "%s: p%d start %llu+%llu is not zone aligned\n",
> -			       disk->disk_name, p, (unsigned long long) from,
> -			       (unsigned long long) size);
> -			continue;
> -		}
> +int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
> +{
> +	int ret;
>  
> -		part = add_partition(disk, p, from, size,
> -				     state->parts[p].flags,
> -				     &state->parts[p].info);
> -		if (IS_ERR(part)) {
> -			printk(KERN_ERR " %s: p%d could not be added: %ld\n",
> -			       disk->disk_name, p, -PTR_ERR(part));
> -			continue;
> -		}
> -#ifdef CONFIG_BLK_DEV_MD
> -		if (state->parts[p].flags & ADDPART_FLAG_RAID)
> -			md_autodetect_dev(part_to_dev(part)->devt);
> -#endif
> -	}
> -	free_partitions(state);
> -	return 0;
> +rescan:
> +	ret = drop_partitions(disk, bdev);
> +	if (ret)
> +		return ret;
> +
> +	if (disk->fops->revalidate_disk)
> +		disk->fops->revalidate_disk(disk);
> +	check_disk_size_change(disk, bdev, true);
> +	bdev->bd_invalidated = 0;
> +
> +	if (!get_capacity(disk))
> +		return 0;
> +	
> +	ret = blk_add_partitions(disk, bdev);
> +	if (ret == -EAGAIN)
> +		goto rescan;
> +	return ret;
>  }
>  
>  int invalidate_partitions(struct gendisk *disk, struct block_device *bdev)
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/5] block: merge invalidate_partitions into rescan_partitions

[Christoph Hellwig]

A lot of the logic in invalidate_partitions and invalidate_partitions
is shared.  Merge the two functions to simplify things.  There is
a small behavior change in that we now send the keven change notice
also if we were not invalidating but no partitions were found, which
seems like the right thing to do.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/ioctl.c             |  2 +-
 block/partition-generic.c | 38 ++++++++++++++------------------------
 fs/block_dev.c            |  5 +----
 include/linux/genhd.h     |  4 ++--
 4 files changed, 18 insertions(+), 31 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 15a0eb80ada9..8a7e33ce2097 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -171,7 +171,7 @@ int __blkdev_reread_part(struct block_device *bdev)
 
 	lockdep_assert_held(&bdev->bd_mutex);
 
-	return rescan_partitions(disk, bdev);
+	return rescan_partitions(disk, bdev, false);
 }
 EXPORT_SYMBOL(__blkdev_reread_part);
 
diff --git a/block/partition-generic.c b/block/partition-generic.c
index f113be069b40..eae9daa8a523 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -632,7 +632,8 @@ static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
 	return ret;
 }
 
-int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
+int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
+		bool invalidate)
 {
 	int ret;
 
@@ -641,13 +642,22 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 	if (ret)
 		return ret;
 
-	if (disk->fops->revalidate_disk)
+	if (invalidate)
+		set_capacity(disk, 0);
+	else if (disk->fops->revalidate_disk)
 		disk->fops->revalidate_disk(disk);
-	check_disk_size_change(disk, bdev, true);
+
+	check_disk_size_change(disk, bdev, !invalidate);
 	bdev->bd_invalidated = 0;
 
-	if (!get_capacity(disk))
+	if (!get_capacity(disk)) {
+		/*
+		 * Tell userspace that the media / partition table may have
+		 * changed.
+		 */
+		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
 		return 0;
+	}
 	
 	ret = blk_add_partitions(disk, bdev);
 	if (ret == -EAGAIN)
@@ -655,26 +665,6 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
 	return ret;
 }
 
-int invalidate_partitions(struct gendisk *disk, struct block_device *bdev)
-{
-	int res;
-
-	if (!bdev->bd_invalidated)
-		return 0;
-
-	res = drop_partitions(disk, bdev);
-	if (res)
-		return res;
-
-	set_capacity(disk, 0);
-	check_disk_size_change(disk, bdev, false);
-	bdev->bd_invalidated = 0;
-	/* tell userspace that the media / partition table may have changed */
-	kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
-
-	return 0;
-}
-
 unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p)
 {
 	struct address_space *mapping = bdev->bd_inode->i_mapping;
diff --git a/fs/block_dev.c b/fs/block_dev.c
index d612468ee66b..0af62b76d031 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1511,10 +1511,7 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
 static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
 {
 	if (disk_part_scan_enabled(bdev->bd_disk)) {
-		if (invalidate)
-			invalidate_partitions(bdev->bd_disk, bdev);
-		else
-			rescan_partitions(bdev->bd_disk, bdev);
+		rescan_partitions(bdev->bd_disk, bdev, invalidate);
 	} else {
 		check_disk_size_change(bdev->bd_disk, bdev, !invalidate);
 		bdev->bd_invalidated = 0;
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 8b5330dd5ac0..fd7774e64f0b 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -622,8 +622,8 @@ extern dev_t blk_lookup_devt(const char *name, int partno);
 extern char *disk_name (struct gendisk *hd, int partno, char *buf);
 
 extern int disk_expand_part_tbl(struct gendisk *disk, int target);
-extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev);
-extern int invalidate_partitions(struct gendisk *disk, struct block_device *bdev);
+int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
+		bool invalidate);
 extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
 						     int partno, sector_t start,
 						     sector_t len, int flags,
-- 
2.20.1

Re: [PATCH 2/5] block: merge invalidate_partitions into rescan_partitions

[Keith Busch]

On Wed, Nov 06, 2019 at 04:14:36PM +0100, Christoph Hellwig wrote:
> A lot of the logic in invalidate_partitions and invalidate_partitions

One of these should say 'rescan_partitions'. Otherwise, patch looks good.

Re: [PATCH 2/5] block: merge invalidate_partitions into rescan_partitions

[Hannes Reinecke]

On 11/6/19 4:14 PM, Christoph Hellwig wrote:
> A lot of the logic in invalidate_partitions and invalidate_partitions

... rescan_partitions and invalidate_partitions ...

> is shared.  Merge the two functions to simplify things.  There is
> a small behavior change in that we now send the keven change notice
> also if we were not invalidating but no partitions were found, which
> seems like the right thing to do.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/ioctl.c             |  2 +-
>  block/partition-generic.c | 38 ++++++++++++++------------------------
>  fs/block_dev.c            |  5 +----
>  include/linux/genhd.h     |  4 ++--
>  4 files changed, 18 insertions(+), 31 deletions(-)
> 
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 15a0eb80ada9..8a7e33ce2097 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -171,7 +171,7 @@ int __blkdev_reread_part(struct block_device *bdev)
>  
>  	lockdep_assert_held(&bdev->bd_mutex);
>  
> -	return rescan_partitions(disk, bdev);
> +	return rescan_partitions(disk, bdev, false);
>  }
>  EXPORT_SYMBOL(__blkdev_reread_part);
>  
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index f113be069b40..eae9daa8a523 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -632,7 +632,8 @@ static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
>  	return ret;
>  }
>  
> -int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
> +int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
> +		bool invalidate)
>  {
>  	int ret;
>  
> @@ -641,13 +642,22 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
>  	if (ret)
>  		return ret;
>  
> -	if (disk->fops->revalidate_disk)
> +	if (invalidate)
> +		set_capacity(disk, 0);
> +	else if (disk->fops->revalidate_disk)
>  		disk->fops->revalidate_disk(disk);
> -	check_disk_size_change(disk, bdev, true);
> +
> +	check_disk_size_change(disk, bdev, !invalidate);
>  	bdev->bd_invalidated = 0;
>  
> -	if (!get_capacity(disk))
> +	if (!get_capacity(disk)) {
> +		/*
> +		 * Tell userspace that the media / partition table may have
> +		 * changed.
> +		 */
> +		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
>  		return 0;
> +	}
>  	
I wonder; wouldn't we miss a true size change here?

check_disk_size_change() will issue a kernel message (but no usevent) if
we have a real disk size change; if we fail to revalidate the disk an
uevent is issued, but no kernel message.

Can't we combine both by making check_disk_size_change() a bool function
and drop the call to 'get_capacity()', seeing that it's done in
check_disk_size_change() anyway?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH 2/5] block: merge invalidate_partitions into rescan_partitions

[Christoph Hellwig]

On Thu, Nov 07, 2019 at 10:47:39AM +0100, Hannes Reinecke wrote:
> > +	check_disk_size_change(disk, bdev, !invalidate);
> >  	bdev->bd_invalidated = 0;
> >  
> > -	if (!get_capacity(disk))
> > +	if (!get_capacity(disk)) {
> > +		/*
> > +		 * Tell userspace that the media / partition table may have
> > +		 * changed.
> > +		 */
> > +		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
> >  		return 0;
> > +	}
> >  	
> I wonder; wouldn't we miss a true size change here?

Why would we?  We first unconditionally drop all partitions.  Then if
the device ha a non-zero capacity we probe for partitions.  What could
we miss?

Re: [PATCH 2/5] block: merge invalidate_partitions into rescan_partitions

[Jan Kara]

On Wed 06-11-19 16:14:36, Christoph Hellwig wrote:
> A lot of the logic in invalidate_partitions and invalidate_partitions
			^^ rescan_partitions as others already mentioned

> is shared.  Merge the two functions to simplify things.  There is
> a small behavior change in that we now send the keven change notice
						  ^^^ kevent

> also if we were not invalidating but no partitions were found, which
> seems like the right thing to do.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Otherwise the patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  block/ioctl.c             |  2 +-
>  block/partition-generic.c | 38 ++++++++++++++------------------------
>  fs/block_dev.c            |  5 +----
>  include/linux/genhd.h     |  4 ++--
>  4 files changed, 18 insertions(+), 31 deletions(-)
> 
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 15a0eb80ada9..8a7e33ce2097 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -171,7 +171,7 @@ int __blkdev_reread_part(struct block_device *bdev)
>  
>  	lockdep_assert_held(&bdev->bd_mutex);
>  
> -	return rescan_partitions(disk, bdev);
> +	return rescan_partitions(disk, bdev, false);
>  }
>  EXPORT_SYMBOL(__blkdev_reread_part);
>  
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index f113be069b40..eae9daa8a523 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -632,7 +632,8 @@ static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
>  	return ret;
>  }
>  
> -int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
> +int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
> +		bool invalidate)
>  {
>  	int ret;
>  
> @@ -641,13 +642,22 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
>  	if (ret)
>  		return ret;
>  
> -	if (disk->fops->revalidate_disk)
> +	if (invalidate)
> +		set_capacity(disk, 0);
> +	else if (disk->fops->revalidate_disk)
>  		disk->fops->revalidate_disk(disk);
> -	check_disk_size_change(disk, bdev, true);
> +
> +	check_disk_size_change(disk, bdev, !invalidate);
>  	bdev->bd_invalidated = 0;
>  
> -	if (!get_capacity(disk))
> +	if (!get_capacity(disk)) {
> +		/*
> +		 * Tell userspace that the media / partition table may have
> +		 * changed.
> +		 */
> +		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
>  		return 0;
> +	}
>  	
>  	ret = blk_add_partitions(disk, bdev);
>  	if (ret == -EAGAIN)
> @@ -655,26 +665,6 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev)
>  	return ret;
>  }
>  
> -int invalidate_partitions(struct gendisk *disk, struct block_device *bdev)
> -{
> -	int res;
> -
> -	if (!bdev->bd_invalidated)
> -		return 0;
> -
> -	res = drop_partitions(disk, bdev);
> -	if (res)
> -		return res;
> -
> -	set_capacity(disk, 0);
> -	check_disk_size_change(disk, bdev, false);
> -	bdev->bd_invalidated = 0;
> -	/* tell userspace that the media / partition table may have changed */
> -	kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
> -
> -	return 0;
> -}
> -
>  unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p)
>  {
>  	struct address_space *mapping = bdev->bd_inode->i_mapping;
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index d612468ee66b..0af62b76d031 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1511,10 +1511,7 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
>  static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
>  {
>  	if (disk_part_scan_enabled(bdev->bd_disk)) {
> -		if (invalidate)
> -			invalidate_partitions(bdev->bd_disk, bdev);
> -		else
> -			rescan_partitions(bdev->bd_disk, bdev);
> +		rescan_partitions(bdev->bd_disk, bdev, invalidate);
>  	} else {
>  		check_disk_size_change(bdev->bd_disk, bdev, !invalidate);
>  		bdev->bd_invalidated = 0;
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 8b5330dd5ac0..fd7774e64f0b 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -622,8 +622,8 @@ extern dev_t blk_lookup_devt(const char *name, int partno);
>  extern char *disk_name (struct gendisk *hd, int partno, char *buf);
>  
>  extern int disk_expand_part_tbl(struct gendisk *disk, int target);
> -extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev);
> -extern int invalidate_partitions(struct gendisk *disk, struct block_device *bdev);
> +int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
> +		bool invalidate);
>  extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
>  						     int partno, sector_t start,
>  						     sector_t len, int flags,
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/5] block: move rescan_partitions to fs/block_dev.c

[Christoph Hellwig]

Large parts of rescan_partitions aren't about partitions, and
moving it to block_dev.c will allow for some further cleanups by
merging it into its only caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/partition-generic.c | 37 ++-----------------------------------
 fs/block_dev.c            | 38 ++++++++++++++++++++++++++++++++++++--
 include/linux/fs.h        |  2 --
 include/linux/genhd.h     |  4 ++--
 4 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/block/partition-generic.c b/block/partition-generic.c
index eae9daa8a523..7a6e406ac490 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -439,7 +439,7 @@ static bool disk_unlock_native_capacity(struct gendisk *disk)
 	}
 }
 
-static int drop_partitions(struct gendisk *disk, struct block_device *bdev)
+int blk_drop_partitions(struct gendisk *disk, struct block_device *bdev)
 {
 	struct disk_part_iter piter;
 	struct hd_struct *part;
@@ -573,7 +573,7 @@ static bool blk_add_partition(struct gendisk *disk, struct block_device *bdev,
 	return true;
 }
 
-static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
+int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
 {
 	struct parsed_partitions *state;
 	int ret = -EAGAIN, p, highest;
@@ -632,39 +632,6 @@ static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
 	return ret;
 }
 
-int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
-		bool invalidate)
-{
-	int ret;
-
-rescan:
-	ret = drop_partitions(disk, bdev);
-	if (ret)
-		return ret;
-
-	if (invalidate)
-		set_capacity(disk, 0);
-	else if (disk->fops->revalidate_disk)
-		disk->fops->revalidate_disk(disk);
-
-	check_disk_size_change(disk, bdev, !invalidate);
-	bdev->bd_invalidated = 0;
-
-	if (!get_capacity(disk)) {
-		/*
-		 * Tell userspace that the media / partition table may have
-		 * changed.
-		 */
-		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
-		return 0;
-	}
-	
-	ret = blk_add_partitions(disk, bdev);
-	if (ret == -EAGAIN)
-		goto rescan;
-	return ret;
-}
-
 unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p)
 {
 	struct address_space *mapping = bdev->bd_inode->i_mapping;
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 0af62b76d031..f0710085e1f1 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1416,8 +1416,8 @@ static void flush_disk(struct block_device *bdev, bool kill_dirty)
  * and adjusts it if it differs. When shrinking the bdev size, its all caches
  * are freed.
  */
-void check_disk_size_change(struct gendisk *disk, struct block_device *bdev,
-		bool verbose)
+static void check_disk_size_change(struct gendisk *disk,
+		struct block_device *bdev, bool verbose)
 {
 	loff_t disk_size, bdev_size;
 
@@ -1508,6 +1508,40 @@ EXPORT_SYMBOL(bd_set_size);
 
 static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
 
+static int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
+		bool invalidate)
+{
+	int ret;
+
+rescan:
+	ret = blk_drop_partitions(disk, bdev);
+	if (ret)
+		return ret;
+
+	if (invalidate)
+		set_capacity(disk, 0);
+	else if (disk->fops->revalidate_disk)
+		disk->fops->revalidate_disk(disk);
+
+	check_disk_size_change(disk, bdev, !invalidate);
+	bdev->bd_invalidated = 0;
+
+	if (!get_capacity(disk)) {
+		/*
+		 * Tell userspace that the media / partition table may have
+		 * changed.
+		 */
+		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
+		return 0;
+	}
+	
+	ret = blk_add_partitions(disk, bdev);
+	if (ret == -EAGAIN)
+		goto rescan;
+	return ret;
+}
+
+
 static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
 {
 	if (disk_part_scan_enabled(bdev->bd_disk)) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e0d909d35763..d233dd661df7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2703,8 +2703,6 @@ extern void make_bad_inode(struct inode *);
 extern bool is_bad_inode(struct inode *);
 
 #ifdef CONFIG_BLOCK
-extern void check_disk_size_change(struct gendisk *disk,
-		struct block_device *bdev, bool verbose);
 extern int revalidate_disk(struct gendisk *);
 extern int check_disk_change(struct block_device *);
 extern int __invalidate_device(struct block_device *, bool);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index fd7774e64f0b..f5cffbf63abf 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -621,9 +621,9 @@ extern void blk_invalidate_devt(dev_t devt);
 extern dev_t blk_lookup_devt(const char *name, int partno);
 extern char *disk_name (struct gendisk *hd, int partno, char *buf);
 
+int blk_add_partitions(struct gendisk *disk, struct block_device *bdev);
+int blk_drop_partitions(struct gendisk *disk, struct block_device *bdev);
 extern int disk_expand_part_tbl(struct gendisk *disk, int target);
-int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
-		bool invalidate);
 extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
 						     int partno, sector_t start,
 						     sector_t len, int flags,
-- 
2.20.1

Re: [PATCH 3/5] block: move rescan_partitions to fs/block_dev.c

[Hannes Reinecke]

On 11/6/19 4:14 PM, Christoph Hellwig wrote:
> Large parts of rescan_partitions aren't about partitions, and
> moving it to block_dev.c will allow for some further cleanups by
> merging it into its only caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/partition-generic.c | 37 ++-----------------------------------
>  fs/block_dev.c            | 38 ++++++++++++++++++++++++++++++++++++--
>  include/linux/fs.h        |  2 --
>  include/linux/genhd.h     |  4 ++--
>  4 files changed, 40 insertions(+), 41 deletions(-)
> 
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index eae9daa8a523..7a6e406ac490 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -439,7 +439,7 @@ static bool disk_unlock_native_capacity(struct gendisk *disk)
>  	}
>  }
>  
> -static int drop_partitions(struct gendisk *disk, struct block_device *bdev)
> +int blk_drop_partitions(struct gendisk *disk, struct block_device *bdev)
>  {
>  	struct disk_part_iter piter;
>  	struct hd_struct *part;
> @@ -573,7 +573,7 @@ static bool blk_add_partition(struct gendisk *disk, struct block_device *bdev,
>  	return true;
>  }
>  
> -static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
> +int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
>  {
>  	struct parsed_partitions *state;
>  	int ret = -EAGAIN, p, highest;
> @@ -632,39 +632,6 @@ static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
>  	return ret;
>  }
>  
> -int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
> -		bool invalidate)
> -{
> -	int ret;
> -
> -rescan:
> -	ret = drop_partitions(disk, bdev);
> -	if (ret)
> -		return ret;
> -
> -	if (invalidate)
> -		set_capacity(disk, 0);
> -	else if (disk->fops->revalidate_disk)
> -		disk->fops->revalidate_disk(disk);
> -
> -	check_disk_size_change(disk, bdev, !invalidate);
> -	bdev->bd_invalidated = 0;
> -
> -	if (!get_capacity(disk)) {
> -		/*
> -		 * Tell userspace that the media / partition table may have
> -		 * changed.
> -		 */
> -		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
> -		return 0;
> -	}
> -	
> -	ret = blk_add_partitions(disk, bdev);
> -	if (ret == -EAGAIN)
> -		goto rescan;
> -	return ret;
> -}
> -
>  unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p)
>  {
>  	struct address_space *mapping = bdev->bd_inode->i_mapping;
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 0af62b76d031..f0710085e1f1 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1416,8 +1416,8 @@ static void flush_disk(struct block_device *bdev, bool kill_dirty)
>   * and adjusts it if it differs. When shrinking the bdev size, its all caches
>   * are freed.
>   */
> -void check_disk_size_change(struct gendisk *disk, struct block_device *bdev,
> -		bool verbose)
> +static void check_disk_size_change(struct gendisk *disk,
> +		struct block_device *bdev, bool verbose)
>  {
>  	loff_t disk_size, bdev_size;
>  
> @@ -1508,6 +1508,40 @@ EXPORT_SYMBOL(bd_set_size);
>  
>  static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
>  
> +static int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
> +		bool invalidate)
> +{
> +	int ret;
> +
> +rescan:
> +	ret = blk_drop_partitions(disk, bdev);
> +	if (ret)
> +		return ret;
> +
> +	if (invalidate)
> +		set_capacity(disk, 0);
> +	else if (disk->fops->revalidate_disk)
> +		disk->fops->revalidate_disk(disk);
> +
> +	check_disk_size_change(disk, bdev, !invalidate);
> +	bdev->bd_invalidated = 0;
> +
> +	if (!get_capacity(disk)) {
> +		/*
> +		 * Tell userspace that the media / partition table may have
> +		 * changed.
> +		 */
> +		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
> +		return 0;
> +	}
> +	
> +	ret = blk_add_partitions(disk, bdev);
> +	if (ret == -EAGAIN)
> +		goto rescan;
> +	return ret;
> +}
> +
> +
>  static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
>  {
>  	if (disk_part_scan_enabled(bdev->bd_disk)) {

... see my comments in the previous patch to drop get_capacity() and
have check_disk_size_change() return bool ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH 3/5] block: move rescan_partitions to fs/block_dev.c

[Jan Kara]

On Wed 06-11-19 16:14:37, Christoph Hellwig wrote:
> Large parts of rescan_partitions aren't about partitions, and
> moving it to block_dev.c will allow for some further cleanups by
> merging it into its only caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/partition-generic.c | 37 ++-----------------------------------
>  fs/block_dev.c            | 38 ++++++++++++++++++++++++++++++++++++--
>  include/linux/fs.h        |  2 --
>  include/linux/genhd.h     |  4 ++--
>  4 files changed, 40 insertions(+), 41 deletions(-)
> 
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index eae9daa8a523..7a6e406ac490 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -439,7 +439,7 @@ static bool disk_unlock_native_capacity(struct gendisk *disk)
>  	}
>  }
>  
> -static int drop_partitions(struct gendisk *disk, struct block_device *bdev)
> +int blk_drop_partitions(struct gendisk *disk, struct block_device *bdev)
>  {
>  	struct disk_part_iter piter;
>  	struct hd_struct *part;
> @@ -573,7 +573,7 @@ static bool blk_add_partition(struct gendisk *disk, struct block_device *bdev,
>  	return true;
>  }
>  
> -static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
> +int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
>  {
>  	struct parsed_partitions *state;
>  	int ret = -EAGAIN, p, highest;
> @@ -632,39 +632,6 @@ static int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
>  	return ret;
>  }
>  
> -int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
> -		bool invalidate)
> -{
> -	int ret;
> -
> -rescan:
> -	ret = drop_partitions(disk, bdev);
> -	if (ret)
> -		return ret;
> -
> -	if (invalidate)
> -		set_capacity(disk, 0);
> -	else if (disk->fops->revalidate_disk)
> -		disk->fops->revalidate_disk(disk);
> -
> -	check_disk_size_change(disk, bdev, !invalidate);
> -	bdev->bd_invalidated = 0;
> -
> -	if (!get_capacity(disk)) {
> -		/*
> -		 * Tell userspace that the media / partition table may have
> -		 * changed.
> -		 */
> -		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
> -		return 0;
> -	}
> -	
> -	ret = blk_add_partitions(disk, bdev);
> -	if (ret == -EAGAIN)
> -		goto rescan;
> -	return ret;
> -}
> -
>  unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p)
>  {
>  	struct address_space *mapping = bdev->bd_inode->i_mapping;
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 0af62b76d031..f0710085e1f1 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1416,8 +1416,8 @@ static void flush_disk(struct block_device *bdev, bool kill_dirty)
>   * and adjusts it if it differs. When shrinking the bdev size, its all caches
>   * are freed.
>   */
> -void check_disk_size_change(struct gendisk *disk, struct block_device *bdev,
> -		bool verbose)
> +static void check_disk_size_change(struct gendisk *disk,
> +		struct block_device *bdev, bool verbose)
>  {
>  	loff_t disk_size, bdev_size;
>  
> @@ -1508,6 +1508,40 @@ EXPORT_SYMBOL(bd_set_size);
>  
>  static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
>  
> +static int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
> +		bool invalidate)
> +{
> +	int ret;
> +
> +rescan:
> +	ret = blk_drop_partitions(disk, bdev);
> +	if (ret)
> +		return ret;
> +
> +	if (invalidate)
> +		set_capacity(disk, 0);
> +	else if (disk->fops->revalidate_disk)
> +		disk->fops->revalidate_disk(disk);
> +
> +	check_disk_size_change(disk, bdev, !invalidate);
> +	bdev->bd_invalidated = 0;
> +
> +	if (!get_capacity(disk)) {
> +		/*
> +		 * Tell userspace that the media / partition table may have
> +		 * changed.
> +		 */
> +		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
> +		return 0;
> +	}
> +	
> +	ret = blk_add_partitions(disk, bdev);
> +	if (ret == -EAGAIN)
> +		goto rescan;
> +	return ret;
> +}
> +
> +
>  static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
>  {
>  	if (disk_part_scan_enabled(bdev->bd_disk)) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e0d909d35763..d233dd661df7 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2703,8 +2703,6 @@ extern void make_bad_inode(struct inode *);
>  extern bool is_bad_inode(struct inode *);
>  
>  #ifdef CONFIG_BLOCK
> -extern void check_disk_size_change(struct gendisk *disk,
> -		struct block_device *bdev, bool verbose);
>  extern int revalidate_disk(struct gendisk *);
>  extern int check_disk_change(struct block_device *);
>  extern int __invalidate_device(struct block_device *, bool);
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index fd7774e64f0b..f5cffbf63abf 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -621,9 +621,9 @@ extern void blk_invalidate_devt(dev_t devt);
>  extern dev_t blk_lookup_devt(const char *name, int partno);
>  extern char *disk_name (struct gendisk *hd, int partno, char *buf);
>  
> +int blk_add_partitions(struct gendisk *disk, struct block_device *bdev);
> +int blk_drop_partitions(struct gendisk *disk, struct block_device *bdev);
>  extern int disk_expand_part_tbl(struct gendisk *disk, int target);
> -int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
> -		bool invalidate);
>  extern struct hd_struct * __must_check add_partition(struct gendisk *disk,
>  						     int partno, sector_t start,
>  						     sector_t len, int flags,
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 4/5] block: fix bdev_disk_changed for non-partitioned devices

[Christoph Hellwig]

We still have to set the capacity to 0 if invalidating or call
revalidate_disk if not even if the disk has no partitions.  Fix
that by merging rescan_partitions into bdev_disk_changed and just
stubbing out blk_add_partitions and blk_drop_partitions for
non-partitioned devices.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/ioctl.c             |  6 ++----
 block/partition-generic.c |  5 +++++
 fs/block_dev.c            | 27 ++++++++-------------------
 include/linux/genhd.h     |  1 +
 4 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 8a7e33ce2097..52380c337078 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -162,16 +162,14 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
  */
 int __blkdev_reread_part(struct block_device *bdev)
 {
-	struct gendisk *disk = bdev->bd_disk;
-
-	if (!disk_part_scan_enabled(disk) || bdev != bdev->bd_contains)
+	if (!disk_part_scan_enabled(bdev->bd_disk) || bdev != bdev->bd_contains)
 		return -EINVAL;
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
 	lockdep_assert_held(&bdev->bd_mutex);
 
-	return rescan_partitions(disk, bdev, false);
+	return bdev_disk_changed(bdev, false);
 }
 EXPORT_SYMBOL(__blkdev_reread_part);
 
diff --git a/block/partition-generic.c b/block/partition-generic.c
index 7a6e406ac490..387d7e3a6bc4 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -445,6 +445,8 @@ int blk_drop_partitions(struct gendisk *disk, struct block_device *bdev)
 	struct hd_struct *part;
 	int res;
 
+	if (!disk_part_scan_enabled(disk))
+		return 0;
 	if (bdev->bd_part_count || bdev->bd_super)
 		return -EBUSY;
 	res = invalidate_partition(disk, 0);
@@ -578,6 +580,9 @@ int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
 	struct parsed_partitions *state;
 	int ret = -EAGAIN, p, highest;
 
+	if (!disk_part_scan_enabled(disk))
+		return 0;
+
 	state = check_partition(disk, bdev);
 	if (!state)
 		return 0;
diff --git a/fs/block_dev.c b/fs/block_dev.c
index f0710085e1f1..ae16466a67f7 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1508,9 +1508,9 @@ EXPORT_SYMBOL(bd_set_size);
 
 static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
 
-static int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
-		bool invalidate)
+int bdev_disk_changed(struct block_device *bdev, bool invalidate)
 {
+	struct gendisk *disk = bdev->bd_disk;
 	int ret;
 
 rescan:
@@ -1526,30 +1526,19 @@ static int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
 	check_disk_size_change(disk, bdev, !invalidate);
 	bdev->bd_invalidated = 0;
 
-	if (!get_capacity(disk)) {
+	if (get_capacity(disk)) {
+		ret = blk_add_partitions(disk, bdev);
+		if (ret == -EAGAIN)
+			goto rescan;
+	} else {
 		/*
 		 * Tell userspace that the media / partition table may have
 		 * changed.
 		 */
 		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
-		return 0;
 	}
-	
-	ret = blk_add_partitions(disk, bdev);
-	if (ret == -EAGAIN)
-		goto rescan;
-	return ret;
-}
 
-
-static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
-{
-	if (disk_part_scan_enabled(bdev->bd_disk)) {
-		rescan_partitions(bdev->bd_disk, bdev, invalidate);
-	} else {
-		check_disk_size_change(bdev->bd_disk, bdev, !invalidate);
-		bdev->bd_invalidated = 0;
-	}
+	return ret;
 }
 
 /*
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index f5cffbf63abf..8bb63027e4d6 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -621,6 +621,7 @@ extern void blk_invalidate_devt(dev_t devt);
 extern dev_t blk_lookup_devt(const char *name, int partno);
 extern char *disk_name (struct gendisk *hd, int partno, char *buf);
 
+int bdev_disk_changed(struct block_device *bdev, bool invalidate);
 int blk_add_partitions(struct gendisk *disk, struct block_device *bdev);
 int blk_drop_partitions(struct gendisk *disk, struct block_device *bdev);
 extern int disk_expand_part_tbl(struct gendisk *disk, int target);
-- 
2.20.1

Re: [PATCH 4/5] block: fix bdev_disk_changed for non-partitioned devices

[Jan Kara]

On Wed 06-11-19 16:14:38, Christoph Hellwig wrote:
> We still have to set the capacity to 0 if invalidating or call
> revalidate_disk if not even if the disk has no partitions.  Fix
> that by merging rescan_partitions into bdev_disk_changed and just
> stubbing out blk_add_partitions and blk_drop_partitions for
> non-partitioned devices.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

I'd just note that e.g. drivers/scsi/sr.c or drivers/scsi/sd.c already call
revalidate_disk() on device open so it seems a bit stupid to call it again
just a bit later. But that's not really a new thing, this patch just makes
it universal.

								Honza

> ---
>  block/ioctl.c             |  6 ++----
>  block/partition-generic.c |  5 +++++
>  fs/block_dev.c            | 27 ++++++++-------------------
>  include/linux/genhd.h     |  1 +
>  4 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 8a7e33ce2097..52380c337078 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -162,16 +162,14 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
>   */
>  int __blkdev_reread_part(struct block_device *bdev)
>  {
> -	struct gendisk *disk = bdev->bd_disk;
> -
> -	if (!disk_part_scan_enabled(disk) || bdev != bdev->bd_contains)
> +	if (!disk_part_scan_enabled(bdev->bd_disk) || bdev != bdev->bd_contains)
>  		return -EINVAL;
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EACCES;
>  
>  	lockdep_assert_held(&bdev->bd_mutex);
>  
> -	return rescan_partitions(disk, bdev, false);
> +	return bdev_disk_changed(bdev, false);
>  }
>  EXPORT_SYMBOL(__blkdev_reread_part);
>  
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 7a6e406ac490..387d7e3a6bc4 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -445,6 +445,8 @@ int blk_drop_partitions(struct gendisk *disk, struct block_device *bdev)
>  	struct hd_struct *part;
>  	int res;
>  
> +	if (!disk_part_scan_enabled(disk))
> +		return 0;
>  	if (bdev->bd_part_count || bdev->bd_super)
>  		return -EBUSY;
>  	res = invalidate_partition(disk, 0);
> @@ -578,6 +580,9 @@ int blk_add_partitions(struct gendisk *disk, struct block_device *bdev)
>  	struct parsed_partitions *state;
>  	int ret = -EAGAIN, p, highest;
>  
> +	if (!disk_part_scan_enabled(disk))
> +		return 0;
> +
>  	state = check_partition(disk, bdev);
>  	if (!state)
>  		return 0;
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index f0710085e1f1..ae16466a67f7 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1508,9 +1508,9 @@ EXPORT_SYMBOL(bd_set_size);
>  
>  static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
>  
> -static int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
> -		bool invalidate)
> +int bdev_disk_changed(struct block_device *bdev, bool invalidate)
>  {
> +	struct gendisk *disk = bdev->bd_disk;
>  	int ret;
>  
>  rescan:
> @@ -1526,30 +1526,19 @@ static int rescan_partitions(struct gendisk *disk, struct block_device *bdev,
>  	check_disk_size_change(disk, bdev, !invalidate);
>  	bdev->bd_invalidated = 0;
>  
> -	if (!get_capacity(disk)) {
> +	if (get_capacity(disk)) {
> +		ret = blk_add_partitions(disk, bdev);
> +		if (ret == -EAGAIN)
> +			goto rescan;
> +	} else {
>  		/*
>  		 * Tell userspace that the media / partition table may have
>  		 * changed.
>  		 */
>  		kobject_uevent(&disk_to_dev(disk)->kobj, KOBJ_CHANGE);
> -		return 0;
>  	}
> -	
> -	ret = blk_add_partitions(disk, bdev);
> -	if (ret == -EAGAIN)
> -		goto rescan;
> -	return ret;
> -}
>  
> -
> -static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
> -{
> -	if (disk_part_scan_enabled(bdev->bd_disk)) {
> -		rescan_partitions(bdev->bd_disk, bdev, invalidate);
> -	} else {
> -		check_disk_size_change(bdev->bd_disk, bdev, !invalidate);
> -		bdev->bd_invalidated = 0;
> -	}
> +	return ret;
>  }
>  
>  /*
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index f5cffbf63abf..8bb63027e4d6 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -621,6 +621,7 @@ extern void blk_invalidate_devt(dev_t devt);
>  extern dev_t blk_lookup_devt(const char *name, int partno);
>  extern char *disk_name (struct gendisk *hd, int partno, char *buf);
>  
> +int bdev_disk_changed(struct block_device *bdev, bool invalidate);
>  int blk_add_partitions(struct gendisk *disk, struct block_device *bdev);
>  int blk_drop_partitions(struct gendisk *disk, struct block_device *bdev);
>  extern int disk_expand_part_tbl(struct gendisk *disk, int target);
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/5] block: fix bdev_disk_changed for non-partitioned devices

[Christoph Hellwig]

On Thu, Nov 14, 2019 at 03:07:16PM +0100, Jan Kara wrote:
> I'd just note that e.g. drivers/scsi/sr.c or drivers/scsi/sd.c already call
> revalidate_disk() on device open so it seems a bit stupid to call it again
> just a bit later. But that's not really a new thing, this patch just makes
> it universal.

That whole area is a bit of a mess, including how the whole
->revalidate_disk callback works, and how we have a complicated events
mechanism, but then also md as the only remaining user of the legacy
->media_changed method.  I have a few idea on how to sort some of that
out, but it's going to take a while.

[PATCH 5/5] block: remove (__)blkdev_reread_part as an exported API

[Christoph Hellwig]

In general drivers should never mess with partition tables directly.
Unfortunately s390 and loop do for somewhat historic reasons, but they
can use bdev_disk_changed directly instead when we export it as they
satisfy the sanity checks we have in __blkdev_reread_part.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/ioctl.c                   | 35 +++++----------------------------
 drivers/block/loop.c            | 13 +++++++-----
 drivers/s390/block/dasd_genhd.c |  4 +++-
 fs/block_dev.c                  |  7 +++++++
 include/linux/fs.h              |  2 --
 5 files changed, 23 insertions(+), 38 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 52380c337078..2ed907ef0f01 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -155,46 +155,21 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
 	}
 }
 
-/*
- * This is an exported API for the block driver, and will not
- * acquire bd_mutex. This API should be used in case that
- * caller has held bd_mutex already.
- */
-int __blkdev_reread_part(struct block_device *bdev)
+static int blkdev_reread_part(struct block_device *bdev)
 {
+	int ret;
+
 	if (!disk_part_scan_enabled(bdev->bd_disk) || bdev != bdev->bd_contains)
 		return -EINVAL;
 	if (!capable(CAP_SYS_ADMIN))
 		return -EACCES;
 
-	lockdep_assert_held(&bdev->bd_mutex);
-
-	return bdev_disk_changed(bdev, false);
-}
-EXPORT_SYMBOL(__blkdev_reread_part);
-
-/*
- * This is an exported API for the block driver, and will
- * try to acquire bd_mutex. If bd_mutex has been held already
- * in current context, please call __blkdev_reread_part().
- *
- * Make sure the held locks in current context aren't required
- * in open()/close() handler and I/O path for avoiding ABBA deadlock:
- * - bd_mutex is held before calling block driver's open/close
- *   handler
- * - reading partition table may submit I/O to the block device
- */
-int blkdev_reread_part(struct block_device *bdev)
-{
-	int res;
-
 	mutex_lock(&bdev->bd_mutex);
-	res = __blkdev_reread_part(bdev);
+	ret = bdev_disk_changed(bdev, false);
 	mutex_unlock(&bdev->bd_mutex);
 
-	return res;
+	return ret;
 }
-EXPORT_SYMBOL(blkdev_reread_part);
 
 static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
 		unsigned long arg, unsigned long flags)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f6f77eaa7217..64b16abee280 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -630,7 +630,9 @@ static void loop_reread_partitions(struct loop_device *lo,
 {
 	int rc;
 
-	rc = blkdev_reread_part(bdev);
+	mutex_lock(&bdev->bd_mutex);
+	rc = bdev_disk_changed(bdev, false);
+	mutex_unlock(&bdev->bd_mutex);
 	if (rc)
 		pr_warn("%s: partition scan of loop%d (%s) failed (rc=%d)\n",
 			__func__, lo->lo_number, lo->lo_file_name, rc);
@@ -1154,10 +1156,11 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
 		 * must be at least one and it can only become zero when the
 		 * current holder is released.
 		 */
-		if (release)
-			err = __blkdev_reread_part(bdev);
-		else
-			err = blkdev_reread_part(bdev);
+		if (!release)
+			mutex_lock(&bdev->bd_mutex);
+		err = bdev_disk_changed(bdev, false);
+		if (!release)
+			mutex_unlock(&bdev->bd_mutex);
 		if (err)
 			pr_warn("%s: partition scan of loop%d failed (rc=%d)\n",
 				__func__, lo_number, err);
diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
index 5542d9eadfe0..7d079154f849 100644
--- a/drivers/s390/block/dasd_genhd.c
+++ b/drivers/s390/block/dasd_genhd.c
@@ -116,7 +116,9 @@ int dasd_scan_partitions(struct dasd_block *block)
 		return -ENODEV;
 	}
 
-	rc = blkdev_reread_part(bdev);
+	mutex_lock(&bdev->bd_mutex);
+	rc = bdev_disk_changed(bdev, false);
+	mutex_unlock(&bdev->bd_mutex);
 	if (rc)
 		DBF_DEV_EVENT(DBF_ERR, block->base,
 				"scan partitions error, rc %d", rc);
diff --git a/fs/block_dev.c b/fs/block_dev.c
index ae16466a67f7..9558a2f064b1 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1513,6 +1513,8 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
 	struct gendisk *disk = bdev->bd_disk;
 	int ret;
 
+	lockdep_assert_held(&bdev->bd_mutex);
+
 rescan:
 	ret = blk_drop_partitions(disk, bdev);
 	if (ret)
@@ -1540,6 +1542,11 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
 
 	return ret;
 }
+/*
+ * Only exported for for loop and dasd for historic reasons.  Don't use in new
+ * code!
+ */
+EXPORT_SYMBOL_GPL(bdev_disk_changed);
 
 /*
  * bd_mutex locking:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index d233dd661df7..ae6c5c37f3ae 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2632,8 +2632,6 @@ extern void bd_finish_claiming(struct block_device *bdev,
 extern void bd_abort_claiming(struct block_device *bdev,
 			      struct block_device *whole, void *holder);
 extern void blkdev_put(struct block_device *bdev, fmode_t mode);
-extern int __blkdev_reread_part(struct block_device *bdev);
-extern int blkdev_reread_part(struct block_device *bdev);
 
 #ifdef CONFIG_SYSFS
 extern int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk);
-- 
2.20.1

Re: [PATCH 5/5] block: remove (__)blkdev_reread_part as an exported API

[Stefan Haberland]

On 06.11.19 16:14, Christoph Hellwig wrote:
> In general drivers should never mess with partition tables directly.
> Unfortunately s390 and loop do for somewhat historic reasons, but they
> can use bdev_disk_changed directly instead when we export it as they
> satisfy the sanity checks we have in __blkdev_reread_part.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Hi,

tested (the whole patchset) with DASD on s390 and for the DASD part:

Reviewed-by: Stefan Haberland <sth@linux.ibm.com>

Re: [PATCH 5/5] block: remove (__)blkdev_reread_part as an exported API

[Jan Kara]

On Wed 06-11-19 16:14:39, Christoph Hellwig wrote:
> In general drivers should never mess with partition tables directly.
> Unfortunately s390 and loop do for somewhat historic reasons, but they
> can use bdev_disk_changed directly instead when we export it as they
> satisfy the sanity checks we have in __blkdev_reread_part.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/ioctl.c                   | 35 +++++----------------------------
>  drivers/block/loop.c            | 13 +++++++-----
>  drivers/s390/block/dasd_genhd.c |  4 +++-
>  fs/block_dev.c                  |  7 +++++++
>  include/linux/fs.h              |  2 --
>  5 files changed, 23 insertions(+), 38 deletions(-)
> 
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 52380c337078..2ed907ef0f01 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -155,46 +155,21 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user
>  	}
>  }
>  
> -/*
> - * This is an exported API for the block driver, and will not
> - * acquire bd_mutex. This API should be used in case that
> - * caller has held bd_mutex already.
> - */
> -int __blkdev_reread_part(struct block_device *bdev)
> +static int blkdev_reread_part(struct block_device *bdev)
>  {
> +	int ret;
> +
>  	if (!disk_part_scan_enabled(bdev->bd_disk) || bdev != bdev->bd_contains)
>  		return -EINVAL;
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EACCES;
>  
> -	lockdep_assert_held(&bdev->bd_mutex);
> -
> -	return bdev_disk_changed(bdev, false);
> -}
> -EXPORT_SYMBOL(__blkdev_reread_part);
> -
> -/*
> - * This is an exported API for the block driver, and will
> - * try to acquire bd_mutex. If bd_mutex has been held already
> - * in current context, please call __blkdev_reread_part().
> - *
> - * Make sure the held locks in current context aren't required
> - * in open()/close() handler and I/O path for avoiding ABBA deadlock:
> - * - bd_mutex is held before calling block driver's open/close
> - *   handler
> - * - reading partition table may submit I/O to the block device
> - */
> -int blkdev_reread_part(struct block_device *bdev)
> -{
> -	int res;
> -
>  	mutex_lock(&bdev->bd_mutex);
> -	res = __blkdev_reread_part(bdev);
> +	ret = bdev_disk_changed(bdev, false);
>  	mutex_unlock(&bdev->bd_mutex);
>  
> -	return res;
> +	return ret;
>  }
> -EXPORT_SYMBOL(blkdev_reread_part);
>  
>  static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
>  		unsigned long arg, unsigned long flags)
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index f6f77eaa7217..64b16abee280 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -630,7 +630,9 @@ static void loop_reread_partitions(struct loop_device *lo,
>  {
>  	int rc;
>  
> -	rc = blkdev_reread_part(bdev);
> +	mutex_lock(&bdev->bd_mutex);
> +	rc = bdev_disk_changed(bdev, false);
> +	mutex_unlock(&bdev->bd_mutex);
>  	if (rc)
>  		pr_warn("%s: partition scan of loop%d (%s) failed (rc=%d)\n",
>  			__func__, lo->lo_number, lo->lo_file_name, rc);
> @@ -1154,10 +1156,11 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
>  		 * must be at least one and it can only become zero when the
>  		 * current holder is released.
>  		 */
> -		if (release)
> -			err = __blkdev_reread_part(bdev);
> -		else
> -			err = blkdev_reread_part(bdev);
> +		if (!release)
> +			mutex_lock(&bdev->bd_mutex);
> +		err = bdev_disk_changed(bdev, false);
> +		if (!release)
> +			mutex_unlock(&bdev->bd_mutex);
>  		if (err)
>  			pr_warn("%s: partition scan of loop%d failed (rc=%d)\n",
>  				__func__, lo_number, err);
> diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
> index 5542d9eadfe0..7d079154f849 100644
> --- a/drivers/s390/block/dasd_genhd.c
> +++ b/drivers/s390/block/dasd_genhd.c
> @@ -116,7 +116,9 @@ int dasd_scan_partitions(struct dasd_block *block)
>  		return -ENODEV;
>  	}
>  
> -	rc = blkdev_reread_part(bdev);
> +	mutex_lock(&bdev->bd_mutex);
> +	rc = bdev_disk_changed(bdev, false);
> +	mutex_unlock(&bdev->bd_mutex);
>  	if (rc)
>  		DBF_DEV_EVENT(DBF_ERR, block->base,
>  				"scan partitions error, rc %d", rc);
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index ae16466a67f7..9558a2f064b1 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1513,6 +1513,8 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
>  	struct gendisk *disk = bdev->bd_disk;
>  	int ret;
>  
> +	lockdep_assert_held(&bdev->bd_mutex);
> +
>  rescan:
>  	ret = blk_drop_partitions(disk, bdev);
>  	if (ret)
> @@ -1540,6 +1542,11 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
>  
>  	return ret;
>  }
> +/*
> + * Only exported for for loop and dasd for historic reasons.  Don't use in new
> + * code!
> + */
> +EXPORT_SYMBOL_GPL(bdev_disk_changed);
>  
>  /*
>   * bd_mutex locking:
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index d233dd661df7..ae6c5c37f3ae 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2632,8 +2632,6 @@ extern void bd_finish_claiming(struct block_device *bdev,
>  extern void bd_abort_claiming(struct block_device *bdev,
>  			      struct block_device *whole, void *holder);
>  extern void blkdev_put(struct block_device *bdev, fmode_t mode);
> -extern int __blkdev_reread_part(struct block_device *bdev);
> -extern int blkdev_reread_part(struct block_device *bdev);
>  
>  #ifdef CONFIG_SYSFS
>  extern int bd_link_disk_holder(struct block_device *bdev, struct gendisk *disk);
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: disk revalidation cleanups and fixlets

[Jens Axboe]

On 11/6/19 8:14 AM, Christoph Hellwig wrote:
> Hi Jens and Jan,
> 
> this series takes the disk size change detection and revalidations
> from Jan a step further and fully integrate the code path for
> partitioned vs non-partitioned devices.  It also fixes up a few
> bits where we have unintentionally differing behavior.
> 

Were you going to re-send this on top of the other stuff for 5.5?
If so, should probably get queued up...

-- 
Jens Axboe

Re: disk revalidation cleanups and fixlets

[Christoph Hellwig]

On Thu, Nov 14, 2019 at 07:08:55AM -0700, Jens Axboe wrote:
> On 11/6/19 8:14 AM, Christoph Hellwig wrote:
> > Hi Jens and Jan,
> > 
> > this series takes the disk size change detection and revalidations
> > from Jan a step further and fully integrate the code path for
> > partitioned vs non-partitioned devices.  It also fixes up a few
> > bits where we have unintentionally differing behavior.
> > 
> 
> Were you going to re-send this on top of the other stuff for 5.5?
> If so, should probably get queued up...

Hmm.  I am pretty sure I resent it, but I can't find it in the
archives either.  Let me do that now with the typo fix and review
tags added to it.