[RFC PATCH v1] sd: drain a request queue during sd_shutdown()

[RFC PATCH v1] sd: drain a request queue during sd_shutdown()

[Lee Sang Hyun]

Need to set a request queue like below in sd_shutdown()
to prevent pending IOs before UFS shutdown.

Change-Id: I2818cf95944d85baa50b626fcf538f19d06d6d54
Signed-off-by: Lee Sang Hyun <sh425.lee@samsung.com>
---
 drivers/scsi/sd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d90feff..7418d27 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3564,6 +3564,8 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
 static void sd_shutdown(struct device *dev)
 {
 	struct scsi_disk *sdkp = dev_get_drvdata(dev);
+	struct request_queue *q = sdp->request_queue;
+	unsigned long flags;
 
 	if (!sdkp)
 		return;         /* this can happen */
@@ -3580,6 +3582,12 @@ static void sd_shutdown(struct device *dev)
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		sd_start_stop_device(sdkp, 0);
 	}
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	queue_flag_set(QUEUE_FLAG_DYING, q);
+	__blk_drain_queue(q, true);
+	queue_flag_set(QUEUE_FLAG_DEAD, q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 
 static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
-- 
2.7.4

Re: [RFC PATCH v1] sd: drain a request queue during sd_shutdown()

[Bart Van Assche]

On 2020-07-08 23:23, Lee Sang Hyun wrote:
> Need to set a request queue like below in sd_shutdown()
> to prevent pending IOs before UFS shutdown.
> 
> Change-Id: I2818cf95944d85baa50b626fcf538f19d06d6d54
> Signed-off-by: Lee Sang Hyun <sh425.lee@samsung.com>
> ---
>  drivers/scsi/sd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d90feff..7418d27 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3564,6 +3564,8 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
>  static void sd_shutdown(struct device *dev)
>  {
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
> +	struct request_queue *q = sdp->request_queue;
> +	unsigned long flags;
>  
>  	if (!sdkp)
>  		return;         /* this can happen */
> @@ -3580,6 +3582,12 @@ static void sd_shutdown(struct device *dev)
>  		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
>  		sd_start_stop_device(sdkp, 0);
>  	}
> +
> +	spin_lock_irqsave(q->queue_lock, flags);
> +	queue_flag_set(QUEUE_FLAG_DYING, q);
> +	__blk_drain_queue(q, true);
> +	queue_flag_set(QUEUE_FLAG_DEAD, q);
> +	spin_unlock_irqrestore(q->queue_lock, flags);
>  }

This patch is unacceptable because:
(a) It introduces a layering violation. The code added in sd_shutdown()
    belongs in the block layer instead of the sd driver.
(b) A description of which problem has been observed and why the sd
    driver is being modified is missing.
(c) An explanation is missing why the blk_cleanup_queue() call in
    __scsi_remove_device() is not sufficient. As you may know
    blk_cleanup_queue() already drains the request queue.
(d) The above patch breaks the kernel build. __blk_drain_queue() was
    removed from the kernel almost two years ago. See also commit
    a1ce35fa4985 ("block: remove dead elevator code") # v5.0.

Bart.