[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()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5593 bytes --]

Hi Lee,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on scsi/for-next v5.8-rc4 next-20200708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lee-Sang-Hyun/sd-drain-a-request-queue-during-sd_shutdown/20200709-143319
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-rhel (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/scsi/sd.c: In function 'sd_shutdown':
>> drivers/scsi/sd.c:3567:28: error: 'sdp' undeclared (first use in this function); did you mean 'sdkp'?
    3567 |  struct request_queue *q = sdp->request_queue;
         |                            ^~~
         |                            sdkp
   drivers/scsi/sd.c:3567:28: note: each undeclared identifier is reported only once for each function it appears in
   In file included from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/scsi/sd.c:36:
>> drivers/scsi/sd.c:3586:21: error: incompatible type for argument 1 of 'spinlock_check'
    3586 |  spin_lock_irqsave(q->queue_lock, flags);
         |                    ~^~~~~~~~~~~~
         |                     |
         |                     spinlock_t {aka struct spinlock}
   include/linux/spinlock.h:251:34: note: in definition of macro 'raw_spin_lock_irqsave'
     251 |   flags = _raw_spin_lock_irqsave(lock); \
         |                                  ^~~~
   drivers/scsi/sd.c:3586:2: note: in expansion of macro 'spin_lock_irqsave'
    3586 |  spin_lock_irqsave(q->queue_lock, flags);
         |  ^~~~~~~~~~~~~~~~~
   In file included from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/scsi/sd.c:36:
   include/linux/spinlock.h:326:67: note: expected 'spinlock_t *' {aka 'struct spinlock *'} but argument is of type 'spinlock_t' {aka 'struct spinlock'}
     326 | static __always_inline raw_spinlock_t *spinlock_check(spinlock_t *lock)
         |                                                       ~~~~~~~~~~~~^~~~
>> drivers/scsi/sd.c:3587:2: error: implicit declaration of function 'queue_flag_set'; did you mean 'blk_queue_flag_set'? [-Werror=implicit-function-declaration]
    3587 |  queue_flag_set(QUEUE_FLAG_DYING, q);
         |  ^~~~~~~~~~~~~~
         |  blk_queue_flag_set
>> drivers/scsi/sd.c:3588:2: error: implicit declaration of function '__blk_drain_queue' [-Werror=implicit-function-declaration]
    3588 |  __blk_drain_queue(q, true);
         |  ^~~~~~~~~~~~~~~~~
>> drivers/scsi/sd.c:3590:26: error: incompatible type for argument 1 of 'spin_unlock_irqrestore'
    3590 |  spin_unlock_irqrestore(q->queue_lock, flags);
         |                         ~^~~~~~~~~~~~
         |                          |
         |                          spinlock_t {aka struct spinlock}
   In file included from include/linux/seqlock.h:36,
                    from include/linux/time.h:6,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/scsi/sd.c:36:
   include/linux/spinlock.h:406:64: note: expected 'spinlock_t *' {aka 'struct spinlock *'} but argument is of type 'spinlock_t' {aka 'struct spinlock'}
     406 | static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
         |                                                    ~~~~~~~~~~~~^~~~
   cc1: some warnings being treated as errors

vim +3567 drivers/scsi/sd.c

  3558	
  3559	/*
  3560	 * Send a SYNCHRONIZE CACHE instruction down to the device through
  3561	 * the normal SCSI command structure.  Wait for the command to
  3562	 * complete.
  3563	 */
  3564	static void sd_shutdown(struct device *dev)
  3565	{
  3566		struct scsi_disk *sdkp = dev_get_drvdata(dev);
> 3567		struct request_queue *q = sdp->request_queue;
  3568		unsigned long flags;
  3569	
  3570		if (!sdkp)
  3571			return;         /* this can happen */
  3572	
  3573		if (pm_runtime_suspended(dev))
  3574			return;
  3575	
  3576		if (sdkp->WCE && sdkp->media_present) {
  3577			sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
  3578			sd_sync_cache(sdkp, NULL);
  3579		}
  3580	
  3581		if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
  3582			sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
  3583			sd_start_stop_device(sdkp, 0);
  3584		}
  3585	
> 3586		spin_lock_irqsave(q->queue_lock, flags);
> 3587		queue_flag_set(QUEUE_FLAG_DYING, q);
> 3588		__blk_drain_queue(q, true);
  3589		queue_flag_set(QUEUE_FLAG_DEAD, q);
> 3590		spin_unlock_irqrestore(q->queue_lock, flags);
  3591	}
  3592	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 45455 bytes --]

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.