Re: remove ->revalidate_disk (resend)

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