[PATCH] Improve requeuing behavior

[PATCH] Improve requeuing behavior

[Bart Van Assche]

Requests are unprepared and reprepared when being requeued. Avoid
that requeuing resets .jiffies_at_alloc and .retries by initializing
these two member variables from inside blk_get_request() and by
preserving both member variables when preparing a request. This patch
affects the requeuing behavior of scsi-sq and scsi-mq.

Reported-by: Brian King <brking@linux.vnet.ibm.com>
References: https://lkml.org/lkml/2017/8/18/923 ("Re: [BUG][bisected 270065e] linux-next fails to boot on powerpc")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Brian King <brking@linux.vnet.ibm.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/scsi/scsi_lib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ebc5c713ee37..8d1ec1e7b0e2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1122,6 +1122,8 @@ void scsi_initialize_rq(struct request *rq)
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
 
 	scsi_req_init(&cmd->req);
+	cmd->jiffies_at_alloc = jiffies;
+	cmd->retries = 0;
 }
 EXPORT_SYMBOL(scsi_initialize_rq);
 
@@ -1160,6 +1162,8 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 	void *buf = cmd->sense_buffer;
 	void *prot = cmd->prot_sdb;
 	unsigned int unchecked_isa_dma = cmd->flags & SCMD_UNCHECKED_ISA_DMA;
+	unsigned long jiffies_at_alloc = cmd->jiffies_at_alloc;
+	int retries = cmd->retries;
 
 	/* zero out the cmd, except for the embedded scsi_request */
 	memset((char *)cmd + sizeof(cmd->req), 0,
@@ -1170,7 +1174,8 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 	cmd->prot_sdb = prot;
 	cmd->flags = unchecked_isa_dma;
 	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
-	cmd->jiffies_at_alloc = jiffies;
+	cmd->jiffies_at_alloc = jiffies_at_alloc;
+	cmd->retries = retries;
 
 	scsi_add_cmd_to_list(cmd);
 }
-- 
2.14.0

Re: [PATCH] Improve requeuing behavior

[Christoph Hellwig]

On Wed, Aug 23, 2017 at 02:05:35PM -0700, Bart Van Assche wrote:
> Requests are unprepared and reprepared when being requeued. Avoid
> that requeuing resets .jiffies_at_alloc and .retries by initializing
> these two member variables from inside blk_get_request() and by
> preserving both member variables when preparing a request. This patch
> affects the requeuing behavior of scsi-sq and scsi-mq.
> 
> Reported-by: Brian King <brking@linux.vnet.ibm.com>
> References: https://lkml.org/lkml/2017/8/18/923 ("Re: [BUG][bisected 270065e] linux-next fails to boot on powerpc")
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Brian King <brking@linux.vnet.ibm.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> ---
>  drivers/scsi/scsi_lib.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ebc5c713ee37..8d1ec1e7b0e2 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1122,6 +1122,8 @@ void scsi_initialize_rq(struct request *rq)
>  	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>  
>  	scsi_req_init(&cmd->req);
> +	cmd->jiffies_at_alloc = jiffies;
> +	cmd->retries = 0;
>  }
>  EXPORT_SYMBOL(scsi_initialize_rq);

How is this working for non-passthrough commands where we don't
call scsi_initialize_rq?

Re: [PATCH] Improve requeuing behavior

[Bart Van Assche]

On Thu, 2017-08-24 at 14:09 +0200, Christoph Hellwig wrote:
> On Wed, Aug 23, 2017 at 02:05:35PM -0700, Bart Van Assche wrote:
> > Requests are unprepared and reprepared when being requeued. Avoid
> > that requeuing resets .jiffies_at_alloc and .retries by initializing
> > these two member variables from inside blk_get_request() and by
> > preserving both member variables when preparing a request. This patch
> > affects the requeuing behavior of scsi-sq and scsi-mq.
> > 
> > Reported-by: Brian King <brking@linux.vnet.ibm.com>
> > References: https://lkml.org/lkml/2017/8/18/923 ("Re: [BUG][bisected 270065e] linux-next fails to boot on powerpc")
> > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> > Cc: Brian King <brking@linux.vnet.ibm.com>
> > Cc: Hannes Reinecke <hare@suse.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Johannes Thumshirn <jthumshirn@suse.de>
> > ---
> >  drivers/scsi/scsi_lib.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index ebc5c713ee37..8d1ec1e7b0e2 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1122,6 +1122,8 @@ void scsi_initialize_rq(struct request *rq)
> >  	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
> >  
> >  	scsi_req_init(&cmd->req);
> > +	cmd->jiffies_at_alloc = jiffies;
> > +	cmd->retries = 0;
> >  }
> >  EXPORT_SYMBOL(scsi_initialize_rq);
> 
> How is this working for non-passthrough commands where we don't
> call scsi_initialize_rq?

Hello Christoph,

Commit ca18d6f769d2 ("block: Make most scsi_req_init() calls implicit")
introduced the scsi_initialize_rq() function in such a way that it is not only
called for pass-through requests but also for FS requests. Because that commit
sets .initialize_rq_fn for both blk-sq and blk-mq blk_get_request() now calls
scsi_initialize_rq() for all SCSI requests. Or did I perhaps overlook something?

Bart.

Re: [PATCH] Improve requeuing behavior

[hch]

On Thu, Aug 24, 2017 at 04:14:22PM +0000, Bart Van Assche wrote:
> Commit ca18d6f769d2 ("block: Make most scsi_req_init() calls implicit")
> introduced the scsi_initialize_rq() function in such a way that it is not only
> called for pass-through requests but also for FS requests. Because that commit
> sets .initialize_rq_fn for both blk-sq and blk-mq blk_get_request() now calls
> scsi_initialize_rq() for all SCSI requests. Or did I perhaps overlook something?

initialize_rq_fn is only called from blk_get_request, which is not
called for normal file system read/write/flush/discard request.

Also unless my memory fails me that's something that I and Jens requested
to be changed from your original version.

Re: [PATCH] Improve requeuing behavior

[Bart Van Assche]

On Thu, 2017-08-24 at 18:57 +0200, hch@lst.de wrote:
> On Thu, Aug 24, 2017 at 04:14:22PM +0000, Bart Van Assche wrote:
> > Commit ca18d6f769d2 ("block: Make most scsi_req_init() calls implicit")
> > introduced the scsi_initialize_rq() function in such a way that it is not only
> > called for pass-through requests but also for FS requests. Because that commit
> > sets .initialize_rq_fn for both blk-sq and blk-mq blk_get_request() now calls
> > scsi_initialize_rq() for all SCSI requests. Or did I perhaps overlook something?
> 
> initialize_rq_fn is only called from blk_get_request, which is not
> called for normal file system read/write/flush/discard request.
> 
> Also unless my memory fails me that's something that I and Jens requested
> to be changed from your original version.

Hello Christoph,

Is one of the following two approaches perhaps what you had in mind?
* Adding an .initialize_rq_fn() call to multiple blk_rq_init() and
  blk_mq_rq_ctx_init() callers, e.g. blk_mq_make_request(), __get_request(),
  blk_kick_flush() and scsi_ioctl_reset().
* Moving the .initialize_rq_fn() call from blk_get_request() into blk_rq_init()
  and blk_mq_rq_ctx_init() (see also
  https://www.spinics.net/lists/linux-scsi/msg108934.html).

Thanks,

Bart.