linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* remove ->revalidate_disk (resend)
@ 2021-03-08  7:45 Christoph Hellwig
  2021-03-08  7:45 ` [PATCH 1/3] paride/pd: remove ->revalidate_disk Christoph Hellwig
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Christoph Hellwig @ 2021-03-08  7:45 UTC (permalink / raw)
  To: Jens Axboe, Tim Waugh; +Cc: linux-block

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] paride/pd: remove ->revalidate_disk
  2021-03-08  7:45 remove ->revalidate_disk (resend) Christoph Hellwig
@ 2021-03-08  7:45 ` Christoph Hellwig
  2021-03-08  7:45 ` [PATCH 2/3] umem: " Christoph Hellwig
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2021-03-08  7:45 UTC (permalink / raw)
  To: Jens Axboe, Tim Waugh; +Cc: linux-block

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/3] umem: remove ->revalidate_disk
  2021-03-08  7:45 remove ->revalidate_disk (resend) Christoph Hellwig
  2021-03-08  7:45 ` [PATCH 1/3] paride/pd: remove ->revalidate_disk Christoph Hellwig
@ 2021-03-08  7:45 ` Christoph Hellwig
  2021-03-08  7:45 ` [PATCH 3/3] block: remove the revalidate_disk method Christoph Hellwig
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2021-03-08  7:45 UTC (permalink / raw)
  To: Jens Axboe, Tim Waugh; +Cc: linux-block

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/3] block: remove the revalidate_disk method
  2021-03-08  7:45 remove ->revalidate_disk (resend) Christoph Hellwig
  2021-03-08  7:45 ` [PATCH 1/3] paride/pd: remove ->revalidate_disk Christoph Hellwig
  2021-03-08  7:45 ` [PATCH 2/3] umem: " Christoph Hellwig
@ 2021-03-08  7:45 ` Christoph Hellwig
  2021-03-29  5:55 ` remove ->revalidate_disk (resend) Christoph Hellwig
  2021-06-16  9:41 ` chenxiang (M)
  4 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2021-03-08  7:45 UTC (permalink / raw)
  To: Jens Axboe, Tim Waugh; +Cc: linux-block

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: remove ->revalidate_disk (resend)
  2021-03-08  7:45 remove ->revalidate_disk (resend) Christoph Hellwig
                   ` (2 preceding siblings ...)
  2021-03-08  7:45 ` [PATCH 3/3] block: remove the revalidate_disk method Christoph Hellwig
@ 2021-03-29  5:55 ` Christoph Hellwig
  2021-03-29 13:01   ` Jens Axboe
  2021-06-16  9:41 ` chenxiang (M)
  4 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2021-03-29  5:55 UTC (permalink / raw)
  To: Jens Axboe, Tim Waugh; +Cc: linux-block

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?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: remove ->revalidate_disk (resend)
  2021-03-29  5:55 ` remove ->revalidate_disk (resend) Christoph Hellwig
@ 2021-03-29 13:01   ` Jens Axboe
  2021-03-30  5:28     ` Christoph Hellwig
  0 siblings, 1 reply; 12+ messages in thread
From: Jens Axboe @ 2021-03-29 13:01 UTC (permalink / raw)
  To: Christoph Hellwig, Tim Waugh; +Cc: linux-block

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: remove ->revalidate_disk (resend)
  2021-03-29 13:01   ` Jens Axboe
@ 2021-03-30  5:28     ` Christoph Hellwig
  0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2021-03-30  5:28 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Christoph Hellwig, Tim Waugh, linux-block

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: remove ->revalidate_disk (resend)
  2021-03-08  7:45 remove ->revalidate_disk (resend) Christoph Hellwig
                   ` (3 preceding siblings ...)
  2021-03-29  5:55 ` remove ->revalidate_disk (resend) Christoph Hellwig
@ 2021-06-16  9:41 ` chenxiang (M)
  2021-06-16 13:50   ` Christoph Hellwig
  4 siblings, 1 reply; 12+ messages in thread
From: chenxiang (M) @ 2021-06-16  9:41 UTC (permalink / raw)
  To: Christoph Hellwig, Jens Axboe, Tim Waugh, martin.petersen
  Cc: linux-block, linux-scsi

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



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: remove ->revalidate_disk (resend)
  2021-06-16  9:41 ` chenxiang (M)
@ 2021-06-16 13:50   ` Christoph Hellwig
  2021-06-17  3:43     ` chenxiang (M)
  0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2021-06-16 13:50 UTC (permalink / raw)
  To: chenxiang (M)
  Cc: Christoph Hellwig, Jens Axboe, Tim Waugh, martin.petersen,
	linux-block, linux-scsi

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: remove ->revalidate_disk (resend)
  2021-06-16 13:50   ` Christoph Hellwig
@ 2021-06-17  3:43     ` chenxiang (M)
  2021-06-17 10:29       ` Christoph Hellwig
  0 siblings, 1 reply; 12+ messages in thread
From: chenxiang (M) @ 2021-06-17  3:43 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, Tim Waugh, martin.petersen, linux-block, linux-scsi,
	linuxarm

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

>
> .
>



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: remove ->revalidate_disk (resend)
  2021-06-17  3:43     ` chenxiang (M)
@ 2021-06-17 10:29       ` Christoph Hellwig
  2021-06-17 11:50         ` chenxiang (M)
  0 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2021-06-17 10:29 UTC (permalink / raw)
  To: chenxiang (M)
  Cc: Christoph Hellwig, Jens Axboe, Tim Waugh, martin.petersen,
	linux-block, linux-scsi, linuxarm

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: remove ->revalidate_disk (resend)
  2021-06-17 10:29       ` Christoph Hellwig
@ 2021-06-17 11:50         ` chenxiang (M)
  0 siblings, 0 replies; 12+ messages in thread
From: chenxiang (M) @ 2021-06-17 11:50 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, Tim Waugh, martin.petersen, linux-block, linux-scsi,
	linuxarm

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



^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-06-17 11:50 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-08  7:45 remove ->revalidate_disk (resend) Christoph Hellwig
2021-03-08  7:45 ` [PATCH 1/3] paride/pd: remove ->revalidate_disk Christoph Hellwig
2021-03-08  7:45 ` [PATCH 2/3] umem: " Christoph Hellwig
2021-03-08  7:45 ` [PATCH 3/3] block: remove the revalidate_disk method Christoph Hellwig
2021-03-29  5:55 ` remove ->revalidate_disk (resend) Christoph Hellwig
2021-03-29 13:01   ` Jens Axboe
2021-03-30  5:28     ` Christoph Hellwig
2021-06-16  9:41 ` chenxiang (M)
2021-06-16 13:50   ` Christoph Hellwig
2021-06-17  3:43     ` chenxiang (M)
2021-06-17 10:29       ` Christoph Hellwig
2021-06-17 11:50         ` chenxiang (M)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).