[PATCH 00/18] Unify and simplify SCSI request initialization

[PATCH 00/18] Unify and simplify SCSI request initialization

[Bart Van Assche]

Hello Martin and James,

The patches in this series unify scsi-mq and scsi-sq request initialization,
significantly simplify request initialization and fix a recently introduced
bug in the request initialization code. Please consider these patches for
kernel v4.13.

Thanks,

Bart.

Bart Van Assche (18):
  block: Introduce blk_queue_cmd_size()
  bsg: Check private request size before attaching to a queue
  pktcdvd: Check private request size before attaching to a queue
  cdrom: Check private request size before attaching to a queue
  nfsd: Check private request size before submitting a SCSI request
  scsi: Make scsi_ioctl_reset() pass the request queue pointer to
    blk_rq_init()
  block: Introduce request_queue.initialize_rq_fn()
  block: Make scsi_req_init() calls implicit
  scsi: Change argument type of scsi_req_init()
  scsi: Only add commands to the device command list if required by the
    LLD
  scsi: Move most of scsi_init_command() into scsi_initialize_rq()
  scsi: Inline scsi_init_command()
  scsi: Move sense buffer pointer initialization into
    scsi_initialize_rq()
  scsi: Make scsi_initialize_rq() zero the entire struct scsi_cmnd
  scsi: storvsc: Initialize driver-private command before using it
  scsi-mq: Make behavior scsi_mq_prep_fn() closer to that of
    scsi_prep_fn()
  scsi: Consolidate more initialization code
  scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it

 block/blk-core.c                   | 13 ++++++
 block/blk-mq.c                     |  3 ++
 block/bsg.c                        |  7 ++-
 block/scsi_ioctl.c                 | 13 +++---
 drivers/block/pktcdvd.c            |  7 ++-
 drivers/cdrom/cdrom.c              |  6 ++-
 drivers/ide/ide-atapi.c            |  2 -
 drivers/ide/ide-cd.c               |  1 -
 drivers/ide/ide-cd_ioctl.c         |  1 -
 drivers/ide/ide-devsets.c          |  1 -
 drivers/ide/ide-disk.c             |  1 -
 drivers/ide/ide-ioctls.c           |  2 -
 drivers/ide/ide-park.c             |  2 -
 drivers/ide/ide-pm.c               |  2 -
 drivers/ide/ide-probe.c            |  6 +--
 drivers/ide/ide-tape.c             |  1 -
 drivers/ide/ide-taskfile.c         |  1 -
 drivers/scsi/osd/osd_initiator.c   |  2 -
 drivers/scsi/osst.c                |  1 -
 drivers/scsi/scsi.c                |  9 +---
 drivers/scsi/scsi_error.c          | 10 +++--
 drivers/scsi/scsi_lib.c            | 91 ++++++++++++++++++--------------------
 drivers/scsi/scsi_priv.h           |  3 +-
 drivers/scsi/scsi_transport_sas.c  |  6 +++
 drivers/scsi/sg.c                  |  2 -
 drivers/scsi/st.c                  |  1 -
 drivers/scsi/storvsc_drv.c         |  1 +
 drivers/target/target_core_pscsi.c |  2 -
 fs/nfsd/blocklayout.c              |  4 +-
 include/linux/blkdev.h             |  5 +++
 include/scsi/scsi_request.h        |  2 +-
 31 files changed, 112 insertions(+), 96 deletions(-)

-- 
2.12.2

[PATCH 01/18] block: Introduce blk_queue_cmd_size()

[Bart Van Assche]

This function will be used by later patches in this series.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: linux-block@vger.kernel.org
---
 block/blk-core.c       | 10 ++++++++++
 include/linux/blkdev.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index c7068520794b..a69d420b7ff0 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -601,6 +601,16 @@ static void free_request_simple(void *element, void *data)
 	kmem_cache_free(request_cachep, element);
 }
 
+/**
+ * blk_queue_cmd_size - size in bytes of driver-private request data
+ * @q: request queue pointer
+ */
+int blk_queue_cmd_size(struct request_queue *q)
+{
+	return q->mq_ops ? q->tag_set->cmd_size : q->cmd_size;
+}
+EXPORT_SYMBOL_GPL(blk_queue_cmd_size);
+
 static void *alloc_request_size(gfp_t gfp_mask, void *data)
 {
 	struct request_queue *q = data;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b5d1e27631ee..75b71374e1ba 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1212,6 +1212,7 @@ extern int blk_rq_map_sg(struct request_queue *, struct request *, struct scatte
 extern void blk_dump_rq_flags(struct request *, char *);
 extern long nr_blockdev_pages(void);
 
+int blk_queue_cmd_size(struct request_queue *q);
 bool __must_check blk_get_queue(struct request_queue *);
 struct request_queue *blk_alloc_queue(gfp_t);
 struct request_queue *blk_alloc_queue_node(gfp_t, int);
-- 
2.12.2

Re: [PATCH 01/18] block: Introduce blk_queue_cmd_size()

[Christoph Hellwig]

On Fri, May 19, 2017 at 11:29:59AM -0700, Bart Van Assche wrote:
> This function will be used by later patches in this series.

And it could already be used to simplify blk_alloc_flush_queue a bit..

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 02/18] bsg: Check private request size before attaching to a queue

[Bart Van Assche]

Since BSG only supports request queues for which struct scsi_request
is the first member of their private request data, refuse to register
block layer queues for which the private data is smaller than struct
scsi_request.

References: commit bd1599d931ca ("scsi_transport_sas: fix BSG ioctl memory corruption")
References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: linux-block@vger.kernel.org
---
 block/bsg.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/bsg.c b/block/bsg.c
index 6fd08544d77e..5ccecc9855ac 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -750,6 +750,12 @@ static struct bsg_device *bsg_add_device(struct inode *inode,
 #ifdef BSG_DEBUG
 	unsigned char buf[32];
 #endif
+
+	if (blk_queue_cmd_size(rq) < sizeof(struct scsi_request)) {
+		WARN_ONCE(true, "Attempt to register a non-SCSI queue\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (!blk_get_queue(rq))
 		return ERR_PTR(-ENXIO);
 
-- 
2.12.2

Re: [PATCH 02/18] bsg: Check private request size before attaching to a queue

[Christoph Hellwig]

Hi Bart,

I think this is the wrong kind of check - while we do care about the
size of the queue, we only do it as a side effect of the queue
being able to handle REQ_OP_SCSI_IN/REQ_OP_SCSI_OUT commands.

I think we'll need a flag for those in the queue instead.

And btw, I didn't get your cover letter [0/18], did that get lost
somewhere?

Re: [PATCH 02/18] bsg: Check private request size before attaching to a queue

[Bart Van Assche]

On Sun, 2017-05-21 at 08:32 +0200, Christoph Hellwig wrote:
> And btw, I didn't get your cover letter [0/18], did that get lost
> somewhere?

Hello Christoph,

Thanks for the review comments. The cover letter should have made it to at
least the linux-scsi mailing list since it shows up in at least one archive of
that mailing list: https://www.spinics.net/lists/linux-scsi/msg108940.html.

Bart.

Re: [PATCH 02/18] bsg: Check private request size before attaching to a queue

[hch]

On Sun, May 21, 2017 at 02:33:05PM +0000, Bart Van Assche wrote:
> Thanks for the review comments. The cover letter should have made it to at
> least the linux-scsi mailing list since it shows up in at least one archive of
> that mailing list: https://www.spinics.net/lists/linux-scsi/msg108940.html.

Yes, I see it on the list now.  But it's missing various Cc that the
actual patches have, including that to me, which seems a bit broken.

[PATCH 03/18] pktcdvd: Check private request size before attaching to a queue

[Bart Van Assche]

Since the pktcdvd driver only supports request queues for which
struct scsi_request is the first member of their private request
data, refuse to register block layer queues for which the private
data is smaller than struct scsi_request.

References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: linux-block@vger.kernel.org
---
 drivers/block/pktcdvd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 205b865ebeb9..4134bb54f6ad 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -2583,6 +2583,12 @@ static int pkt_new_dev(struct pktcdvd_device *pd, dev_t dev)
 	bdev = bdget(dev);
 	if (!bdev)
 		return -ENOMEM;
+	if (blk_queue_cmd_size(bdev_get_queue(bdev)) <
+	    sizeof(struct scsi_request)) {
+		WARN_ONCE(true, "Attempt to register a non-SCSI queue\n");
+		bdput(bdev);
+		return -EINVAL;
+	}
 	ret = blkdev_get(bdev, FMODE_READ | FMODE_NDELAY, NULL);
 	if (ret)
 		return ret;
-- 
2.12.2

[PATCH 04/18] cdrom: Check private request size before attaching to a queue

[Bart Van Assche]

Since the cdrom driver only supports request queues for which
struct scsi_request is the first member of their private request
data, refuse to register block layer queues for which the private
data is smaller than struct scsi_request.

References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: linux-block@vger.kernel.org
---
 drivers/cdrom/cdrom.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 76c952fd9ab9..dc20e4368136 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -594,6 +594,11 @@ int register_cdrom(struct cdrom_device_info *cdi)
 
 	if (cdo->open == NULL || cdo->release == NULL)
 		return -EINVAL;
+	if (blk_queue_cmd_size(cdi->disk->queue) <
+	    sizeof(struct scsi_request)) {
+		WARN_ONCE(true, "Attempt to register a non-SCSI queue\n");
+		return -EINVAL;
+	}
 	if (!banner_printed) {
 		pr_info("Uniform CD-ROM driver " REVISION "\n");
 		banner_printed = 1;
-- 
2.12.2

[PATCH 05/18] nfsd: Check private request size before submitting a SCSI request

[Bart Van Assche]

Since using scsi_req() is only allowed against request queues for which
struct scsi_request is the first member of their private request
data, refuse to register block layer queues for which the private
data is smaller than struct scsi_request.

References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: Jeff Layton <jlayton@poochiereds.net>
Cc: Jens Axboe <axboe@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: linux-nfs@vger.kernel.org
Cc: linux-block@vger.kernel.org
---
 fs/nfsd/blocklayout.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
index fb5213afc854..90d1df31491b 100644
--- a/fs/nfsd/blocklayout.c
+++ b/fs/nfsd/blocklayout.c
@@ -219,6 +219,9 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev,
 	u8 *buf, *d, type, assoc;
 	int error;
 
+	if (WARN_ON_ONCE(blk_queue_cmd_size(q) < sizeof(struct scsi_request)))
+		return -EINVAL;
+
 	buf = kzalloc(bufflen, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
-- 
2.12.2

Re: [PATCH 05/18] nfsd: Check private request size before submitting a SCSI request

[J . Bruce Fields]

ACK as far as I'm concerned.--b.

On Fri, May 19, 2017 at 11:30:03AM -0700, Bart Van Assche wrote:
> Since using scsi_req() is only allowed against request queues for which
> struct scsi_request is the first member of their private request
> data, refuse to register block layer queues for which the private
> data is smaller than struct scsi_request.
> 
> References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: J. Bruce Fields <bfields@fieldses.org>
> Cc: Jeff Layton <jlayton@poochiereds.net>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: linux-nfs@vger.kernel.org
> Cc: linux-block@vger.kernel.org
> ---
>  fs/nfsd/blocklayout.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
> index fb5213afc854..90d1df31491b 100644
> --- a/fs/nfsd/blocklayout.c
> +++ b/fs/nfsd/blocklayout.c
> @@ -219,6 +219,9 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev,
>  	u8 *buf, *d, type, assoc;
>  	int error;
>  
> +	if (WARN_ON_ONCE(blk_queue_cmd_size(q) < sizeof(struct scsi_request)))
> +		return -EINVAL;
> +
>  	buf = kzalloc(bufflen, GFP_KERNEL);
>  	if (!buf)
>  		return -ENOMEM;
> -- 
> 2.12.2

[PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Bart Van Assche]

A later patch will add a call to a request initialization function
into blk_rq_init(). Hence make sure that all blk_rq_init() calls
specify the request queue pointer. Since TMF callback functions in
SCSI LLD drivers do not use request.q, this patch does not change
the behavior of any SCSI driver.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_error.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ce654e35b060..23d6f225c671 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2286,7 +2286,12 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
 			shost->hostt->cmd_size, GFP_KERNEL);
 	if (!rq)
 		goto out_put_autopm_host;
-	blk_rq_init(NULL, rq);
+	/*
+	 * Although blk_rq_init() is intended for single queue block
+	 * drivers, this code path even uses blk_rq_init() when @dev is
+	 * a scsi-mq device.
+	 */
+	blk_rq_init(dev->request_queue, rq);
 
 	scmd = (struct scsi_cmnd *)(rq + 1);
 	scsi_init_command(dev, scmd);
-- 
2.12.2

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Although we really need to stop abusing requests/cmds for EH..
(Hannes, time to dust off your old patches!)

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Bart Van Assche]

On Sun, 2017-05-21 at 08:50 +0200, Christoph Hellwig wrote:
> Although we really need to stop abusing requests/cmds for EH..
> (Hannes, time to dust off your old patches!)

Hello Christoph and Hannes,

How about passing a struct scsi_device pointer to the eh_*_reset_handler
callbacks instead of a struct scsi_cmnd pointer? Most SCSI LLD
eh_*_reset_handler implementations don't do anything with the information
passed through the struct scsi_cmnd pointer except reading the SCSI device
pointer and logging the SCSI command CDB. Hannes, is this the same as your
proposal? Do you want to work on this or do you perhaps expect me to prepare
patches to implement that change?

Bart.

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Hannes Reinecke]

On 05/21/2017 06:41 PM, Bart Van Assche wrote:
> On Sun, 2017-05-21 at 08:50 +0200, Christoph Hellwig wrote:
>> Although we really need to stop abusing requests/cmds for EH..
>> (Hannes, time to dust off your old patches!)
> 
> Hello Christoph and Hannes,
> 
> How about passing a struct scsi_device pointer to the eh_*_reset_handler
> callbacks instead of a struct scsi_cmnd pointer? Most SCSI LLD
> eh_*_reset_handler implementations don't do anything with the information
> passed through the struct scsi_cmnd pointer except reading the SCSI device
> pointer and logging the SCSI command CDB. Hannes, is this the same as your
> proposal? Do you want to work on this or do you perhaps expect me to prepare
> patches to implement that change?
> 
If it were so easy.

Problem is that quite some LLDDs require a hardware tag for sending
TMFs, and currently the re-use the tag from the passed in scmd for that.
So first we need to move those to a sane interface, and having them
requesting a new tag for TMFs.
Should be made easier with Christophs rework, but we still don't have
any defined way how TMFs should request their tag; should they use
private tags? Should they use 'normal' tags? Should the driver implement
their own tag pool, seeing that these commands will never ever making
use of the associated request?

Hence I'm looking into implementing a REQ_RESET block request operation,
which then could be used to facilitate all of this (the request would be
allocated from the private tag pool if present).
It would also neatly solve the scsi_ioctl_reset() problem, as we then
could just issue a REQ_RESET and would avoid having to call into the
eh_* function directly.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[hch]

On Mon, May 22, 2017 at 08:06:46AM +0200, Hannes Reinecke wrote:
> Problem is that quite some LLDDs require a hardware tag for sending
> TMFs, and currently the re-use the tag from the passed in scmd for that.
> So first we need to move those to a sane interface, and having them
> requesting a new tag for TMFs.

Some do indeed.  But these seems to be the exception and not the rule.

> Should be made easier with Christophs rework, but we still don't have
> any defined way how TMFs should request their tag; should they use
> private tags? Should they use 'normal' tags? Should the driver implement
> their own tag pool, seeing that these commands will never ever making
> use of the associated request?

I think the right way to go is that every driver that needs tags for
TMFs should use private tags internally without the core knowing
about it.

> Hence I'm looking into implementing a REQ_RESET block request operation,
> which then could be used to facilitate all of this (the request would be
> allocated from the private tag pool if present).

That seems to be overkill to me for the few drivers.  And I suspect
most of them would be better off now even using blk-mq private tags
(which we'd have to implement for the legacy path first or just
kill it off) but just not expose a tag per host to the scsi and block
layers and set that aside.

> It would also neatly solve the scsi_ioctl_reset() problem, as we then
> could just issue a REQ_RESET and would avoid having to call into the
> eh_* function directly.

I don't think calling the eh_* methods is a problem per see.  It's just
their stupid calling conventions..

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Hannes Reinecke]

On 05/22/2017 09:54 AM, hch@lst.de wrote:
> On Mon, May 22, 2017 at 08:06:46AM +0200, Hannes Reinecke wrote:
>> Problem is that quite some LLDDs require a hardware tag for sending
>> TMFs, and currently the re-use the tag from the passed in scmd for that.
>> So first we need to move those to a sane interface, and having them
>> requesting a new tag for TMFs.
> 
> Some do indeed.  But these seems to be the exception and not the rule.
> 
Still need to be handled, though ...

>> Should be made easier with Christophs rework, but we still don't have
>> any defined way how TMFs should request their tag; should they use
>> private tags? Should they use 'normal' tags? Should the driver implement
>> their own tag pool, seeing that these commands will never ever making
>> use of the associated request?
> 
> I think the right way to go is that every driver that needs tags for
> TMFs should use private tags internally without the core knowing
> about it.
> 
That was my idea, too.

>> Hence I'm looking into implementing a REQ_RESET block request operation,
>> which then could be used to facilitate all of this (the request would be
>> allocated from the private tag pool if present).
> 
> That seems to be overkill to me for the few drivers.  And I suspect
> most of them would be better off now even using blk-mq private tags
> (which we'd have to implement for the legacy path first or just
> kill it off) but just not expose a tag per host to the scsi and block
> layers and set that aside.
> 
IE not using blk-mq private tags for EH? Hmm.
But then we'd need a SCSI-internal mechanism to get one of them. I
really would want to avoid having each driver to implement it's own
mechanism on how to get a TMF tag; that sort of thing only leads to
copy-and-paste errors.
Ok; will be looking into it.

>> It would also neatly solve the scsi_ioctl_reset() problem, as we then
>> could just issue a REQ_RESET and would avoid having to call into the
>> eh_* function directly.
> 
> I don't think calling the eh_* methods is a problem per see.  It's just
> their stupid calling conventions..
> 
I know. I've been working on a patchset to move at least eh_host_reset()
to take the scsi_host as argument. But even that requires some
preliminary patches to some LLDDs :-(

Right. Dusting off those patches, then.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[hch]

On Mon, May 22, 2017 at 10:46:10AM +0200, Hannes Reinecke wrote:
> > That seems to be overkill to me for the few drivers.  And I suspect
> > most of them would be better off now even using blk-mq private tags
> > (which we'd have to implement for the legacy path first or just
> > kill it off) but just not expose a tag per host to the scsi and block
> > layers and set that aside.
> > 
> IE not using blk-mq private tags for EH? Hmm.
> But then we'd need a SCSI-internal mechanism to get one of them. I
> really would want to avoid having each driver to implement it's own
> mechanism on how to get a TMF tag; that sort of thing only leads to
> copy-and-paste errors.
> Ok; will be looking into it.

No, we don't.  The driver simply sets a tag aside and doesn't expose
it to the block layer.  Similar to what smartpqi already does for LUN
resets and AENs, mpt3sas does for the ioctl tags and NVMe does for AERs.

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[Hannes Reinecke]

On 05/22/2017 02:48 PM, hch@lst.de wrote:
> On Mon, May 22, 2017 at 10:46:10AM +0200, Hannes Reinecke wrote:
>>> That seems to be overkill to me for the few drivers.  And I suspect
>>> most of them would be better off now even using blk-mq private tags
>>> (which we'd have to implement for the legacy path first or just
>>> kill it off) but just not expose a tag per host to the scsi and block
>>> layers and set that aside.
>>>
>> IE not using blk-mq private tags for EH? Hmm.
>> But then we'd need a SCSI-internal mechanism to get one of them. I
>> really would want to avoid having each driver to implement it's own
>> mechanism on how to get a TMF tag; that sort of thing only leads to
>> copy-and-paste errors.
>> Ok; will be looking into it.
> 
> No, we don't.  The driver simply sets a tag aside and doesn't expose
> it to the block layer.  Similar to what smartpqi already does for LUN
> resets and AENs, mpt3sas does for the ioctl tags and NVMe does for AERs.
> Personally I feel a bit uncomfortable by setting aside just one tag for
TMFs; this assumes we'll never be sending LUN resets to devices in parallel.

But maybe this is a discussion for another time, if and when we finally
move to that.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 06/18] scsi: Make scsi_ioctl_reset() pass the request queue pointer to blk_rq_init()

[hch]

On Mon, May 22, 2017 at 02:56:18PM +0200, Hannes Reinecke wrote:
> > No, we don't.  The driver simply sets a tag aside and doesn't expose
> > it to the block layer.  Similar to what smartpqi already does for LUN
> > resets and AENs, mpt3sas does for the ioctl tags and NVMe does for AERs.
> > Personally I feel a bit uncomfortable by setting aside just one tag for
> TMFs; this assumes we'll never be sending LUN resets to devices in parallel.
> 
> But maybe this is a discussion for another time, if and when we finally
> move to that.

For midlayer initiated EH that's the case - they are serialized by
being issued from the eh thread.  For the ioctl we have the
scsi_block_when_processing_errors / scsi_host_in_recovery magic,
although that hand crafted primitive is a bit of a mess, we'd
be much better off with a real EH mutex.

[PATCH 07/18] block: Introduce request_queue.initialize_rq_fn()

[Bart Van Assche]

Several block drivers need to initialize the driver-private data
after having called blk_get_request() and before .prep_rq_fn() is
called, e.g. when submitting a REQ_OP_SCSI_* request. Avoid that
that initialization code has to be repeated after every
blk_get_request() call by adding a new callback function to struct
request_queue.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: linux-block@vger.kernel.org
---
 block/blk-core.c       | 3 +++
 block/blk-mq.c         | 3 +++
 include/linux/blkdev.h | 4 ++++
 3 files changed, 10 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index a69d420b7ff0..f2540d164679 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -126,6 +126,9 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
 	rq->start_time = jiffies;
 	set_start_time_ns(rq);
 	rq->part = NULL;
+
+	if (q->initialize_rq_fn)
+		q->initialize_rq_fn(rq);
 }
 EXPORT_SYMBOL(blk_rq_init);
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index a69ad122ed66..2af43d4e5b96 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -241,6 +241,9 @@ void blk_mq_rq_ctx_init(struct request_queue *q, struct blk_mq_ctx *ctx,
 	rq->end_io_data = NULL;
 	rq->next_rq = NULL;
 
+	if (q->initialize_rq_fn)
+		q->initialize_rq_fn(rq);
+
 	ctx->rq_dispatched[op_is_sync(op)]++;
 }
 EXPORT_SYMBOL_GPL(blk_mq_rq_ctx_init);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 75b71374e1ba..2ee8da93619d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -410,8 +410,12 @@ struct request_queue {
 	rq_timed_out_fn		*rq_timed_out_fn;
 	dma_drain_needed_fn	*dma_drain_needed;
 	lld_busy_fn		*lld_busy_fn;
+	/* Called just after a request is allocated */
 	init_rq_fn		*init_rq_fn;
+	/* Called just before a request is freed */
 	exit_rq_fn		*exit_rq_fn;
+	/* Called from inside blk_get_request() */
+	void (*initialize_rq_fn)(struct request *rq);
 
 	const struct blk_mq_ops	*mq_ops;
 
-- 
2.12.2

Re: [PATCH 07/18] block: Introduce request_queue.initialize_rq_fn()

[Christoph Hellwig]

On Fri, May 19, 2017 at 11:30:05AM -0700, Bart Van Assche wrote:
> Several block drivers need to initialize the driver-private data
> after having called blk_get_request() and before .prep_rq_fn() is
> called, e.g. when submitting a REQ_OP_SCSI_* request. Avoid that
> that initialization code has to be repeated after every
> blk_get_request() call by adding a new callback function to struct
> request_queue.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Omar Sandoval <osandov@fb.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: linux-block@vger.kernel.org
> ---
>  block/blk-core.c       | 3 +++
>  block/blk-mq.c         | 3 +++
>  include/linux/blkdev.h | 4 ++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index a69d420b7ff0..f2540d164679 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -126,6 +126,9 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
>  	rq->start_time = jiffies;
>  	set_start_time_ns(rq);
>  	rq->part = NULL;
> +
> +	if (q->initialize_rq_fn)
> +		q->initialize_rq_fn(rq);

Can we keep this out of the fast path and only do it from the
blk_get_request / blk_mq_alloc_request path?  And while were at it
I think those two should be merged as far as the public interface
goes, that is we should also expose the flags argument for the
legacy path.

Re: [PATCH 07/18] block: Introduce request_queue.initialize_rq_fn()

[Bart Van Assche]

On Sun, 2017-05-21 at 08:34 +0200, Christoph Hellwig wrote:
> On Fri, May 19, 2017 at 11:30:05AM -0700, Bart Van Assche wrote:
> > Several block drivers need to initialize the driver-private data
> > after having called blk_get_request() and before .prep_rq_fn() is
> > called, e.g. when submitting a REQ_OP_SCSI_* request. Avoid that
> > that initialization code has to be repeated after every
> > blk_get_request() call by adding a new callback function to struct
> > request_queue.
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Jens Axboe <axboe@fb.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Omar Sandoval <osandov@fb.com>
> > Cc: Hannes Reinecke <hare@suse.com>
> > Cc: linux-block@vger.kernel.org
> > ---
> >  block/blk-core.c       | 3 +++
> >  block/blk-mq.c         | 3 +++
> >  include/linux/blkdev.h | 4 ++++
> >  3 files changed, 10 insertions(+)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index a69d420b7ff0..f2540d164679 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -126,6 +126,9 @@ void blk_rq_init(struct request_queue *q, struct request *rq)
> >  	rq->start_time = jiffies;
> >  	set_start_time_ns(rq);
> >  	rq->part = NULL;
> > +
> > +	if (q->initialize_rq_fn)
> > +		q->initialize_rq_fn(rq);
> 
> Can we keep this out of the fast path and only do it from the
> blk_get_request / blk_mq_alloc_request path?  And while were at it
> I think those two should be merged as far as the public interface
> goes, that is we should also expose the flags argument for the
> legacy path.

Hello Christoph,

For blk-mq I could move the .initialize_rq_fn() call from
blk_mq_rq_ctx_init() into blk_mq_sched_get_request(). I can't move it
higher up in the request allocation call chain because otherwise
blk_mq_alloc_request_hctx() wouldn't call .initialize_rq_fn().

For blk-sq I'm not sure that the .initialize_rq_fn() call can be
moved because I'd like all blk_rq_init() callers to call
.initialize_rq_fn(), including ide_prep_sense().

Are you sure it would help to move the .initialize_rq_fn() calls?

Thanks,

Bart.

[PATCH 08/18] block: Make scsi_req_init() calls implicit

[Bart Van Assche]

Instead of explicitly calling scsi_req_init(), let
blk_get_request() call that function from inside blk_rq_init().
Add an .initialize_rq_fn() callback function to the block drivers
that need it. Merge the IDE .init_rq_fn() function into
.initialize_rq_fn() because it is too small to keep it as a
separate function.

References: commit 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Jens Axboe <axboe@fb.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: linux-block@vger.kernel.org
---
 block/bsg.c                        |  1 -
 block/scsi_ioctl.c                 |  3 ---
 drivers/block/pktcdvd.c            |  1 -
 drivers/cdrom/cdrom.c              |  1 -
 drivers/ide/ide-atapi.c            |  2 --
 drivers/ide/ide-cd.c               |  1 -
 drivers/ide/ide-cd_ioctl.c         |  1 -
 drivers/ide/ide-devsets.c          |  1 -
 drivers/ide/ide-disk.c             |  1 -
 drivers/ide/ide-ioctls.c           |  2 --
 drivers/ide/ide-park.c             |  2 --
 drivers/ide/ide-pm.c               |  2 --
 drivers/ide/ide-probe.c            |  6 +++---
 drivers/ide/ide-tape.c             |  1 -
 drivers/ide/ide-taskfile.c         |  1 -
 drivers/scsi/osd/osd_initiator.c   |  2 --
 drivers/scsi/osst.c                |  1 -
 drivers/scsi/scsi_error.c          |  1 -
 drivers/scsi/scsi_lib.c            | 10 +++++++++-
 drivers/scsi/scsi_transport_sas.c  |  6 ++++++
 drivers/scsi/sg.c                  |  2 --
 drivers/scsi/st.c                  |  1 -
 drivers/target/target_core_pscsi.c |  2 --
 fs/nfsd/blocklayout.c              |  1 -
 24 files changed, 18 insertions(+), 34 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 5ccecc9855ac..0fe9b584fde0 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -236,7 +236,6 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm)
 	rq = blk_get_request(q, op, GFP_KERNEL);
 	if (IS_ERR(rq))
 		return rq;
-	scsi_req_init(rq);
 
 	ret = blk_fill_sgv4_hdr_rq(q, rq, hdr, bd, has_write_perm);
 	if (ret)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 4a294a5f7fab..f96c51f5df40 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -326,7 +326,6 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
 	if (IS_ERR(rq))
 		return PTR_ERR(rq);
 	req = scsi_req(rq);
-	scsi_req_init(rq);
 
 	if (hdr->cmd_len > BLK_MAX_CDB) {
 		req->cmd = kzalloc(hdr->cmd_len, GFP_KERNEL);
@@ -456,7 +455,6 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
 		goto error_free_buffer;
 	}
 	req = scsi_req(rq);
-	scsi_req_init(rq);
 
 	cmdlen = COMMAND_SIZE(opcode);
 
@@ -542,7 +540,6 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	rq = blk_get_request(q, REQ_OP_SCSI_OUT, __GFP_RECLAIM);
 	if (IS_ERR(rq))
 		return PTR_ERR(rq);
-	scsi_req_init(rq);
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	scsi_req(rq)->cmd[0] = cmd;
 	scsi_req(rq)->cmd[4] = data;
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 4134bb54f6ad..12dc1f334420 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -707,7 +707,6 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
 			     REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, __GFP_RECLAIM);
 	if (IS_ERR(rq))
 		return PTR_ERR(rq);
-	scsi_req_init(rq);
 
 	if (cgc->buflen) {
 		ret = blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen,
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index dc20e4368136..547157b8fa1f 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2200,7 +2200,6 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
 			break;
 		}
 		req = scsi_req(rq);
-		scsi_req_init(rq);
 
 		ret = blk_rq_map_user(q, rq, NULL, ubuf, len, GFP_KERNEL);
 		if (ret) {
diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 5901937284e7..7edebe0fb1eb 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -93,7 +93,6 @@ int ide_queue_pc_tail(ide_drive_t *drive, struct gendisk *disk,
 	int error;
 
 	rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	ide_req(rq)->type = ATA_PRIV_MISC;
 	rq->special = (char *)pc;
 
@@ -200,7 +199,6 @@ void ide_prep_sense(ide_drive_t *drive, struct request *rq)
 	memset(sense, 0, sizeof(*sense));
 
 	blk_rq_init(rq->q, sense_rq);
-	scsi_req_init(sense_rq);
 
 	err = blk_rq_map_kern(drive->queue, sense_rq, sense, sense_len,
 			      GFP_NOIO);
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 07e5ff3a64c3..a14ccb34c923 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -438,7 +438,6 @@ int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
 
 		rq = blk_get_request(drive->queue,
 			write ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN,  __GFP_RECLAIM);
-		scsi_req_init(rq);
 		memcpy(scsi_req(rq)->cmd, cmd, BLK_MAX_CDB);
 		ide_req(rq)->type = ATA_PRIV_PC;
 		rq->rq_flags |= rq_flags;
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 55cd736c39c6..9d26c9737e21 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -304,7 +304,6 @@ int ide_cdrom_reset(struct cdrom_device_info *cdi)
 	int ret;
 
 	rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	ide_req(rq)->type = ATA_PRIV_MISC;
 	rq->rq_flags = RQF_QUIET;
 	blk_execute_rq(drive->queue, cd->disk, rq, 0);
diff --git a/drivers/ide/ide-devsets.c b/drivers/ide/ide-devsets.c
index 9b69c32ee560..ef7c8c43a380 100644
--- a/drivers/ide/ide-devsets.c
+++ b/drivers/ide/ide-devsets.c
@@ -166,7 +166,6 @@ int ide_devset_execute(ide_drive_t *drive, const struct ide_devset *setting,
 		return setting->set(drive, arg);
 
 	rq = blk_get_request(q, REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	ide_req(rq)->type = ATA_PRIV_MISC;
 	scsi_req(rq)->cmd_len = 5;
 	scsi_req(rq)->cmd[0] = REQ_DEVSET_EXEC;
diff --git a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
index 7c06237f3479..241983da5fc4 100644
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -478,7 +478,6 @@ static int set_multcount(ide_drive_t *drive, int arg)
 		return -EBUSY;
 
 	rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	ide_req(rq)->type = ATA_PRIV_TASKFILE;
 
 	drive->mult_req = arg;
diff --git a/drivers/ide/ide-ioctls.c b/drivers/ide/ide-ioctls.c
index 8c0d17297a7a..3661abb16a5f 100644
--- a/drivers/ide/ide-ioctls.c
+++ b/drivers/ide/ide-ioctls.c
@@ -126,7 +126,6 @@ static int ide_cmd_ioctl(ide_drive_t *drive, unsigned long arg)
 		struct request *rq;
 
 		rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
-		scsi_req_init(rq);
 		ide_req(rq)->type = ATA_PRIV_TASKFILE;
 		blk_execute_rq(drive->queue, NULL, rq, 0);
 		err = scsi_req(rq)->result ? -EIO : 0;
@@ -224,7 +223,6 @@ static int generic_drive_reset(ide_drive_t *drive)
 	int ret = 0;
 
 	rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	ide_req(rq)->type = ATA_PRIV_MISC;
 	scsi_req(rq)->cmd_len = 1;
 	scsi_req(rq)->cmd[0] = REQ_DRIVE_RESET;
diff --git a/drivers/ide/ide-park.c b/drivers/ide/ide-park.c
index 94e3107f59b9..1f264d5d3f3f 100644
--- a/drivers/ide/ide-park.c
+++ b/drivers/ide/ide-park.c
@@ -32,7 +32,6 @@ static void issue_park_cmd(ide_drive_t *drive, unsigned long timeout)
 	spin_unlock_irq(&hwif->lock);
 
 	rq = blk_get_request(q, REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	scsi_req(rq)->cmd[0] = REQ_PARK_HEADS;
 	scsi_req(rq)->cmd_len = 1;
 	ide_req(rq)->type = ATA_PRIV_MISC;
@@ -48,7 +47,6 @@ static void issue_park_cmd(ide_drive_t *drive, unsigned long timeout)
 	 * timeout has expired, so power management will be reenabled.
 	 */
 	rq = blk_get_request(q, REQ_OP_DRV_IN, GFP_NOWAIT);
-	scsi_req_init(rq);
 	if (IS_ERR(rq))
 		goto out;
 
diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c
index 0977fc1f40ce..cfe3c2d7db7f 100644
--- a/drivers/ide/ide-pm.c
+++ b/drivers/ide/ide-pm.c
@@ -19,7 +19,6 @@ int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 
 	memset(&rqpm, 0, sizeof(rqpm));
 	rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	ide_req(rq)->type = ATA_PRIV_PM_SUSPEND;
 	rq->special = &rqpm;
 	rqpm.pm_step = IDE_PM_START_SUSPEND;
@@ -91,7 +90,6 @@ int generic_ide_resume(struct device *dev)
 
 	memset(&rqpm, 0, sizeof(rqpm));
 	rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	ide_req(rq)->type = ATA_PRIV_PM_RESUME;
 	rq->rq_flags |= RQF_PREEMPT;
 	rq->special = &rqpm;
diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
index 023562565d11..824eae707d25 100644
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -741,12 +741,12 @@ static void ide_port_tune_devices(ide_hwif_t *hwif)
 	}
 }
 
-static int ide_init_rq(struct request_queue *q, struct request *rq, gfp_t gfp)
+static void ide_initialize_rq(struct request *rq)
 {
 	struct ide_request *req = blk_mq_rq_to_pdu(rq);
 
+	scsi_req_init(rq);
 	req->sreq.sense = req->sense;
-	return 0;
 }
 
 /*
@@ -771,7 +771,7 @@ static int ide_init_queue(ide_drive_t *drive)
 		return 1;
 
 	q->request_fn = do_ide_request;
-	q->init_rq_fn = ide_init_rq;
+	q->initialize_rq_fn = ide_initialize_rq;
 	q->cmd_size = sizeof(struct ide_request);
 	if (blk_init_allocated_queue(q) < 0) {
 		blk_cleanup_queue(q);
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index a0651f948b76..370fd39dce94 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -855,7 +855,6 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int size)
 	BUG_ON(size < 0 || size % tape->blk_size);
 
 	rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	ide_req(rq)->type = ATA_PRIV_MISC;
 	scsi_req(rq)->cmd[13] = cmd;
 	rq->rq_disk = tape->disk;
diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index d71199d23c9e..d915a8eba557 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -433,7 +433,6 @@ int ide_raw_taskfile(ide_drive_t *drive, struct ide_cmd *cmd, u8 *buf,
 	rq = blk_get_request(drive->queue,
 		(cmd->tf_flags & IDE_TFLAG_WRITE) ?
 			REQ_OP_DRV_OUT : REQ_OP_DRV_IN, __GFP_RECLAIM);
-	scsi_req_init(rq);
 	ide_req(rq)->type = ATA_PRIV_TASKFILE;
 
 	/*
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 8a1b94816419..d974e7f1d2f1 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1572,7 +1572,6 @@ static struct request *_make_request(struct request_queue *q, bool has_write,
 			flags);
 	if (IS_ERR(req))
 		return req;
-	scsi_req_init(req);
 
 	for_each_bio(bio) {
 		struct bio *bounce_bio = bio;
@@ -1617,7 +1616,6 @@ static int _init_blk_request(struct osd_request *or,
 				ret = PTR_ERR(req);
 				goto out;
 			}
-			scsi_req_init(req);
 			or->in.req = or->request->next_rq = req;
 		}
 	} else if (has_in)
diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index 67cbed92f07d..22080148c6a8 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -373,7 +373,6 @@ static int osst_execute(struct osst_request *SRpnt, const unsigned char *cmd,
 		return DRIVER_ERROR << 24;
 
 	rq = scsi_req(req);
-	scsi_req_init(req);
 	req->rq_flags |= RQF_QUIET;
 
 	SRpnt->bio = NULL;
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 23d6f225c671..fbbdc345be85 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1915,7 +1915,6 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
 	if (IS_ERR(req))
 		return;
 	rq = scsi_req(req);
-	scsi_req_init(req);
 
 	rq->cmd[0] = ALLOW_MEDIUM_REMOVAL;
 	rq->cmd[1] = 0;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 22bc8701ba19..a294a010d585 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -250,7 +250,6 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	if (IS_ERR(req))
 		return ret;
 	rq = scsi_req(req);
-	scsi_req_init(req);
 
 	if (bufflen &&	blk_rq_map_kern(sdev->request_queue, req,
 					buffer, bufflen, __GFP_RECLAIM))
@@ -1134,6 +1133,13 @@ int scsi_init_io(struct scsi_cmnd *cmd)
 }
 EXPORT_SYMBOL(scsi_init_io);
 
+/* Called from inside blk_get_request() */
+static void scsi_initialize_rq(struct request *rq)
+{
+	scsi_req_init(rq);
+}
+
+/* Called after a request has been started. */
 void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 {
 	void *buf = cmd->sense_buffer;
@@ -2087,6 +2093,8 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q)
 	 * blk_queue_update_dma_alignment() later.
 	 */
 	blk_queue_dma_alignment(q, 0x03);
+
+	q->initialize_rq_fn = scsi_initialize_rq;
 }
 EXPORT_SYMBOL_GPL(__scsi_init_queue);
 
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 0ebe2f1bb908..6de65e73201d 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -213,6 +213,11 @@ static void sas_host_release(struct device *dev)
 		blk_cleanup_queue(q);
 }
 
+static void sas_initialize_rq(struct request *rq)
+{
+	scsi_req_init(rq);
+}
+
 static int sas_bsg_initialize(struct Scsi_Host *shost, struct sas_rphy *rphy)
 {
 	struct request_queue *q;
@@ -230,6 +235,7 @@ static int sas_bsg_initialize(struct Scsi_Host *shost, struct sas_rphy *rphy)
 	q = blk_alloc_queue(GFP_KERNEL);
 	if (!q)
 		return -ENOMEM;
+	q->initialize_rq_fn = sas_initialize_rq;
 	q->cmd_size = sizeof(struct scsi_request);
 
 	if (rphy) {
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 0a38ba01b7b4..071a7fe27c11 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1732,8 +1732,6 @@ sg_start_req(Sg_request *srp, unsigned char *cmd)
 	}
 	req = scsi_req(rq);
 
-	scsi_req_init(rq);
-
 	if (hp->cmd_len > BLK_MAX_CDB)
 		req->cmd = long_cmdp;
 	memcpy(req->cmd, cmd, hp->cmd_len);
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 1ea34d6f5437..dc4d2b9e15a0 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -549,7 +549,6 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd,
 	if (IS_ERR(req))
 		return DRIVER_ERROR << 24;
 	rq = scsi_req(req);
-	scsi_req_init(req);
 	req->rq_flags |= RQF_QUIET;
 
 	mdata->null_mapped = 1;
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index a93d94e68ab5..4df4fd5783c3 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -992,8 +992,6 @@ pscsi_execute_cmd(struct se_cmd *cmd)
 		goto fail;
 	}
 
-	scsi_req_init(req);
-
 	if (sgl) {
 		ret = pscsi_map_sg(cmd, sgl, sgl_nents, req);
 		if (ret)
diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
index 90d1df31491b..d5a1f6c015c9 100644
--- a/fs/nfsd/blocklayout.c
+++ b/fs/nfsd/blocklayout.c
@@ -232,7 +232,6 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev,
 		goto out_free_buf;
 	}
 	req = scsi_req(rq);
-	scsi_req_init(rq);
 
 	error = blk_rq_map_kern(q, rq, buf, bufflen, GFP_KERNEL);
 	if (error)
-- 
2.12.2

Re: [PATCH 08/18] block: Make scsi_req_init() calls implicit

[Christoph Hellwig]

On Fri, May 19, 2017 at 11:30:06AM -0700, Bart Van Assche wrote:
> Instead of explicitly calling scsi_req_init(), let
> blk_get_request() call that function from inside blk_rq_init().
> Add an .initialize_rq_fn() callback function to the block drivers
> that need it.

Thanks Bart,

this looks like a very nice cleanup.

> Merge the IDE .init_rq_fn() function into
> .initialize_rq_fn() because it is too small to keep it as a
> separate function.

Although we can't do this IFF we don't call .initialize_rq_fn from
the fast path (which I think we should).

[PATCH 09/18] scsi: Change argument type of scsi_req_init()

[Bart Van Assche]

Since scsi_req_init() works on a struct scsi_request, change the
argument type into struct scsi_request *.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 block/scsi_ioctl.c                | 10 +++++++---
 drivers/ide/ide-probe.c           |  2 +-
 drivers/scsi/scsi_lib.c           |  4 +++-
 drivers/scsi/scsi_transport_sas.c |  2 +-
 include/scsi/scsi_request.h       |  2 +-
 5 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index f96c51f5df40..7440de44dd85 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -741,10 +741,14 @@ int scsi_cmd_blk_ioctl(struct block_device *bd, fmode_t mode,
 }
 EXPORT_SYMBOL(scsi_cmd_blk_ioctl);
 
-void scsi_req_init(struct request *rq)
+/**
+ * scsi_req_init - initialize certain fields of a scsi_request structure
+ * @req: Pointer to a scsi_request structure.
+ * Initializes .__cmd[], .cmd, .cmd_len and .sense_len but no other members
+ * of struct scsi_request.
+ */
+void scsi_req_init(struct scsi_request *req)
 {
-	struct scsi_request *req = scsi_req(rq);
-
 	memset(req->__cmd, 0, sizeof(req->__cmd));
 	req->cmd = req->__cmd;
 	req->cmd_len = BLK_MAX_CDB;
diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
index 824eae707d25..5f77e31c9487 100644
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -745,7 +745,7 @@ static void ide_initialize_rq(struct request *rq)
 {
 	struct ide_request *req = blk_mq_rq_to_pdu(rq);
 
-	scsi_req_init(rq);
+	scsi_req_init(&req->sreq);
 	req->sreq.sense = req->sense;
 }
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a294a010d585..efa5741cab02 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1136,7 +1136,9 @@ EXPORT_SYMBOL(scsi_init_io);
 /* Called from inside blk_get_request() */
 static void scsi_initialize_rq(struct request *rq)
 {
-	scsi_req_init(rq);
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
+
+	scsi_req_init(&cmd->req);
 }
 
 /* Called after a request has been started. */
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 6de65e73201d..fc049b0265ee 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -215,7 +215,7 @@ static void sas_host_release(struct device *dev)
 
 static void sas_initialize_rq(struct request *rq)
 {
-	scsi_req_init(rq);
+	scsi_req_init(scsi_req(rq));
 }
 
 static int sas_bsg_initialize(struct Scsi_Host *shost, struct sas_rphy *rphy)
diff --git a/include/scsi/scsi_request.h b/include/scsi/scsi_request.h
index f0c76f9dc285..e0afa445ee4e 100644
--- a/include/scsi/scsi_request.h
+++ b/include/scsi/scsi_request.h
@@ -27,6 +27,6 @@ static inline void scsi_req_free_cmd(struct scsi_request *req)
 		kfree(req->cmd);
 }
 
-void scsi_req_init(struct request *);
+void scsi_req_init(struct scsi_request *req);
 
 #endif /* _SCSI_SCSI_REQUEST_H */
-- 
2.12.2

Re: [PATCH 09/18] scsi: Change argument type of scsi_req_init()

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 10/18] scsi: Only add commands to the device command list if required by the LLD

[Bart Van Assche]

Just like for the scsi-mq code path, in the single queue SCSI code
path only add commands to the per-device command list if required
by the SCSI LLD. This patch will make it easier to merge the
single-queue and multiqueue command initialization code.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi.c      |  9 +--------
 drivers/scsi/scsi_lib.c  | 52 +++++++++++++++++++++++++++++-------------------
 drivers/scsi/scsi_priv.h |  2 ++
 3 files changed, 35 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 7bfbcfa7af40..485684aafb9b 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -108,14 +108,7 @@ EXPORT_SYMBOL(scsi_sd_pm_domain);
  */
 void scsi_put_command(struct scsi_cmnd *cmd)
 {
-	unsigned long flags;
-
-	/* serious error if the command hasn't come from a device list */
-	spin_lock_irqsave(&cmd->device->list_lock, flags);
-	BUG_ON(list_empty(&cmd->list));
-	list_del_init(&cmd->list);
-	spin_unlock_irqrestore(&cmd->device->list_lock, flags);
-
+	scsi_del_cmd_from_list(cmd);
 	BUG_ON(delayed_work_pending(&cmd->abort_work));
 }
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index efa5741cab02..a8d4f17ad5aa 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -583,19 +583,9 @@ static void scsi_mq_free_sgtables(struct scsi_cmnd *cmd)
 
 static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
 {
-	struct scsi_device *sdev = cmd->device;
-	struct Scsi_Host *shost = sdev->host;
-	unsigned long flags;
-
 	scsi_mq_free_sgtables(cmd);
 	scsi_uninit_cmd(cmd);
-
-	if (shost->use_cmd_list) {
-		BUG_ON(list_empty(&cmd->list));
-		spin_lock_irqsave(&sdev->list_lock, flags);
-		list_del_init(&cmd->list);
-		spin_unlock_irqrestore(&sdev->list_lock, flags);
-	}
+	scsi_del_cmd_from_list(cmd);
 }
 
 /*
@@ -1133,6 +1123,35 @@ int scsi_init_io(struct scsi_cmnd *cmd)
 }
 EXPORT_SYMBOL(scsi_init_io);
 
+/* Add a command to the list used by the aacraid and dpt_i2o drivers */
+void scsi_add_cmd_to_list(struct scsi_cmnd *cmd)
+{
+	struct scsi_device *sdev = cmd->device;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
+
+	if (shost->use_cmd_list) {
+		spin_lock_irqsave(&sdev->list_lock, flags);
+		list_add_tail(&cmd->list, &sdev->cmd_list);
+		spin_unlock_irqrestore(&sdev->list_lock, flags);
+	}
+}
+
+/* Remove a command from the list used by the aacraid and dpt_i2o drivers */
+void scsi_del_cmd_from_list(struct scsi_cmnd *cmd)
+{
+	struct scsi_device *sdev = cmd->device;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
+
+	if (shost->use_cmd_list) {
+		spin_lock_irqsave(&sdev->list_lock, flags);
+		BUG_ON(list_empty(&cmd->list));
+		list_del_init(&cmd->list);
+		spin_unlock_irqrestore(&sdev->list_lock, flags);
+	}
+}
+
 /* Called from inside blk_get_request() */
 static void scsi_initialize_rq(struct request *rq)
 {
@@ -1146,7 +1165,6 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 {
 	void *buf = cmd->sense_buffer;
 	void *prot = cmd->prot_sdb;
-	unsigned long flags;
 
 	/* zero out the cmd, except for the embedded scsi_request */
 	memset((char *)cmd + sizeof(cmd->req), 0,
@@ -1158,9 +1176,7 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
 	cmd->jiffies_at_alloc = jiffies;
 
-	spin_lock_irqsave(&dev->list_lock, flags);
-	list_add_tail(&cmd->list, &dev->cmd_list);
-	spin_unlock_irqrestore(&dev->list_lock, flags);
+	scsi_add_cmd_to_list(cmd);
 }
 
 static int scsi_setup_scsi_cmnd(struct scsi_device *sdev, struct request *req)
@@ -1875,11 +1891,7 @@ static int scsi_mq_prep_fn(struct request *req)
 	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
 	cmd->jiffies_at_alloc = jiffies;
 
-	if (shost->use_cmd_list) {
-		spin_lock_irq(&sdev->list_lock);
-		list_add_tail(&cmd->list, &sdev->cmd_list);
-		spin_unlock_irq(&sdev->list_lock);
-	}
+	scsi_add_cmd_to_list(cmd);
 
 	sg = (void *)cmd + sizeof(struct scsi_cmnd) + shost->hostt->cmd_size;
 	cmd->sdb.table.sgl = sg;
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index f86057842f9a..c11c1f9c912c 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -80,6 +80,8 @@ int scsi_eh_get_sense(struct list_head *work_q,
 int scsi_noretry_cmd(struct scsi_cmnd *scmd);
 
 /* scsi_lib.c */
+extern void scsi_add_cmd_to_list(struct scsi_cmnd *cmd);
+extern void scsi_del_cmd_from_list(struct scsi_cmnd *cmd);
 extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
 extern void scsi_device_unbusy(struct scsi_device *sdev);
 extern void scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
-- 
2.12.2

Re: [PATCH 10/18] scsi: Only add commands to the device command list if required by the LLD

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 11/18] scsi: Move most of scsi_init_command() into scsi_initialize_rq()

[Bart Van Assche]

Move the initializations that only have to be performed once and
not every time a request is prepared from scsi_init_command()
into scsi_initialize_rq(). This patch also moves the
jiffies_at_alloc assignment such that it gets back the meaning it
had before commit e9c787e65c0c, namely the value of the jiffies
counter at request allocation time.

Fixes: commit e9c787e65c0c ("scsi: allocate scsi_cmnd structures as part of struct request")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_lib.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a8d4f17ad5aa..68ce053b184b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1156,26 +1156,24 @@ void scsi_del_cmd_from_list(struct scsi_cmnd *cmd)
 static void scsi_initialize_rq(struct request *rq)
 {
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
-
-	scsi_req_init(&cmd->req);
-}
-
-/* Called after a request has been started. */
-void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
-{
+	struct scsi_device *dev = rq->q->queuedata;
 	void *buf = cmd->sense_buffer;
 	void *prot = cmd->prot_sdb;
 
 	/* zero out the cmd, except for the embedded scsi_request */
 	memset((char *)cmd + sizeof(cmd->req), 0,
 		sizeof(*cmd) - sizeof(cmd->req) + dev->host->hostt->cmd_size);
-
+	scsi_req_init(&cmd->req);
 	cmd->device = dev;
 	cmd->sense_buffer = buf;
 	cmd->prot_sdb = prot;
 	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
 	cmd->jiffies_at_alloc = jiffies;
+}
 
+/* Called after a request has been started. */
+void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
+{
 	scsi_add_cmd_to_list(cmd);
 }
 
-- 
2.12.2

Re: [PATCH 11/18] scsi: Move most of scsi_init_command() into scsi_initialize_rq()

[Christoph Hellwig]

On Fri, May 19, 2017 at 11:30:09AM -0700, Bart Van Assche wrote:
> Move the initializations that only have to be performed once and
> not every time a request is prepared from scsi_init_command()
> into scsi_initialize_rq(). This patch also moves the
> jiffies_at_alloc assignment such that it gets back the meaning it
> had before commit e9c787e65c0c, namely the value of the jiffies
> counter at request allocation time.

How does this account for the kmalloced request in scsi_ioctl_reset()?

Re: [PATCH 11/18] scsi: Move most of scsi_init_command() into scsi_initialize_rq()

[Christoph Hellwig]

On Sun, May 21, 2017 at 08:45:23AM +0200, Christoph Hellwig wrote:
> On Fri, May 19, 2017 at 11:30:09AM -0700, Bart Van Assche wrote:
> > Move the initializations that only have to be performed once and
> > not every time a request is prepared from scsi_init_command()
> > into scsi_initialize_rq(). This patch also moves the
> > jiffies_at_alloc assignment such that it gets back the meaning it
> > had before commit e9c787e65c0c, namely the value of the jiffies
> > counter at request allocation time.
> 
> How does this account for the kmalloced request in scsi_ioctl_reset()?

Oh, because that currently manually calls blk_rq_init..

Re: [PATCH 11/18] scsi: Move most of scsi_init_command() into scsi_initialize_rq()

[Christoph Hellwig]

On Fri, May 19, 2017 at 11:30:09AM -0700, Bart Van Assche wrote:
> Move the initializations that only have to be performed once and
> not every time a request is prepared from scsi_init_command()
> into scsi_initialize_rq(). This patch also moves the
> jiffies_at_alloc assignment such that it gets back the meaning it
> had before commit e9c787e65c0c, namely the value of the jiffies
> counter at request allocation time.

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 12/18] scsi: Inline scsi_init_command()

[Bart Van Assche]

The two drivers that use the per-device command list, namely aacraid
and dpt_i2o, expect that that list contains only SCSI commands and
no task management functions. Hence only call scsi_add_cmd_to_list()
from the block layer prep callback functions and not from
scsi_ioctl_reset().

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_error.c | 1 -
 drivers/scsi/scsi_lib.c   | 8 +-------
 drivers/scsi/scsi_priv.h  | 1 -
 3 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index fbbdc345be85..2b7221bc0f32 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2293,7 +2293,6 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
 	blk_rq_init(dev->request_queue, rq);
 
 	scmd = (struct scsi_cmnd *)(rq + 1);
-	scsi_init_command(dev, scmd);
 	scmd->request = rq;
 	scmd->cmnd = scsi_req(rq)->cmd;
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 68ce053b184b..b2e3dc22ecf3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1171,12 +1171,6 @@ static void scsi_initialize_rq(struct request *rq)
 	cmd->jiffies_at_alloc = jiffies;
 }
 
-/* Called after a request has been started. */
-void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
-{
-	scsi_add_cmd_to_list(cmd);
-}
-
 static int scsi_setup_scsi_cmnd(struct scsi_device *sdev, struct request *req)
 {
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
@@ -1347,8 +1341,8 @@ static int scsi_prep_fn(struct request_queue *q, struct request *req)
 			goto out;
 		}
 
-		scsi_init_command(sdev, cmd);
 		req->special = cmd;
+		scsi_add_cmd_to_list(cmd);
 	}
 
 	cmd->tag = req->tag;
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index c11c1f9c912c..67d1550d8b64 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -30,7 +30,6 @@ extern void scsi_exit_hosts(void);
 /* scsi.c */
 extern bool scsi_use_blk_mq;
 int scsi_init_sense_cache(struct Scsi_Host *shost);
-void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd);
 #ifdef CONFIG_SCSI_LOGGING
 void scsi_log_send(struct scsi_cmnd *cmd);
 void scsi_log_completion(struct scsi_cmnd *cmd, int disposition);
-- 
2.12.2

Re: [PATCH 12/18] scsi: Inline scsi_init_command()

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 13/18] scsi: Move sense buffer pointer initialization into scsi_initialize_rq()

[Bart Van Assche]

This patch is a preparation for the next patch that will zero
the struct scsi_request embedded in struct scsi_cmnd before
calling scsi_req_init().

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_lib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b2e3dc22ecf3..eeb668935836 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1164,6 +1164,7 @@ static void scsi_initialize_rq(struct request *rq)
 	memset((char *)cmd + sizeof(cmd->req), 0,
 		sizeof(*cmd) - sizeof(cmd->req) + dev->host->hostt->cmd_size);
 	scsi_req_init(&cmd->req);
+	cmd->req.sense = cmd->sense_buffer;
 	cmd->device = dev;
 	cmd->sense_buffer = buf;
 	cmd->prot_sdb = prot;
@@ -2022,7 +2023,6 @@ static int scsi_init_request(struct blk_mq_tag_set *set, struct request *rq,
 		scsi_alloc_sense_buffer(shost, GFP_KERNEL, numa_node);
 	if (!cmd->sense_buffer)
 		return -ENOMEM;
-	cmd->req.sense = cmd->sense_buffer;
 	return 0;
 }
 
@@ -2114,7 +2114,6 @@ static int scsi_init_rq(struct request_queue *q, struct request *rq, gfp_t gfp)
 	cmd->sense_buffer = scsi_alloc_sense_buffer(shost, gfp, NUMA_NO_NODE);
 	if (!cmd->sense_buffer)
 		goto fail;
-	cmd->req.sense = cmd->sense_buffer;
 
 	if (scsi_host_get_prot(shost) >= SHOST_DIX_TYPE0_PROTECTION) {
 		cmd->prot_sdb = kmem_cache_zalloc(scsi_sdb_cache, gfp);
-- 
2.12.2

Re: [PATCH 13/18] scsi: Move sense buffer pointer initialization into scsi_initialize_rq()

[Christoph Hellwig]

On Fri, May 19, 2017 at 11:30:11AM -0700, Bart Van Assche wrote:
> This patch is a preparation for the next patch that will zero
> the struct scsi_request embedded in struct scsi_cmnd before
> calling scsi_req_init().

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 14/18] scsi: Make scsi_initialize_rq() zero the entire struct scsi_cmnd

[Bart Van Assche]

This simplifies the memset() call in scsi_initialize_rq() and avoids
that any stale data is left behind in struct scsi_request.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_lib.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eeb668935836..791bae192bfb 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1160,11 +1160,9 @@ static void scsi_initialize_rq(struct request *rq)
 	void *buf = cmd->sense_buffer;
 	void *prot = cmd->prot_sdb;
 
-	/* zero out the cmd, except for the embedded scsi_request */
-	memset((char *)cmd + sizeof(cmd->req), 0,
-		sizeof(*cmd) - sizeof(cmd->req) + dev->host->hostt->cmd_size);
+	memset(cmd, 0, blk_queue_cmd_size(rq->q));
 	scsi_req_init(&cmd->req);
-	cmd->req.sense = cmd->sense_buffer;
+	cmd->req.sense = buf;
 	cmd->device = dev;
 	cmd->sense_buffer = buf;
 	cmd->prot_sdb = prot;
-- 
2.12.2

Re: [PATCH 14/18] scsi: Make scsi_initialize_rq() zero the entire struct scsi_cmnd

[Christoph Hellwig]

On Fri, May 19, 2017 at 11:30:12AM -0700, Bart Van Assche wrote:
> This simplifies the memset() call in scsi_initialize_rq() and avoids
> that any stale data is left behind in struct scsi_request.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi_lib.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index eeb668935836..791bae192bfb 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1160,11 +1160,9 @@ static void scsi_initialize_rq(struct request *rq)
>  	void *buf = cmd->sense_buffer;
>  	void *prot = cmd->prot_sdb;
>  
> -	/* zero out the cmd, except for the embedded scsi_request */
> -	memset((char *)cmd + sizeof(cmd->req), 0,
> -		sizeof(*cmd) - sizeof(cmd->req) + dev->host->hostt->cmd_size);
> +	memset(cmd, 0, blk_queue_cmd_size(rq->q));
>  	scsi_req_init(&cmd->req);
> -	cmd->req.sense = cmd->sense_buffer;
> +	cmd->req.sense = buf;
>  	cmd->device = dev;
>  	cmd->sense_buffer = buf;

maybe move the two sense buffer initializations together?

Otherwise this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 14/18] scsi: Make scsi_initialize_rq() zero the entire struct scsi_cmnd

[Bart Van Assche]

On Sun, 2017-05-21 at 08:49 +0200, Christoph Hellwig wrote:
> On Fri, May 19, 2017 at 11:30:12AM -0700, Bart Van Assche wrote:
> > This simplifies the memset() call in scsi_initialize_rq() and avoids
> > that any stale data is left behind in struct scsi_request.
> > 
> > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.com>
> > ---
> >  drivers/scsi/scsi_lib.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index eeb668935836..791bae192bfb 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1160,11 +1160,9 @@ static void scsi_initialize_rq(struct request *rq)
> >  	void *buf = cmd->sense_buffer;
> >  	void *prot = cmd->prot_sdb;
> >  
> > -	/* zero out the cmd, except for the embedded scsi_request */
> > -	memset((char *)cmd + sizeof(cmd->req), 0,
> > -		sizeof(*cmd) - sizeof(cmd->req) + dev->host->hostt->cmd_size);
> > +	memset(cmd, 0, blk_queue_cmd_size(rq->q));
> >  	scsi_req_init(&cmd->req);
> > -	cmd->req.sense = cmd->sense_buffer;
> > +	cmd->req.sense = buf;
> >  	cmd->device = dev;
> >  	cmd->sense_buffer = buf;
> 
> maybe move the two sense buffer initializations together?

Hello Christoph,

That sounds like a good idea to me. I will make that change.

Bart.

[PATCH 15/18] scsi: storvsc: Initialize driver-private command before using it

[Bart Van Assche]

The storvsc driver is the only SCSI LLD that uses driver-private
command data and that does not zero-initialize that data before
reading it. Make this driver consistent with the other SCSI LLDs
that use driver-private command data.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Long Li <longli@microsoft.com>
Cc: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/scsi/storvsc_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 8d955db6424f..cc08593c5218 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1550,6 +1550,7 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd)
 		}
 	}
 
+	memset(cmd_request, 0, sizeof(*cmd_request));
 	/* Setup the cmd request */
 	cmd_request->cmd = scmnd;
 
-- 
2.12.2

Re: [PATCH 15/18] scsi: storvsc: Initialize driver-private command before using it

[Christoph Hellwig]

On Fri, May 19, 2017 at 11:30:13AM -0700, Bart Van Assche wrote:
> The storvsc driver is the only SCSI LLD that uses driver-private
> command data and that does not zero-initialize that data before
> reading it. Make this driver consistent with the other SCSI LLDs
> that use driver-private command data.

Well.  Either we add zeroing to storvsc and remove it from common
code, or we remove the zeroing from the drivers.

We shouldn't do both.  Given that we already zero the remaining
command it seems to me like keeping the zeroing in common code
would be preferred, but I'm open to discussion.

Re: [PATCH 15/18] scsi: storvsc: Initialize driver-private command before using it

[Bart Van Assche]

On Sun, 2017-05-21 at 08:51 +0200, Christoph Hellwig wrote:
> On Fri, May 19, 2017 at 11:30:13AM -0700, Bart Van Assche wrote:
> > The storvsc driver is the only SCSI LLD that uses driver-private
> > command data and that does not zero-initialize that data before
> > reading it. Make this driver consistent with the other SCSI LLDs
> > that use driver-private command data.
> 
> Well.  Either we add zeroing to storvsc and remove it from common
> code, or we remove the zeroing from the drivers.
> 
> We shouldn't do both.  Given that we already zero the remaining
> command it seems to me like keeping the zeroing in common code
> would be preferred, but I'm open to discussion.

Hello Christoph,

I will remove the code that zeroes driver-private command data from the other
SCSI LLDs and add it to the scsi-sq request preparation code path.

Bart.

[PATCH 16/18] scsi-mq: Make behavior scsi_mq_prep_fn() closer to that of scsi_prep_fn()

[Bart Van Assche]

Instead of clearing most of struct scsi_cmnd and reinitializing
it, rely on scsi_initialize_rq() for initialization of struct
scsi_cmnd. This patch fixes a bug, namely that it avoids that
jiffies_at_alloc gets overwritten if a request is requeued.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_lib.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 791bae192bfb..e0c4481cb943 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1861,27 +1861,15 @@ static int scsi_mq_prep_fn(struct request *req)
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
 	struct scsi_device *sdev = req->q->queuedata;
 	struct Scsi_Host *shost = sdev->host;
-	unsigned char *sense_buf = cmd->sense_buffer;
 	struct scatterlist *sg;
 
-	/* zero out the cmd, except for the embedded scsi_request */
-	memset((char *)cmd + sizeof(cmd->req), 0,
-		sizeof(*cmd) - sizeof(cmd->req) + shost->hostt->cmd_size);
-
 	req->special = cmd;
 
 	cmd->request = req;
-	cmd->device = sdev;
-	cmd->sense_buffer = sense_buf;
 
 	cmd->tag = req->tag;
-
 	cmd->prot_op = SCSI_PROT_NORMAL;
 
-	INIT_LIST_HEAD(&cmd->list);
-	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
-	cmd->jiffies_at_alloc = jiffies;
-
 	scsi_add_cmd_to_list(cmd);
 
 	sg = (void *)cmd + sizeof(struct scsi_cmnd) + shost->hostt->cmd_size;
-- 
2.12.2

Re: [PATCH 16/18] scsi-mq: Make behavior scsi_mq_prep_fn() closer to that of scsi_prep_fn()

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 17/18] scsi: Consolidate more initialization code

[Bart Van Assche]

Initialize struct scsi_cmnd.request from inside scsi_initialize_rq()
instead of every time a request is prepared. Note: moving the tag
initialization into scsi_initialize_rq() is not possible because
the single-queue block layer only assigns a tag to a request after
a request has been started.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_error.c | 1 -
 drivers/scsi/scsi_lib.c   | 4 +---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 2b7221bc0f32..7c2aba4b167a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2293,7 +2293,6 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
 	blk_rq_init(dev->request_queue, rq);
 
 	scmd = (struct scsi_cmnd *)(rq + 1);
-	scmd->request = rq;
 	scmd->cmnd = scsi_req(rq)->cmd;
 
 	scmd->scsi_done		= scsi_reset_provider_done_command;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e0c4481cb943..5431d114c897 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1166,6 +1166,7 @@ static void scsi_initialize_rq(struct request *rq)
 	cmd->device = dev;
 	cmd->sense_buffer = buf;
 	cmd->prot_sdb = prot;
+	cmd->request = rq;
 	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
 	cmd->jiffies_at_alloc = jiffies;
 }
@@ -1345,7 +1346,6 @@ static int scsi_prep_fn(struct request_queue *q, struct request *req)
 	}
 
 	cmd->tag = req->tag;
-	cmd->request = req;
 	cmd->prot_op = SCSI_PROT_NORMAL;
 
 	ret = scsi_setup_cmnd(sdev, req);
@@ -1865,8 +1865,6 @@ static int scsi_mq_prep_fn(struct request *req)
 
 	req->special = cmd;
 
-	cmd->request = req;
-
 	cmd->tag = req->tag;
 	cmd->prot_op = SCSI_PROT_NORMAL;
 
-- 
2.12.2

Re: [PATCH 17/18] scsi: Consolidate more initialization code

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 18/18] scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it

[Bart Van Assche]

The only functional change is that this patch causes scsi_setup_fs_cmnd()
to clear scsi_request.sense_len.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5431d114c897..a93c7d5a9322 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1212,8 +1212,8 @@ static int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 			return ret;
 	}
 
-	cmd->cmnd = scsi_req(req)->cmd = scsi_req(req)->__cmd;
-	memset(cmd->cmnd, 0, BLK_MAX_CDB);
+	scsi_req_init(&cmd->req);
+	cmd->cmnd = scsi_req(req)->cmd;
 	return scsi_cmd_to_driver(cmd)->init_command(cmd);
 }
 
-- 
2.12.2

Re: [PATCH 18/18] scsi_setup_fs_cmnd(): Call scsi_req_init() instead of open-coding it

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>