remove ->revalidate_disk (resend)

remove ->revalidate_disk (resend)

[Christoph Hellwig]

Hi Jens,

with the previously merged patches all real users of ->revalidate_disk
are gone.  This series removes the two remaining not actually required
instances and the method itself.

[PATCH 1/3] paride/pd: remove ->revalidate_disk

[Christoph Hellwig]

->revalidate_disk is only called during add_disk for pd, but at that
point the driver has already set the capacity to the one returned from
Identify a little earlier, so this additional update is entirely
superflous.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/paride/pd.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index 897acda20ac85a..828a45ffe0e7d8 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -859,16 +859,6 @@ static unsigned int pd_check_events(struct gendisk *p, unsigned int clearing)
 	return r ? DISK_EVENT_MEDIA_CHANGE : 0;
 }
 
-static int pd_revalidate(struct gendisk *p)
-{
-	struct pd_unit *disk = p->private_data;
-	if (pd_special_command(disk, pd_identify) == 0)
-		set_capacity(p, disk->capacity);
-	else
-		set_capacity(p, 0);
-	return 0;
-}
-
 static const struct block_device_operations pd_fops = {
 	.owner		= THIS_MODULE,
 	.open		= pd_open,
@@ -877,7 +867,6 @@ static const struct block_device_operations pd_fops = {
 	.compat_ioctl	= pd_ioctl,
 	.getgeo		= pd_getgeo,
 	.check_events	= pd_check_events,
-	.revalidate_disk= pd_revalidate
 };
 
 /* probing */
-- 
2.30.1

[PATCH 2/3] umem: remove ->revalidate_disk

[Christoph Hellwig]

->revalidate_disk is only called during add_disk for pd, but at that
point the driver has already set the capacity to the same value a little
earlier, so this additional update is entirely superflous.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/umem.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/block/umem.c b/drivers/block/umem.c
index 982732dbe82e69..4c8320bfc46b5c 100644
--- a/drivers/block/umem.c
+++ b/drivers/block/umem.c
@@ -746,21 +746,6 @@ static void del_battery_timer(void)
 	del_timer(&battery_timer);
 }
 
-/*
- * Note no locks taken out here.  In a worst case scenario, we could drop
- * a chunk of system memory.  But that should never happen, since validation
- * happens at open or mount time, when locks are held.
- *
- *	That's crap, since doing that while some partitions are opened
- * or mounted will give you really nasty results.
- */
-static int mm_revalidate(struct gendisk *disk)
-{
-	struct cardinfo *card = disk->private_data;
-	set_capacity(disk, card->mm_size << 1);
-	return 0;
-}
-
 static int mm_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 {
 	struct cardinfo *card = bdev->bd_disk->private_data;
@@ -781,7 +766,6 @@ static const struct block_device_operations mm_fops = {
 	.owner		= THIS_MODULE,
 	.submit_bio	= mm_submit_bio,
 	.getgeo		= mm_getgeo,
-	.revalidate_disk = mm_revalidate,
 };
 
 static int mm_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
-- 
2.30.1

[PATCH 3/3] block: remove the revalidate_disk method

[Christoph Hellwig]

No implementations left.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/filesystems/locking.rst | 2 --
 fs/block_dev.c                        | 3 ---
 include/linux/blkdev.h                | 1 -
 3 files changed, 6 deletions(-)

diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index b7dcc86c92a45f..9774e92e449fbd 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -469,7 +469,6 @@ prototypes::
 	int (*direct_access) (struct block_device *, sector_t, void **,
 				unsigned long *);
 	void (*unlock_native_capacity) (struct gendisk *);
-	int (*revalidate_disk) (struct gendisk *);
 	int (*getgeo)(struct block_device *, struct hd_geometry *);
 	void (*swap_slot_free_notify) (struct block_device *, unsigned long);
 
@@ -484,7 +483,6 @@ ioctl:			no
 compat_ioctl:		no
 direct_access:		no
 unlock_native_capacity:	no
-revalidate_disk:	no
 getgeo:			no
 swap_slot_free_notify:	no	(see below)
 ======================= ===================
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 4aa1f88d5bf8b3..422d088ac7dbaa 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1250,9 +1250,6 @@ int bdev_disk_changed(struct block_device *bdev, bool invalidate)
 		if (disk_part_scan_enabled(disk) ||
 		    !(disk->flags & GENHD_FL_REMOVABLE))
 			set_capacity(disk, 0);
-	} else {
-		if (disk->fops->revalidate_disk)
-			disk->fops->revalidate_disk(disk);
 	}
 
 	if (get_capacity(disk)) {
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index bc6bc8383b434e..b4241f73f7a89c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1870,7 +1870,6 @@ struct block_device_operations {
 	unsigned int (*check_events) (struct gendisk *disk,
 				      unsigned int clearing);
 	void (*unlock_native_capacity) (struct gendisk *);
-	int (*revalidate_disk) (struct gendisk *);
 	int (*getgeo)(struct block_device *, struct hd_geometry *);
 	int (*set_read_only)(struct block_device *bdev, bool ro);
 	/* this callback is with swap_lock and sometimes page table lock held */
-- 
2.30.1

Re: remove ->revalidate_disk (resend)

[Christoph Hellwig]

On Mon, Mar 08, 2021 at 08:45:47AM +0100, Christoph Hellwig wrote:
> Hi Jens,
> 
> with the previously merged patches all real users of ->revalidate_disk
> are gone.  This series removes the two remaining not actually required
> instances and the method itself.

Jens,

can you consider this for the 5.13 tree?

Re: remove ->revalidate_disk (resend)

[Jens Axboe]

On 3/28/21 11:55 PM, Christoph Hellwig wrote:
> On Mon, Mar 08, 2021 at 08:45:47AM +0100, Christoph Hellwig wrote:
>> Hi Jens,
>>
>> with the previously merged patches all real users of ->revalidate_disk
>> are gone.  This series removes the two remaining not actually required
>> instances and the method itself.
> 
> Jens,
> 
> can you consider this for the 5.13 tree?

Looks fine to me, we just need to drop the umem change as it was
removed. And paride really should be as well... But in any case,
I'll queue up the other two or 5.13.

-- 
Jens Axboe

Re: remove ->revalidate_disk (resend)

[Christoph Hellwig]

On Mon, Mar 29, 2021 at 07:01:54AM -0600, Jens Axboe wrote:
> On 3/28/21 11:55 PM, Christoph Hellwig wrote:
> > On Mon, Mar 08, 2021 at 08:45:47AM +0100, Christoph Hellwig wrote:
> >> Hi Jens,
> >>
> >> with the previously merged patches all real users of ->revalidate_disk
> >> are gone.  This series removes the two remaining not actually required
> >> instances and the method itself.
> > 
> > Jens,
> > 
> > can you consider this for the 5.13 tree?
> 
> Looks fine to me, we just need to drop the umem change as it was
> removed. And paride really should be as well... But in any case,

Last time asked around (for the blk-mq conversion) we still had people
actively using it.

> I'll queue up the other two or 5.13.

So this ended up on the drivers branch.  I do have a buch of core
changes pending that will depend on it.

Re: remove ->revalidate_disk (resend)

[chenxiang (M)]

Hi,

Before i reported a issue related to revalidate disk 
(https://www.spinics.net/lists/linux-scsi/msg151610.html), and no one 
replies, but the issue is still.

And i plan to resend it, but i find that revalidate_disk interface is 
completely removed in this patchset.

Do you have any idea about the above issue?

在 2021/3/8 15:45, Christoph Hellwig 写道:
> Hi Jens,
>
> with the previously merged patches all real users of ->revalidate_disk
> are gone.  This series removes the two remaining not actually required
> instances and the method itself.
>
> .
>

Re: remove ->revalidate_disk (resend)

[Christoph Hellwig]

On Wed, Jun 16, 2021 at 05:41:54PM +0800, chenxiang (M) wrote:
> Hi,
>
> Before i reported a issue related to revalidate disk 
> (https://www.spinics.net/lists/linux-scsi/msg151610.html), and no one 
> replies, but the issue is still.
>
> And i plan to resend it, but i find that revalidate_disk interface is 
> completely removed in this patchset.
>
> Do you have any idea about the above issue?

bdev_disk_changed still calls into sd_revalidate_disk through sd_open.
How did bdev_disk_changed get called for you previously?  If it was
through the BLKRRPART ioctl please try latest mainline, which ensures
that ->open is called for that case.

Re: remove ->revalidate_disk (resend)

[chenxiang (M)]

Hi Christoph,


在 2021/6/16 21:50, Christoph Hellwig 写道:
> On Wed, Jun 16, 2021 at 05:41:54PM +0800, chenxiang (M) wrote:
>> Hi,
>>
>> Before i reported a issue related to revalidate disk
>> (https://www.spinics.net/lists/linux-scsi/msg151610.html), and no one
>> replies, but the issue is still.
>>
>> And i plan to resend it, but i find that revalidate_disk interface is
>> completely removed in this patchset.
>>
>> Do you have any idea about the above issue?
> bdev_disk_changed still calls into sd_revalidate_disk through sd_open.
> How did bdev_disk_changed get called for you previously?  If it was
> through the BLKRRPART ioctl please try latest mainline, which ensures
> that ->open is called for that case.

I use the latest mainline (Linux Euler 5.13.0-rc6-next-20210616), and 
the issue is still.
It is through BLKRRPART ioctl, and the call stack is as follows:

BLKRRPART ->
         block_ioctl ->
                 blkdev_ioctl ->
                         blkdev_common_ioctl ->
                                 blkdev_get_by_dev ->
                                         __blkdev_get ->
                                                 ...
disk->fops->open() ->
                                                         sd_open()
                                                 ...
                                                 dev_disk_changed()
                                                 ...



In function sd_open(), it calls sd_revalidate_disk() when 
sdev->removable or sdkp-> write_prot is true, but for our disk, 
sdev->removable = 0 and
sdkp->write_prot = 0, so sd_revalidate_disk() is not called actually.
For previous code, it will call sd_revalidate_disk() in 
bdev_disk_changed() from here 
(https://elixir.bootlin.com/linux/v5.10-rc1/source/fs/block_dev.c#L1411).

>
> .
>

Re: remove ->revalidate_disk (resend)

[Christoph Hellwig]

Can you try the patch below?

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5c8b5c5356a3..6d2d63629a90 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1387,6 +1387,22 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
 	}
 }
 
+static bool sd_need_revalidate(struct block_device *bdev,
+		struct scsi_disk *sdkp)
+{
+	if (sdkp->device->removable || sdkp->write_prot) {
+		if (bdev_check_media_change(bdev))
+			return true;
+	}
+
+	/*
+	 * Force a full rescan after ioctl(BLKRRPART).  While the disk state has
+	 * nothing to do with partitions, BLKRRPART is used to force a full
+	 * revalidate after things like a format for historical reasons.
+	 */
+	return test_bit(GD_NEED_PART_SCAN, &bdev->bd_disk->state);
+}
+
 /**
  *	sd_open - open a scsi disk device
  *	@bdev: Block device of the scsi disk to open
@@ -1423,10 +1439,8 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
 	if (!scsi_block_when_processing_errors(sdev))
 		goto error_out;
 
-	if (sdev->removable || sdkp->write_prot) {
-		if (bdev_check_media_change(bdev))
-			sd_revalidate_disk(bdev->bd_disk);
-	}
+	if (sd_need_revalidate(bdev, sdkp))
+		sd_revalidate_disk(bdev->bd_disk);
 
 	/*
 	 * If the drive is empty, just let the open fail.

Re: remove ->revalidate_disk (resend)

[chenxiang (M)]

Hi Christoph,


在 2021/6/17 18:29, Christoph Hellwig 写道:
> Can you try the patch below?

I have tested the patch, and it fixes the issue i reported, and so 
please feel free to add:
Tested-by: Xiang Chen <chenxiang66@hisilicon.com>

>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 5c8b5c5356a3..6d2d63629a90 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1387,6 +1387,22 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
>   	}
>   }
>   
> +static bool sd_need_revalidate(struct block_device *bdev,
> +		struct scsi_disk *sdkp)
> +{
> +	if (sdkp->device->removable || sdkp->write_prot) {
> +		if (bdev_check_media_change(bdev))
> +			return true;
> +	}
> +
> +	/*
> +	 * Force a full rescan after ioctl(BLKRRPART).  While the disk state has
> +	 * nothing to do with partitions, BLKRRPART is used to force a full
> +	 * revalidate after things like a format for historical reasons.
> +	 */
> +	return test_bit(GD_NEED_PART_SCAN, &bdev->bd_disk->state);
> +}
> +
>   /**
>    *	sd_open - open a scsi disk device
>    *	@bdev: Block device of the scsi disk to open
> @@ -1423,10 +1439,8 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
>   	if (!scsi_block_when_processing_errors(sdev))
>   		goto error_out;
>   
> -	if (sdev->removable || sdkp->write_prot) {
> -		if (bdev_check_media_change(bdev))
> -			sd_revalidate_disk(bdev->bd_disk);
> -	}
> +	if (sd_need_revalidate(bdev, sdkp))
> +		sd_revalidate_disk(bdev->bd_disk);
>   
>   	/*
>   	 * If the drive is empty, just let the open fail.
>
> .
>

remove ->revalidate_disk

[Christoph Hellwig]

Hi Jens,

with the previously merged patches all real users of ->revalidate_disk
are gone.  This series removes the two remaining not actually required
instances and the method itself.

[PATCH 1/3] paride/pd: remove ->revalidate_disk

[Christoph Hellwig]

->revalidate_disk is only called during add_disk for pd, but at that
point the driver has already set the capacity to the one returned from
Identify a little earlier, so this additional update is entirely
superflous.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/paride/pd.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index a7af4f27b7c3f1..022a7ea451307e 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -859,16 +859,6 @@ static unsigned int pd_check_events(struct gendisk *p, unsigned int clearing)
 	return r ? DISK_EVENT_MEDIA_CHANGE : 0;
 }
 
-static int pd_revalidate(struct gendisk *p)
-{
-	struct pd_unit *disk = p->private_data;
-	if (pd_special_command(disk, pd_identify) == 0)
-		set_capacity(p, disk->capacity);
-	else
-		set_capacity(p, 0);
-	return 0;
-}
-
 static const struct block_device_operations pd_fops = {
 	.owner		= THIS_MODULE,
 	.open		= pd_open,
@@ -877,7 +867,6 @@ static const struct block_device_operations pd_fops = {
 	.compat_ioctl	= pd_ioctl,
 	.getgeo		= pd_getgeo,
 	.check_events	= pd_check_events,
-	.revalidate_disk= pd_revalidate
 };
 
 /* probing */
-- 
2.28.0