[PATCH 0/2] block,scsi: fixup blk_get_request dead queue scenarios

[PATCH 0/2] block,scsi: fixup blk_get_request dead queue scenarios

[Joe Lawrence]

Hello Jens,

This bug was originally reported against 3.10 and still exists in
3.15-rc5 [1] [2].  

These changes were tested on-top of 3.15-rc5 with user-program that
opens a CD device, its media is removed, and then the program issues a
CDROMEJECT ioctl.  Without this change, the kernel can crash in
sg_scsi_ioctl on NULL request pointer.

The first patch adds return checking to a few blk_get_request callers.
The second patch is much larger, modifying the return value to include
an ERR_PTR to indicate failure reason.  I didn't touch any of the IDE
callers save one since all but that one assume success.  As such, the
first can be merged without the second if the change is considered too
dangerous.

Feel free to drop any changes to files (like paride/pd.c) if they're
considered deprecated.

[1] http://thread.gmane.org/gmane.linux.scsi/80934
[2] http://thread.gmane.org/gmane.linux.kernel/1502882

Joe Lawrence (2):
  block,scsi: verify return pointer from blk_get_request
  block,scsi: convert and handle ERR_PTR from blk_get_request

 block/blk-core.c                            | 34 ++++++++++++++---------------
 block/bsg.c                                 |  8 +++----
 block/scsi_ioctl.c                          | 13 ++++++++---
 drivers/block/paride/pd.c                   |  2 ++
 drivers/block/pktcdvd.c                     |  2 ++
 drivers/block/sx8.c                         |  2 +-
 drivers/cdrom/cdrom.c                       |  4 ++--
 drivers/ide/ide-park.c                      |  2 +-
 drivers/scsi/device_handler/scsi_dh_alua.c  |  2 +-
 drivers/scsi/device_handler/scsi_dh_emc.c   |  2 +-
 drivers/scsi/device_handler/scsi_dh_hp_sw.c |  4 ++--
 drivers/scsi/device_handler/scsi_dh_rdac.c  |  2 +-
 drivers/scsi/osd/osd_initiator.c            |  4 ++--
 drivers/scsi/osst.c                         |  2 +-
 drivers/scsi/scsi_error.c                   |  2 ++
 drivers/scsi/scsi_lib.c                     |  2 +-
 drivers/scsi/scsi_tgt_lib.c                 |  2 +-
 drivers/scsi/sg.c                           |  4 ++--
 drivers/scsi/st.c                           |  2 +-
 drivers/target/target_core_pscsi.c          |  2 +-
 20 files changed, 55 insertions(+), 42 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] block,scsi: verify return pointer from blk_get_request

[Joe Lawrence]

The blk-core dead queue checks introduce an error scenario to
blk_get_request that returns NULL if the request queue has been
shutdown. This affects the behavior for __GFP_WAIT callers, who should
verify the return value before dereferencing.

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Acked-by: Jiri Kosina <jkosina@suse.cz> [for pktdvd]
---
 block/scsi_ioctl.c        | 9 ++++++++-
 drivers/block/paride/pd.c | 2 ++
 drivers/block/pktcdvd.c   | 2 ++
 drivers/scsi/scsi_error.c | 2 ++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 2648797..e6485c9 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -442,6 +442,10 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
 	}
 
 	rq = blk_get_request(q, in_len ? WRITE : READ, __GFP_WAIT);
+	if (!rq) {
+		err = -ENODEV;
+		goto error_free_buffer;
+	}
 
 	cmdlen = COMMAND_SIZE(opcode);
 
@@ -514,8 +518,9 @@ out:
 	}
 	
 error:
-	kfree(buffer);
 	blk_put_request(rq);
+error_free_buffer:
+	kfree(buffer);
 	return err;
 }
 EXPORT_SYMBOL_GPL(sg_scsi_ioctl);
@@ -528,6 +533,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	int err;
 
 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
+	if (!rq)
+		return -ENODEV;
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	rq->cmd[0] = cmd;
diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index 19ad8f0..856178a 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -722,6 +722,8 @@ static int pd_special_command(struct pd_unit *disk,
 	int err = 0;
 
 	rq = blk_get_request(disk->gd->queue, READ, __GFP_WAIT);
+	if (!rq)
+		return -ENODEV;
 
 	rq->cmd_type = REQ_TYPE_SPECIAL;
 	rq->special = func;
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index a2af73d..ff39837 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -704,6 +704,8 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
 
 	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
 			     WRITE : READ, __GFP_WAIT);
+	if (!rq)
+		return -ENODEV;
 
 	if (cgc->buflen) {
 		ret = blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen,
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index f17aa7a..d50531f 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1950,6 +1950,8 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
 	 * request becomes available
 	 */
 	req = blk_get_request(sdev->request_queue, READ, GFP_KERNEL);
+	if (!req)
+		return;
 
 	req->cmd[0] = ALLOW_MEDIUM_REMOVAL;
 	req->cmd[1] = 0;
-- 
1.8.3.1

Re: [PATCH 1/2] block,scsi: verify return pointer from blk_get_request

[Jeff Moyer]

Joe Lawrence <joe.lawrence@stratus.com> writes:

> The blk-core dead queue checks introduce an error scenario to
> blk_get_request that returns NULL if the request queue has been
> shutdown. This affects the behavior for __GFP_WAIT callers, who should
> verify the return value before dereferencing.
>
> Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> Acked-by: Jiri Kosina <jkosina@suse.cz> [for pktdvd]

Acked-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>  block/scsi_ioctl.c        | 9 ++++++++-
>  drivers/block/paride/pd.c | 2 ++
>  drivers/block/pktcdvd.c   | 2 ++
>  drivers/scsi/scsi_error.c | 2 ++
>  4 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index 2648797..e6485c9 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -442,6 +442,10 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
>  	}
>  
>  	rq = blk_get_request(q, in_len ? WRITE : READ, __GFP_WAIT);
> +	if (!rq) {
> +		err = -ENODEV;
> +		goto error_free_buffer;
> +	}
>  
>  	cmdlen = COMMAND_SIZE(opcode);
>  
> @@ -514,8 +518,9 @@ out:
>  	}
>  	
>  error:
> -	kfree(buffer);
>  	blk_put_request(rq);
> +error_free_buffer:
> +	kfree(buffer);
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(sg_scsi_ioctl);
> @@ -528,6 +533,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
>  	int err;
>  
>  	rq = blk_get_request(q, WRITE, __GFP_WAIT);
> +	if (!rq)
> +		return -ENODEV;
>  	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>  	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
>  	rq->cmd[0] = cmd;
> diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
> index 19ad8f0..856178a 100644
> --- a/drivers/block/paride/pd.c
> +++ b/drivers/block/paride/pd.c
> @@ -722,6 +722,8 @@ static int pd_special_command(struct pd_unit *disk,
>  	int err = 0;
>  
>  	rq = blk_get_request(disk->gd->queue, READ, __GFP_WAIT);
> +	if (!rq)
> +		return -ENODEV;
>  
>  	rq->cmd_type = REQ_TYPE_SPECIAL;
>  	rq->special = func;
> diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> index a2af73d..ff39837 100644
> --- a/drivers/block/pktcdvd.c
> +++ b/drivers/block/pktcdvd.c
> @@ -704,6 +704,8 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
>  
>  	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
>  			     WRITE : READ, __GFP_WAIT);
> +	if (!rq)
> +		return -ENODEV;
>  
>  	if (cgc->buflen) {
>  		ret = blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen,
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index f17aa7a..d50531f 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1950,6 +1950,8 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
>  	 * request becomes available
>  	 */
>  	req = blk_get_request(sdev->request_queue, READ, GFP_KERNEL);
> +	if (!req)
> +		return;
>  
>  	req->cmd[0] = ALLOW_MEDIUM_REMOVAL;
>  	req->cmd[1] = 0;

[PATCH 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

[Joe Lawrence]

The blk_get_request function may fail in low-memory conditions or during
device removal (even if __GFP_WAIT is set). To distinguish between these
errors, modify the blk_get_request call stack to return the appropriate
ERR_PTR. Verify that all callers check the return status and consider
IS_ERR instead of a simple NULL pointer check.

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
Acked-by: Jiri Kosina <jkosina@suse.cz> [for pktdvd]
Acked-by: Boaz Harrosh <bharrosh@panasas.com> [for osd]
---
 block/blk-core.c                            | 34 ++++++++++++++---------------
 block/bsg.c                                 |  8 +++----
 block/scsi_ioctl.c                          | 12 +++++-----
 drivers/block/paride/pd.c                   |  4 ++--
 drivers/block/pktcdvd.c                     |  4 ++--
 drivers/block/sx8.c                         |  2 +-
 drivers/cdrom/cdrom.c                       |  4 ++--
 drivers/ide/ide-park.c                      |  2 +-
 drivers/scsi/device_handler/scsi_dh_alua.c  |  2 +-
 drivers/scsi/device_handler/scsi_dh_emc.c   |  2 +-
 drivers/scsi/device_handler/scsi_dh_hp_sw.c |  4 ++--
 drivers/scsi/device_handler/scsi_dh_rdac.c  |  2 +-
 drivers/scsi/osd/osd_initiator.c            |  4 ++--
 drivers/scsi/osst.c                         |  2 +-
 drivers/scsi/scsi_error.c                   |  2 +-
 drivers/scsi/scsi_lib.c                     |  2 +-
 drivers/scsi/scsi_tgt_lib.c                 |  2 +-
 drivers/scsi/sg.c                           |  4 ++--
 drivers/scsi/st.c                           |  2 +-
 drivers/target/target_core_pscsi.c          |  2 +-
 20 files changed, 50 insertions(+), 50 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index a0e3096..cd0e9f3 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -891,9 +891,9 @@ static struct io_context *rq_ioc(struct bio *bio)
  * Get a free request from @q.  This function may fail under memory
  * pressure or if @q is dead.
  *
- * Must be callled with @q->queue_lock held and,
- * Returns %NULL on failure, with @q->queue_lock held.
- * Returns !%NULL on success, with @q->queue_lock *not held*.
+ * Must be called with @q->queue_lock held and,
+ * Returns ERR_PTR on failure, with @q->queue_lock held.
+ * Returns request pointer on success, with @q->queue_lock *not held*.
  */
 static struct request *__get_request(struct request_list *rl, int rw_flags,
 				     struct bio *bio, gfp_t gfp_mask)
@@ -907,7 +907,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
 	int may_queue;
 
 	if (unlikely(blk_queue_dying(q)))
-		return NULL;
+		return ERR_PTR(-ENODEV);
 
 	may_queue = elv_may_queue(q, rw_flags);
 	if (may_queue == ELV_MQUEUE_NO)
@@ -932,7 +932,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
 					 * process is not a "batcher", and not
 					 * exempted by the IO scheduler
 					 */
-					return NULL;
+					return ERR_PTR(-ENOMEM);
 				}
 			}
 		}
@@ -950,7 +950,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
 	 * allocated with any setting of ->nr_requests
 	 */
 	if (rl->count[is_sync] >= (3 * q->nr_requests / 2))
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	q->nr_rqs[is_sync]++;
 	rl->count[is_sync]++;
@@ -1055,7 +1055,7 @@ fail_alloc:
 rq_starved:
 	if (unlikely(rl->count[is_sync] == 0))
 		rl->starved[is_sync] = 1;
-	return NULL;
+	return ERR_PTR(-ENOMEM);
 }
 
 /**
@@ -1068,9 +1068,9 @@ rq_starved:
  * Get a free request from @q.  If %__GFP_WAIT is set in @gfp_mask, this
  * function keeps retrying under memory pressure and fails iff @q is dead.
  *
- * Must be callled with @q->queue_lock held and,
- * Returns %NULL on failure, with @q->queue_lock held.
- * Returns !%NULL on success, with @q->queue_lock *not held*.
+ * Must be called with @q->queue_lock held and,
+ * Returns ERR_PTR on failure, with @q->queue_lock held.
+ * Returns request pointer on success, with @q->queue_lock *not held*.
  */
 static struct request *get_request(struct request_queue *q, int rw_flags,
 				   struct bio *bio, gfp_t gfp_mask)
@@ -1083,12 +1083,12 @@ static struct request *get_request(struct request_queue *q, int rw_flags,
 	rl = blk_get_rl(q, bio);	/* transferred to @rq on success */
 retry:
 	rq = __get_request(rl, rw_flags, bio, gfp_mask);
-	if (rq)
+	if (!IS_ERR(rq))
 		return rq;
 
 	if (!(gfp_mask & __GFP_WAIT) || unlikely(blk_queue_dying(q))) {
 		blk_put_rl(rl);
-		return NULL;
+		return rq;
 	}
 
 	/* wait on @rl and retry */
@@ -1125,7 +1125,7 @@ static struct request *blk_old_get_request(struct request_queue *q, int rw,
 
 	spin_lock_irq(q->queue_lock);
 	rq = get_request(q, rw, NULL, gfp_mask);
-	if (!rq)
+	if (IS_ERR(rq))
 		spin_unlock_irq(q->queue_lock);
 	/* q->queue_lock is unlocked at this point */
 
@@ -1177,8 +1177,8 @@ struct request *blk_make_request(struct request_queue *q, struct bio *bio,
 {
 	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
 
-	if (unlikely(!rq))
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(rq))
+		return rq;
 
 	for_each_bio(bio) {
 		struct bio *bounce_bio = bio;
@@ -1559,8 +1559,8 @@ get_rq:
 	 * Returns with the queue unlocked.
 	 */
 	req = get_request(q, rw_flags, bio, GFP_NOIO);
-	if (unlikely(!req)) {
-		bio_endio(bio, -ENODEV);	/* @q is dead */
+	if (IS_ERR(req)) {
+		bio_endio(bio, PTR_ERR(req));	/* @q is dead */
 		goto out_unlock;
 	}
 
diff --git a/block/bsg.c b/block/bsg.c
index 420a5a9..2b1c322 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -271,8 +271,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 	 * map scatter-gather elements separately and string them to request
 	 */
 	rq = blk_get_request(q, rw, GFP_KERNEL);
-	if (!rq)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(rq))
+		return rq;
 	ret = blk_fill_sgv4_hdr_rq(q, rq, hdr, bd, has_write_perm);
 	if (ret)
 		goto out;
@@ -284,8 +284,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 		}
 
 		next_rq = blk_get_request(q, READ, GFP_KERNEL);
-		if (!next_rq) {
-			ret = -ENOMEM;
+		if (IS_ERR(next_rq)) {
+			ret = PTR_ERR(rq);
 			goto out;
 		}
 		rq->next_rq = next_rq;
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index e6485c9..f901fb5 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -313,8 +313,8 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
 		}
 
 	rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);
-	if (!rq)
-		return -ENOMEM;
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
 
 	if (blk_fill_sghdr_rq(q, rq, hdr, mode)) {
 		blk_put_request(rq);
@@ -442,8 +442,8 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
 	}
 
 	rq = blk_get_request(q, in_len ? WRITE : READ, __GFP_WAIT);
-	if (!rq) {
-		err = -ENODEV;
+	if (IS_ERR(rq)) {
+		err = PTR_ERR(rq);
 		goto error_free_buffer;
 	}
 
@@ -533,8 +533,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	int err;
 
 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
-	if (!rq)
-		return -ENODEV;
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	rq->cmd[0] = cmd;
diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index 856178a..2af4101 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -722,8 +722,8 @@ static int pd_special_command(struct pd_unit *disk,
 	int err = 0;
 
 	rq = blk_get_request(disk->gd->queue, READ, __GFP_WAIT);
-	if (!rq)
-		return -ENODEV;
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
 
 	rq->cmd_type = REQ_TYPE_SPECIAL;
 	rq->special = func;
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index ff39837..1de6e90 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -704,8 +704,8 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
 
 	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
 			     WRITE : READ, __GFP_WAIT);
-	if (!rq)
-		return -ENODEV;
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
 
 	if (cgc->buflen) {
 		ret = blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen,
diff --git a/drivers/block/sx8.c b/drivers/block/sx8.c
index d5e2d12..5d55285 100644
--- a/drivers/block/sx8.c
+++ b/drivers/block/sx8.c
@@ -568,7 +568,7 @@ static struct carm_request *carm_get_special(struct carm_host *host)
 		return NULL;
 
 	rq = blk_get_request(host->oob_q, WRITE /* bogus */, GFP_KERNEL);
-	if (!rq) {
+	if (IS_ERR(rq)) {
 		spin_lock_irqsave(&host->lock, flags);
 		carm_put_request(host, crq);
 		spin_unlock_irqrestore(&host->lock, flags);
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 8a3aff7..85e63d3 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2161,8 +2161,8 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
 		len = nr * CD_FRAMESIZE_RAW;
 
 		rq = blk_get_request(q, READ, GFP_KERNEL);
-		if (!rq) {
-			ret = -ENOMEM;
+		if (IS_ERR(rq)) {
+			ret = PTR_ERR(rq);
 			break;
 		}
 
diff --git a/drivers/ide/ide-park.c b/drivers/ide/ide-park.c
index f41558a..ca95860 100644
--- a/drivers/ide/ide-park.c
+++ b/drivers/ide/ide-park.c
@@ -46,7 +46,7 @@ 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, READ, GFP_NOWAIT);
-	if (unlikely(!rq))
+	if (IS_ERR(rq))
 		goto out;
 
 	rq->cmd[0] = REQ_UNPARK_HEADS;
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index 5248c88..5aefcf9 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -115,7 +115,7 @@ static struct request *get_alua_req(struct scsi_device *sdev,
 
 	rq = blk_get_request(q, rw, GFP_NOIO);
 
-	if (!rq) {
+	if (IS_ERR(rq)) {
 		sdev_printk(KERN_INFO, sdev,
 			    "%s: blk_get_request failed\n", __func__);
 		return NULL;
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index e1c8be0..2ec3261 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -275,7 +275,7 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 
 	rq = blk_get_request(sdev->request_queue,
 			(cmd != INQUIRY) ? WRITE : READ, GFP_NOIO);
-	if (!rq) {
+	if (IS_ERR(rq)) {
 		sdev_printk(KERN_INFO, sdev, "get_req: blk_get_request failed");
 		return NULL;
 	}
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 084062b..1cf4019 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -117,7 +117,7 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
 
 retry:
 	req = blk_get_request(sdev->request_queue, WRITE, GFP_NOIO);
-	if (!req)
+	if (IS_ERR(req))
 		return SCSI_DH_RES_TEMP_UNAVAIL;
 
 	req->cmd_type = REQ_TYPE_BLOCK_PC;
@@ -247,7 +247,7 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h)
 	struct request *req;
 
 	req = blk_get_request(h->sdev->request_queue, WRITE, GFP_ATOMIC);
-	if (!req)
+	if (IS_ERR(req))
 		return SCSI_DH_RES_TEMP_UNAVAIL;
 
 	req->cmd_type = REQ_TYPE_BLOCK_PC;
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 4b9cf93..5f6e5d8 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -274,7 +274,7 @@ static struct request *get_rdac_req(struct scsi_device *sdev,
 
 	rq = blk_get_request(q, rw, GFP_NOIO);
 
-	if (!rq) {
+	if (IS_ERR(rq)) {
 		sdev_printk(KERN_INFO, sdev,
 				"get_rdac_req: blk_get_request failed.\n");
 		return NULL;
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index bac04c2..831f1f9 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1567,8 +1567,8 @@ static struct request *_make_request(struct request_queue *q, bool has_write,
 		struct request *req;
 
 		req = blk_get_request(q, has_write ? WRITE : READ, flags);
-		if (unlikely(!req))
-			return ERR_PTR(-ENOMEM);
+		if (IS_ERR(req))
+			return req;
 
 		return req;
 	}
diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
index 21883a2..826cda9 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -362,7 +362,7 @@ static int osst_execute(struct osst_request *SRpnt, const unsigned char *cmd,
 	int write = (data_direction == DMA_TO_DEVICE);
 
 	req = blk_get_request(SRpnt->stp->device->request_queue, write, GFP_KERNEL);
-	if (!req)
+	if (IS_ERR(req))
 		return DRIVER_ERROR << 24;
 
 	req->cmd_type = REQ_TYPE_BLOCK_PC;
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index d50531f..ca038ee 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1950,7 +1950,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
 	 * request becomes available
 	 */
 	req = blk_get_request(sdev->request_queue, READ, GFP_KERNEL);
-	if (!req)
+	if (IS_ERR(req))
 		return;
 
 	req->cmd[0] = ALLOW_MEDIUM_REMOVAL;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9db097a..b25fab0b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -193,7 +193,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	int ret = DRIVER_ERROR << 24;
 
 	req = blk_get_request(sdev->request_queue, write, __GFP_WAIT);
-	if (!req)
+	if (IS_ERR(req))
 		return ret;
 
 	if (bufflen &&	blk_rq_map_kern(sdev->request_queue, req,
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index e51add0..b14c15b 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -97,7 +97,7 @@ struct scsi_cmnd *scsi_host_get_command(struct Scsi_Host *shost,
 	 * we are in target mode we want the opposite.
 	 */
 	rq = blk_get_request(shost->uspace_req_q, !write, gfp_mask);
-	if (!rq)
+	if (IS_ERR(rq))
 		goto free_tcmd;
 
 	cmd = __scsi_get_command(shost, gfp_mask);
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index df5e961..2e54684 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1650,8 +1650,8 @@ static int sg_start_req(Sg_request *srp, unsigned char *cmd)
 				   dxfer_len));
 
 	rq = blk_get_request(q, rw, GFP_ATOMIC);
-	if (!rq)
-		return -ENOMEM;
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
 
 	memcpy(rq->cmd, cmd, hp->cmd_len);
 
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index afc834e..48a6cef 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -481,7 +481,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd,
 
 	req = blk_get_request(SRpnt->stp->device->request_queue, write,
 			      GFP_KERNEL);
-	if (!req)
+	if (IS_ERR(req))
 		return DRIVER_ERROR << 24;
 
 	req->cmd_type = REQ_TYPE_BLOCK_PC;
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 0f199f6..23d340d 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -1050,7 +1050,7 @@ pscsi_execute_cmd(struct se_cmd *cmd)
 		req = blk_get_request(pdv->pdv_sd->request_queue,
 				(data_direction == DMA_TO_DEVICE),
 				GFP_KERNEL);
-		if (!req) {
+		if (IS_ERR(req)) {
 			pr_err("PSCSI: blk_get_request() failed\n");
 			ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 			goto fail;
-- 
1.8.3.1

Re: [PATCH 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

[Jeff Moyer]

Joe Lawrence <joe.lawrence@stratus.com> writes:

> The blk_get_request function may fail in low-memory conditions or during
> device removal (even if __GFP_WAIT is set). To distinguish between these
> errors, modify the blk_get_request call stack to return the appropriate
> ERR_PTR. Verify that all callers check the return status and consider
> IS_ERR instead of a simple NULL pointer check.

I'm curious to know what testing you did, and what code paths do
anything different for ENOMEM vs EIO.  I guess ENOMEM may make it all
the way back to userspace?

-Jeff

>
> Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
> Acked-by: Jiri Kosina <jkosina@suse.cz> [for pktdvd]
> Acked-by: Boaz Harrosh <bharrosh@panasas.com> [for osd]
> ---
>  block/blk-core.c                            | 34 ++++++++++++++---------------
>  block/bsg.c                                 |  8 +++----
>  block/scsi_ioctl.c                          | 12 +++++-----
>  drivers/block/paride/pd.c                   |  4 ++--
>  drivers/block/pktcdvd.c                     |  4 ++--
>  drivers/block/sx8.c                         |  2 +-
>  drivers/cdrom/cdrom.c                       |  4 ++--
>  drivers/ide/ide-park.c                      |  2 +-
>  drivers/scsi/device_handler/scsi_dh_alua.c  |  2 +-
>  drivers/scsi/device_handler/scsi_dh_emc.c   |  2 +-
>  drivers/scsi/device_handler/scsi_dh_hp_sw.c |  4 ++--
>  drivers/scsi/device_handler/scsi_dh_rdac.c  |  2 +-
>  drivers/scsi/osd/osd_initiator.c            |  4 ++--
>  drivers/scsi/osst.c                         |  2 +-
>  drivers/scsi/scsi_error.c                   |  2 +-
>  drivers/scsi/scsi_lib.c                     |  2 +-
>  drivers/scsi/scsi_tgt_lib.c                 |  2 +-
>  drivers/scsi/sg.c                           |  4 ++--
>  drivers/scsi/st.c                           |  2 +-
>  drivers/target/target_core_pscsi.c          |  2 +-
>  20 files changed, 50 insertions(+), 50 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index a0e3096..cd0e9f3 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -891,9 +891,9 @@ static struct io_context *rq_ioc(struct bio *bio)
>   * Get a free request from @q.  This function may fail under memory
>   * pressure or if @q is dead.
>   *
> - * Must be callled with @q->queue_lock held and,
> - * Returns %NULL on failure, with @q->queue_lock held.
> - * Returns !%NULL on success, with @q->queue_lock *not held*.
> + * Must be called with @q->queue_lock held and,
> + * Returns ERR_PTR on failure, with @q->queue_lock held.
> + * Returns request pointer on success, with @q->queue_lock *not held*.
>   */
>  static struct request *__get_request(struct request_list *rl, int rw_flags,
>  				     struct bio *bio, gfp_t gfp_mask)
> @@ -907,7 +907,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
>  	int may_queue;
>  
>  	if (unlikely(blk_queue_dying(q)))
> -		return NULL;
> +		return ERR_PTR(-ENODEV);
>  
>  	may_queue = elv_may_queue(q, rw_flags);
>  	if (may_queue == ELV_MQUEUE_NO)
> @@ -932,7 +932,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
>  					 * process is not a "batcher", and not
>  					 * exempted by the IO scheduler
>  					 */
> -					return NULL;
> +					return ERR_PTR(-ENOMEM);
>  				}
>  			}
>  		}
> @@ -950,7 +950,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
>  	 * allocated with any setting of ->nr_requests
>  	 */
>  	if (rl->count[is_sync] >= (3 * q->nr_requests / 2))
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	q->nr_rqs[is_sync]++;
>  	rl->count[is_sync]++;
> @@ -1055,7 +1055,7 @@ fail_alloc:
>  rq_starved:
>  	if (unlikely(rl->count[is_sync] == 0))
>  		rl->starved[is_sync] = 1;
> -	return NULL;
> +	return ERR_PTR(-ENOMEM);
>  }
>  
>  /**
> @@ -1068,9 +1068,9 @@ rq_starved:
>   * Get a free request from @q.  If %__GFP_WAIT is set in @gfp_mask, this
>   * function keeps retrying under memory pressure and fails iff @q is dead.
>   *
> - * Must be callled with @q->queue_lock held and,
> - * Returns %NULL on failure, with @q->queue_lock held.
> - * Returns !%NULL on success, with @q->queue_lock *not held*.
> + * Must be called with @q->queue_lock held and,
> + * Returns ERR_PTR on failure, with @q->queue_lock held.
> + * Returns request pointer on success, with @q->queue_lock *not held*.
>   */
>  static struct request *get_request(struct request_queue *q, int rw_flags,
>  				   struct bio *bio, gfp_t gfp_mask)
> @@ -1083,12 +1083,12 @@ static struct request *get_request(struct request_queue *q, int rw_flags,
>  	rl = blk_get_rl(q, bio);	/* transferred to @rq on success */
>  retry:
>  	rq = __get_request(rl, rw_flags, bio, gfp_mask);
> -	if (rq)
> +	if (!IS_ERR(rq))
>  		return rq;
>  
>  	if (!(gfp_mask & __GFP_WAIT) || unlikely(blk_queue_dying(q))) {
>  		blk_put_rl(rl);
> -		return NULL;
> +		return rq;
>  	}
>  
>  	/* wait on @rl and retry */
> @@ -1125,7 +1125,7 @@ static struct request *blk_old_get_request(struct request_queue *q, int rw,
>  
>  	spin_lock_irq(q->queue_lock);
>  	rq = get_request(q, rw, NULL, gfp_mask);
> -	if (!rq)
> +	if (IS_ERR(rq))
>  		spin_unlock_irq(q->queue_lock);
>  	/* q->queue_lock is unlocked at this point */
>  
> @@ -1177,8 +1177,8 @@ struct request *blk_make_request(struct request_queue *q, struct bio *bio,
>  {
>  	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
>  
> -	if (unlikely(!rq))
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(rq))
> +		return rq;
>  
>  	for_each_bio(bio) {
>  		struct bio *bounce_bio = bio;
> @@ -1559,8 +1559,8 @@ get_rq:
>  	 * Returns with the queue unlocked.
>  	 */
>  	req = get_request(q, rw_flags, bio, GFP_NOIO);
> -	if (unlikely(!req)) {
> -		bio_endio(bio, -ENODEV);	/* @q is dead */
> +	if (IS_ERR(req)) {
> +		bio_endio(bio, PTR_ERR(req));	/* @q is dead */
>  		goto out_unlock;
>  	}
>  
> diff --git a/block/bsg.c b/block/bsg.c
> index 420a5a9..2b1c322 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -271,8 +271,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
>  	 * map scatter-gather elements separately and string them to request
>  	 */
>  	rq = blk_get_request(q, rw, GFP_KERNEL);
> -	if (!rq)
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(rq))
> +		return rq;
>  	ret = blk_fill_sgv4_hdr_rq(q, rq, hdr, bd, has_write_perm);
>  	if (ret)
>  		goto out;
> @@ -284,8 +284,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
>  		}
>  
>  		next_rq = blk_get_request(q, READ, GFP_KERNEL);
> -		if (!next_rq) {
> -			ret = -ENOMEM;
> +		if (IS_ERR(next_rq)) {
> +			ret = PTR_ERR(rq);
>  			goto out;
>  		}
>  		rq->next_rq = next_rq;
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index e6485c9..f901fb5 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -313,8 +313,8 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
>  		}
>  
>  	rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);
> -	if (!rq)
> -		return -ENOMEM;
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
>  
>  	if (blk_fill_sghdr_rq(q, rq, hdr, mode)) {
>  		blk_put_request(rq);
> @@ -442,8 +442,8 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
>  	}
>  
>  	rq = blk_get_request(q, in_len ? WRITE : READ, __GFP_WAIT);
> -	if (!rq) {
> -		err = -ENODEV;
> +	if (IS_ERR(rq)) {
> +		err = PTR_ERR(rq);
>  		goto error_free_buffer;
>  	}
>  
> @@ -533,8 +533,8 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
>  	int err;
>  
>  	rq = blk_get_request(q, WRITE, __GFP_WAIT);
> -	if (!rq)
> -		return -ENODEV;
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
>  	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>  	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
>  	rq->cmd[0] = cmd;
> diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
> index 856178a..2af4101 100644
> --- a/drivers/block/paride/pd.c
> +++ b/drivers/block/paride/pd.c
> @@ -722,8 +722,8 @@ static int pd_special_command(struct pd_unit *disk,
>  	int err = 0;
>  
>  	rq = blk_get_request(disk->gd->queue, READ, __GFP_WAIT);
> -	if (!rq)
> -		return -ENODEV;
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
>  
>  	rq->cmd_type = REQ_TYPE_SPECIAL;
>  	rq->special = func;
> diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> index ff39837..1de6e90 100644
> --- a/drivers/block/pktcdvd.c
> +++ b/drivers/block/pktcdvd.c
> @@ -704,8 +704,8 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
>  
>  	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
>  			     WRITE : READ, __GFP_WAIT);
> -	if (!rq)
> -		return -ENODEV;
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
>  
>  	if (cgc->buflen) {
>  		ret = blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen,
> diff --git a/drivers/block/sx8.c b/drivers/block/sx8.c
> index d5e2d12..5d55285 100644
> --- a/drivers/block/sx8.c
> +++ b/drivers/block/sx8.c
> @@ -568,7 +568,7 @@ static struct carm_request *carm_get_special(struct carm_host *host)
>  		return NULL;
>  
>  	rq = blk_get_request(host->oob_q, WRITE /* bogus */, GFP_KERNEL);
> -	if (!rq) {
> +	if (IS_ERR(rq)) {
>  		spin_lock_irqsave(&host->lock, flags);
>  		carm_put_request(host, crq);
>  		spin_unlock_irqrestore(&host->lock, flags);
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 8a3aff7..85e63d3 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -2161,8 +2161,8 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
>  		len = nr * CD_FRAMESIZE_RAW;
>  
>  		rq = blk_get_request(q, READ, GFP_KERNEL);
> -		if (!rq) {
> -			ret = -ENOMEM;
> +		if (IS_ERR(rq)) {
> +			ret = PTR_ERR(rq);
>  			break;
>  		}
>  
> diff --git a/drivers/ide/ide-park.c b/drivers/ide/ide-park.c
> index f41558a..ca95860 100644
> --- a/drivers/ide/ide-park.c
> +++ b/drivers/ide/ide-park.c
> @@ -46,7 +46,7 @@ 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, READ, GFP_NOWAIT);
> -	if (unlikely(!rq))
> +	if (IS_ERR(rq))
>  		goto out;
>  
>  	rq->cmd[0] = REQ_UNPARK_HEADS;
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 5248c88..5aefcf9 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -115,7 +115,7 @@ static struct request *get_alua_req(struct scsi_device *sdev,
>  
>  	rq = blk_get_request(q, rw, GFP_NOIO);
>  
> -	if (!rq) {
> +	if (IS_ERR(rq)) {
>  		sdev_printk(KERN_INFO, sdev,
>  			    "%s: blk_get_request failed\n", __func__);
>  		return NULL;
> diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
> index e1c8be0..2ec3261 100644
> --- a/drivers/scsi/device_handler/scsi_dh_emc.c
> +++ b/drivers/scsi/device_handler/scsi_dh_emc.c
> @@ -275,7 +275,7 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
>  
>  	rq = blk_get_request(sdev->request_queue,
>  			(cmd != INQUIRY) ? WRITE : READ, GFP_NOIO);
> -	if (!rq) {
> +	if (IS_ERR(rq)) {
>  		sdev_printk(KERN_INFO, sdev, "get_req: blk_get_request failed");
>  		return NULL;
>  	}
> diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> index 084062b..1cf4019 100644
> --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> @@ -117,7 +117,7 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
>  
>  retry:
>  	req = blk_get_request(sdev->request_queue, WRITE, GFP_NOIO);
> -	if (!req)
> +	if (IS_ERR(req))
>  		return SCSI_DH_RES_TEMP_UNAVAIL;
>  
>  	req->cmd_type = REQ_TYPE_BLOCK_PC;
> @@ -247,7 +247,7 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h)
>  	struct request *req;
>  
>  	req = blk_get_request(h->sdev->request_queue, WRITE, GFP_ATOMIC);
> -	if (!req)
> +	if (IS_ERR(req))
>  		return SCSI_DH_RES_TEMP_UNAVAIL;
>  
>  	req->cmd_type = REQ_TYPE_BLOCK_PC;
> diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
> index 4b9cf93..5f6e5d8 100644
> --- a/drivers/scsi/device_handler/scsi_dh_rdac.c
> +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
> @@ -274,7 +274,7 @@ static struct request *get_rdac_req(struct scsi_device *sdev,
>  
>  	rq = blk_get_request(q, rw, GFP_NOIO);
>  
> -	if (!rq) {
> +	if (IS_ERR(rq)) {
>  		sdev_printk(KERN_INFO, sdev,
>  				"get_rdac_req: blk_get_request failed.\n");
>  		return NULL;
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index bac04c2..831f1f9 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -1567,8 +1567,8 @@ static struct request *_make_request(struct request_queue *q, bool has_write,
>  		struct request *req;
>  
>  		req = blk_get_request(q, has_write ? WRITE : READ, flags);
> -		if (unlikely(!req))
> -			return ERR_PTR(-ENOMEM);
> +		if (IS_ERR(req))
> +			return req;
>  
>  		return req;
>  	}
> diff --git a/drivers/scsi/osst.c b/drivers/scsi/osst.c
> index 21883a2..826cda9 100644
> --- a/drivers/scsi/osst.c
> +++ b/drivers/scsi/osst.c
> @@ -362,7 +362,7 @@ static int osst_execute(struct osst_request *SRpnt, const unsigned char *cmd,
>  	int write = (data_direction == DMA_TO_DEVICE);
>  
>  	req = blk_get_request(SRpnt->stp->device->request_queue, write, GFP_KERNEL);
> -	if (!req)
> +	if (IS_ERR(req))
>  		return DRIVER_ERROR << 24;
>  
>  	req->cmd_type = REQ_TYPE_BLOCK_PC;
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index d50531f..ca038ee 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1950,7 +1950,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
>  	 * request becomes available
>  	 */
>  	req = blk_get_request(sdev->request_queue, READ, GFP_KERNEL);
> -	if (!req)
> +	if (IS_ERR(req))
>  		return;
>  
>  	req->cmd[0] = ALLOW_MEDIUM_REMOVAL;
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 9db097a..b25fab0b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -193,7 +193,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
>  	int ret = DRIVER_ERROR << 24;
>  
>  	req = blk_get_request(sdev->request_queue, write, __GFP_WAIT);
> -	if (!req)
> +	if (IS_ERR(req))
>  		return ret;
>  
>  	if (bufflen &&	blk_rq_map_kern(sdev->request_queue, req,
> diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
> index e51add0..b14c15b 100644
> --- a/drivers/scsi/scsi_tgt_lib.c
> +++ b/drivers/scsi/scsi_tgt_lib.c
> @@ -97,7 +97,7 @@ struct scsi_cmnd *scsi_host_get_command(struct Scsi_Host *shost,
>  	 * we are in target mode we want the opposite.
>  	 */
>  	rq = blk_get_request(shost->uspace_req_q, !write, gfp_mask);
> -	if (!rq)
> +	if (IS_ERR(rq))
>  		goto free_tcmd;
>  
>  	cmd = __scsi_get_command(shost, gfp_mask);
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index df5e961..2e54684 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1650,8 +1650,8 @@ static int sg_start_req(Sg_request *srp, unsigned char *cmd)
>  				   dxfer_len));
>  
>  	rq = blk_get_request(q, rw, GFP_ATOMIC);
> -	if (!rq)
> -		return -ENOMEM;
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
>  
>  	memcpy(rq->cmd, cmd, hp->cmd_len);
>  
> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index afc834e..48a6cef 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -481,7 +481,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd,
>  
>  	req = blk_get_request(SRpnt->stp->device->request_queue, write,
>  			      GFP_KERNEL);
> -	if (!req)
> +	if (IS_ERR(req))
>  		return DRIVER_ERROR << 24;
>  
>  	req->cmd_type = REQ_TYPE_BLOCK_PC;
> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> index 0f199f6..23d340d 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -1050,7 +1050,7 @@ pscsi_execute_cmd(struct se_cmd *cmd)
>  		req = blk_get_request(pdv->pdv_sd->request_queue,
>  				(data_direction == DMA_TO_DEVICE),
>  				GFP_KERNEL);
> -		if (!req) {
> +		if (IS_ERR(req)) {
>  			pr_err("PSCSI: blk_get_request() failed\n");
>  			ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>  			goto fail;

Re: [PATCH 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

[Joe Lawrence]

On Tue, 3 Jun 2014 15:45:36 -0400
Jeff Moyer <jmoyer@redhat.com> wrote:

> Joe Lawrence <joe.lawrence@stratus.com> writes:
> 
> > The blk_get_request function may fail in low-memory conditions or during
> > device removal (even if __GFP_WAIT is set). To distinguish between these
> > errors, modify the blk_get_request call stack to return the appropriate
> > ERR_PTR. Verify that all callers check the return status and consider
> > IS_ERR instead of a simple NULL pointer check.
> 
> I'm curious to know what testing you did, and what code paths do
> anything different for ENOMEM vs EIO.  I guess ENOMEM may make it all
> the way back to userspace?

Hi Jeff,

Good question.  In the case of the short repro program provided by
Paolo in [1], the ioctl call returned error and set -ENODEV.

As for each individual caller of blk_get_request, I did not audit all
code paths back out to user space.  I can take a look, but I'm guessing
that 24-ish callers might make for quite a *few* scenarios to verify.

Conversion to ERR_PTR was suggested by Jens as a complete fix, but
without sufficient Ack's, I can understand holding off on this part of
the set.

-- Joe

[1] http://thread.gmane.org/gmane.linux.scsi/85824

Re: [PATCH 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

[Jeff Moyer]

Joe Lawrence <joe.lawrence@stratus.com> writes:

> diff --git a/block/bsg.c b/block/bsg.c
> index 420a5a9..2b1c322 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -271,8 +271,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
>  	 * map scatter-gather elements separately and string them to request
>  	 */
>  	rq = blk_get_request(q, rw, GFP_KERNEL);
> -	if (!rq)
> -		return ERR_PTR(-ENOMEM);
> +	if (IS_ERR(rq))
> +		return rq;
>  	ret = blk_fill_sgv4_hdr_rq(q, rq, hdr, bd, has_write_perm);
>  	if (ret)
>  		goto out;
> @@ -284,8 +284,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
>  		}
>  
>  		next_rq = blk_get_request(q, READ, GFP_KERNEL);
> -		if (!next_rq) {
> -			ret = -ENOMEM;
> +		if (IS_ERR(next_rq)) {
> +			ret = PTR_ERR(rq);

ITYM ret = PTR_ERR(next_rq), right?

> diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> index 084062b..1cf4019 100644
> --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> @@ -117,7 +117,7 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
>  
>  retry:
>  	req = blk_get_request(sdev->request_queue, WRITE, GFP_NOIO);
> -	if (!req)
> +	if (IS_ERR(req))
>  		return SCSI_DH_RES_TEMP_UNAVAIL;
>  

Don't we have an opportunity here to differentiate between enomem and
eio?  I have no idea what the right SCSI_DH error would be, I'm just
asking the question.

>  	req->cmd_type = REQ_TYPE_BLOCK_PC;
> @@ -247,7 +247,7 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h)
>  	struct request *req;
>  
>  	req = blk_get_request(h->sdev->request_queue, WRITE, GFP_ATOMIC);
> -	if (!req)
> +	if (IS_ERR(req))
>  		return SCSI_DH_RES_TEMP_UNAVAIL;

Same goes here.

Cheers,
Jeff

Re: [PATCH 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

[Joe Lawrence]

On Tue, 3 Jun 2014 16:07:01 -0400
Jeff Moyer <jmoyer@redhat.com> wrote:

> Joe Lawrence <joe.lawrence@stratus.com> writes:
> 
> > diff --git a/block/bsg.c b/block/bsg.c
> > index 420a5a9..2b1c322 100644
> > --- a/block/bsg.c
> > +++ b/block/bsg.c
> > @@ -271,8 +271,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
> >  	 * map scatter-gather elements separately and string them to request
> >  	 */
> >  	rq = blk_get_request(q, rw, GFP_KERNEL);
> > -	if (!rq)
> > -		return ERR_PTR(-ENOMEM);
> > +	if (IS_ERR(rq))
> > +		return rq;
> >  	ret = blk_fill_sgv4_hdr_rq(q, rq, hdr, bd, has_write_perm);
> >  	if (ret)
> >  		goto out;
> > @@ -284,8 +284,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
> >  		}
> >  
> >  		next_rq = blk_get_request(q, READ, GFP_KERNEL);
> > -		if (!next_rq) {
> > -			ret = -ENOMEM;
> > +		if (IS_ERR(next_rq)) {
> > +			ret = PTR_ERR(rq);
> 
> ITYM ret = PTR_ERR(next_rq), right?

Oops, you are right of course.  I can include this change if there should be another version (Jens?)

> > diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> > index 084062b..1cf4019 100644
> > --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> > +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
> > @@ -117,7 +117,7 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
> >  
> >  retry:
> >  	req = blk_get_request(sdev->request_queue, WRITE, GFP_NOIO);
> > -	if (!req)
> > +	if (IS_ERR(req))
> >  		return SCSI_DH_RES_TEMP_UNAVAIL;
> >  
> 
> Don't we have an opportunity here to differentiate between enomem and
> eio?  I have no idea what the right SCSI_DH error would be, I'm just
> asking the question.
> 
> >  	req->cmd_type = REQ_TYPE_BLOCK_PC;
> > @@ -247,7 +247,7 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h)
> >  	struct request *req;
> >  
> >  	req = blk_get_request(h->sdev->request_queue, WRITE, GFP_ATOMIC);
> > -	if (!req)
> > +	if (IS_ERR(req))
> >  		return SCSI_DH_RES_TEMP_UNAVAIL;
> 
> Same goes here.

This sounded familiar, so I dug through my mailbox and found the
following patch.  When the blk_get_request patches stalled, I stopped
working on the scsi_dh patch.  But I think you would be talking about
the introduction of something like "errno_to_SCSI_DH" below...

Thanks,

-- Joe

-- >8 --

>From dd6d4a0753ca056f85df411507a17284e08b0da7 Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence@stratus.com>
Date: Thu, 25 Apr 2013 16:00:51 -0400
Subject: [PATCH draft 2/2] scsi_dh: cleanup blk_get_request callers

Collect common code scattered throughout the SCSI device handler drivers
that call blk_get_request.  Create a wrapper in scsi_dh that gets the
request, maps a kernel buffer, and sets command failfast behavior.
Callers should be prepared to handle various SCSI_DH_* error codes.

Also fix the callers of the scsi_dh_emc routine send_inquiry_cmd to
properly handle errors when the sense length is zero.

Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com>
---
 drivers/scsi/device_handler/scsi_dh.c       |  58 +++++++++++++++
 drivers/scsi/device_handler/scsi_dh_alua.c  |  58 +++++----------
 drivers/scsi/device_handler/scsi_dh_emc.c   | 111 ++++++++++------------------
 drivers/scsi/device_handler/scsi_dh_hp_sw.c |  36 ++++-----
 drivers/scsi/device_handler/scsi_dh_rdac.c  |  60 +++++----------
 include/scsi/scsi_dh.h                      |  10 +++
 6 files changed, 161 insertions(+), 172 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index 33e422e..b629de0 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -561,6 +561,64 @@ const char *scsi_dh_attached_handler_name(struct request_queue *q, gfp_t gfp)
 }
 EXPORT_SYMBOL_GPL(scsi_dh_attached_handler_name);
 
+#define errno_to_SCSI_DH(errno)						\
+	(((errno) == -ENODEV) ? SCSI_DH_DEV_OFFLINED			\
+			      : SCSI_DH_RES_TEMP_UNAVAIL)		\
+
+/*
+ * scsi_dh_get_request - Wrapper of blk_get_request for device handlers
+ * @rqp - pointer to new request pointer
+ * @rw - request RW and SYNC flags
+ * @sdev - SCSI device of associated request queue
+ * @buf - kernel buffer to map into request
+ * @buflen - kernel buffer length
+ * @timeout - request timeout
+ * @retries - request retries
+ *
+ * Get a free request from SCSI device request queue.  Maps kernel data
+ * buffer if @buf and @buflen are non-zero.  Sets new request structure
+ * timeout and retries members.
+ *
+ * Returns SCSI_DH_OK and sets @*rqp on success, SCSI_DH_* on error.
+ */
+int scsi_dh_get_request(struct request **rqp, int rw, gfp_t gfp_mask,
+	struct scsi_device *sdev, void *buf, unsigned buflen,
+	unsigned timeout, int retries)
+{
+	struct request *rq;
+	struct request_queue *q = sdev->request_queue;
+	int err;
+
+	rq = blk_get_request(q, rw, gfp_mask);
+
+	if (IS_ERR(rq)) {
+		sdev_printk(KERN_INFO, sdev,
+			    "%s: blk_get_request failed\n", __func__);
+		return errno_to_SCSI_DH(PTR_ERR(rq));
+	}
+
+	if (buf && buflen) {
+		err = blk_rq_map_kern(q, rq, buf, buflen, gfp_mask);
+		if (err) {
+			blk_put_request(rq);
+			sdev_printk(KERN_INFO, sdev,
+				    "%s: blk__rq_map_kern failed.\n", __func__);
+			return errno_to_SCSI_DH(err);
+		}
+	}
+
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
+	rq->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
+			 REQ_FAILFAST_DRIVER;
+	rq->timeout = timeout;
+	rq->retries = retries;
+
+	*rqp = rq;
+
+	return SCSI_DH_OK;
+}
+EXPORT_SYMBOL_GPL(scsi_dh_get_request);
+
 static struct notifier_block scsi_dh_nb = {
 	.notifier_call = scsi_dh_notifier
 };
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index f07f152..ed9e0c7 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -107,36 +107,6 @@ static int realloc_buffer(struct alua_dh_data *h, unsigned len)
 	return 0;
 }
 
-static struct request *get_alua_req(struct scsi_device *sdev,
-				    void *buffer, unsigned buflen, int rw)
-{
-	struct request *rq;
-	struct request_queue *q = sdev->request_queue;
-
-	rq = blk_get_request(q, rw, GFP_NOIO);
-
-	if (IS_ERR(rq)) {
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: blk_get_request failed\n", __func__);
-		return NULL;
-	}
-
-	if (buflen && blk_rq_map_kern(q, rq, buffer, buflen, GFP_NOIO)) {
-		blk_put_request(rq);
-		sdev_printk(KERN_INFO, sdev,
-			    "%s: blk_rq_map_kern failed\n", __func__);
-		return NULL;
-	}
-
-	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-	rq->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
-			 REQ_FAILFAST_DRIVER;
-	rq->retries = ALUA_FAILOVER_RETRIES;
-	rq->timeout = ALUA_FAILOVER_TIMEOUT * HZ;
-
-	return rq;
-}
-
 /*
  * submit_vpd_inquiry - Issue an INQUIRY VPD page 0x83 command
  * @sdev: sdev the command should be sent to
@@ -144,10 +114,12 @@ static struct request *get_alua_req(struct scsi_device *sdev,
 static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h)
 {
 	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
+	int err;
 
-	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
+	err = scsi_dh_get_request(&rq, READ, GFP_NOIO, sdev, h->buff,
+				  h->bufflen, ALUA_FAILOVER_TIMEOUT * HZ,
+				  ALUA_FAILOVER_RETRIES);
+	if (err)
 		goto done;
 
 	/* Prepare the command. */
@@ -182,10 +154,12 @@ static unsigned submit_rtpg(struct scsi_device *sdev, struct alua_dh_data *h,
 			    bool rtpg_ext_hdr_req)
 {
 	struct request *rq;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
+	int err;
 
-	rq = get_alua_req(sdev, h->buff, h->bufflen, READ);
-	if (!rq)
+	err = scsi_dh_get_request(&rq, READ, GFP_NOIO, sdev, h->buff,
+				  h->bufflen, ALUA_FAILOVER_TIMEOUT * HZ,
+				  ALUA_FAILOVER_RETRIES);
+	if (err)
 		goto done;
 
 	/* Prepare the command. */
@@ -285,6 +259,7 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 	struct request *rq;
 	int stpg_len = 8;
 	struct scsi_device *sdev = h->sdev;
+	int err;
 
 	/* Prepare the data buffer */
 	memset(h->buff, 0, stpg_len);
@@ -292,9 +267,11 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 	h->buff[6] = (h->group_id >> 8) & 0xff;
 	h->buff[7] = h->group_id & 0xff;
 
-	rq = get_alua_req(sdev, h->buff, stpg_len, WRITE);
-	if (!rq)
-		return SCSI_DH_RES_TEMP_UNAVAIL;
+	err = scsi_dh_get_request(&rq, WRITE, GFP_NOIO, sdev, h->buff,
+				  stpg_len, ALUA_FAILOVER_TIMEOUT * HZ,
+				  ALUA_FAILOVER_RETRIES);
+	if (err)
+		goto done;
 
 	/* Prepare the command. */
 	rq->cmd[0] = MAINTENANCE_OUT;
@@ -311,7 +288,8 @@ static unsigned submit_stpg(struct alua_dh_data *h)
 	rq->end_io_data = h;
 
 	blk_execute_rq_nowait(rq->q, NULL, rq, 1, stpg_endio);
-	return SCSI_DH_OK;
+done:
+	return err;
 }
 
 /*
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 2ec3261..eab62c5 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -260,82 +260,32 @@ out:
 	return sp_model;
 }
 
-/*
- * Get block request for REQ_BLOCK_PC command issued to path.  Currently
- * limited to MODE_SELECT (trespass) and INQUIRY (VPD page 0xC0) commands.
- *
- * Uses data and sense buffers in hardware handler context structure and
- * assumes serial servicing of commands, both issuance and completion.
- */
-static struct request *get_req(struct scsi_device *sdev, int cmd,
-				unsigned char *buffer)
-{
-	struct request *rq;
-	int len = 0;
-
-	rq = blk_get_request(sdev->request_queue,
-			(cmd != INQUIRY) ? WRITE : READ, GFP_NOIO);
-	if (IS_ERR(rq)) {
-		sdev_printk(KERN_INFO, sdev, "get_req: blk_get_request failed");
-		return NULL;
-	}
-
-	rq->cmd_len = COMMAND_SIZE(cmd);
-	rq->cmd[0] = cmd;
-
-	switch (cmd) {
-	case MODE_SELECT:
-		len = sizeof(short_trespass);
-		rq->cmd[1] = 0x10;
-		rq->cmd[4] = len;
-		break;
-	case MODE_SELECT_10:
-		len = sizeof(long_trespass);
-		rq->cmd[1] = 0x10;
-		rq->cmd[8] = len;
-		break;
-	case INQUIRY:
-		len = CLARIION_BUFFER_SIZE;
-		rq->cmd[4] = len;
-		memset(buffer, 0, len);
-		break;
-	default:
-		BUG_ON(1);
-		break;
-	}
-
-	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-	rq->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
-			 REQ_FAILFAST_DRIVER;
-	rq->timeout = CLARIION_TIMEOUT;
-	rq->retries = CLARIION_RETRIES;
-
-	if (blk_rq_map_kern(rq->q, rq, buffer, len, GFP_NOIO)) {
-		blk_put_request(rq);
-		return NULL;
-	}
-
-	return rq;
-}
-
 static int send_inquiry_cmd(struct scsi_device *sdev, int page,
 			    struct clariion_dh_data *csdev)
 {
-	struct request *rq = get_req(sdev, INQUIRY, csdev->buffer);
+	struct request *rq;
 	int err;
 
-	if (!rq)
-		return SCSI_DH_RES_TEMP_UNAVAIL;
-
-	rq->sense = csdev->sense;
-	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
-	rq->sense_len = csdev->senselen = 0;
+	err = scsi_dh_get_request(&rq, READ, GFP_NOIO, sdev, csdev->buffer,
+				  CLARIION_BUFFER_SIZE, CLARIION_TIMEOUT,
+				  CLARIION_RETRIES);
+	if (err)
+		goto done;
 
+	/* Prepare the command. */
 	rq->cmd[0] = INQUIRY;
 	if (page != 0) {
 		rq->cmd[1] = 1;
 		rq->cmd[2] = page;
 	}
+	rq->cmd[4] = CLARIION_BUFFER_SIZE;
+	memset(csdev->buffer, 0, CLARIION_BUFFER_SIZE);
+	rq->cmd_len = COMMAND_SIZE(INQUIRY);
+
+	rq->sense = csdev->sense;
+	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
+	rq->sense_len = csdev->senselen = 0;
+
 	err = blk_execute_rq(sdev->request_queue, NULL, rq, 1);
 	if (err == -EIO) {
 		sdev_printk(KERN_INFO, sdev,
@@ -347,7 +297,7 @@ static int send_inquiry_cmd(struct scsi_device *sdev, int page,
 	}
 
 	blk_put_request(rq);
-
+done:
 	return err;
 }
 
@@ -376,9 +326,19 @@ static int send_trespass_cmd(struct scsi_device *sdev,
 	BUG_ON((len > CLARIION_BUFFER_SIZE));
 	memcpy(csdev->buffer, page22, len);
 
-	rq = get_req(sdev, cmd, csdev->buffer);
-	if (!rq)
-		return SCSI_DH_RES_TEMP_UNAVAIL;
+	err = scsi_dh_get_request(&rq, WRITE, GFP_NOIO, sdev, csdev->buffer,
+				  len, CLARIION_TIMEOUT, CLARIION_RETRIES);
+	if (err)
+		goto done;
+
+	/* Prepare the command. */
+	rq->cmd[0] = cmd;
+	rq->cmd[1] = 0x10;
+	if (cmd == MODE_SELECT)
+		rq->cmd[4] = len;
+	else if (cmd == MODE_SELECT_10)
+		rq->cmd[8] = len;
+	rq->cmd_len = COMMAND_SIZE(cmd);
 
 	rq->sense = csdev->sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
@@ -396,7 +356,7 @@ static int send_trespass_cmd(struct scsi_device *sdev,
 	}
 
 	blk_put_request(rq);
-
+done:
 	return err;
 }
 
@@ -468,9 +428,12 @@ static int clariion_std_inquiry(struct scsi_device *sdev,
 	char *sp_model;
 
 	err = send_inquiry_cmd(sdev, 0, csdev);
-	if (err != SCSI_DH_OK && csdev->senselen) {
+	if (err != SCSI_DH_OK) {
 		struct scsi_sense_hdr sshdr;
 
+		if (!csdev->senselen)
+			goto out;
+
 		if (scsi_normalize_sense(csdev->sense, SCSI_SENSE_BUFFERSIZE,
 					 &sshdr)) {
 			sdev_printk(KERN_ERR, sdev, "%s: INQUIRY sense code "
@@ -507,9 +470,12 @@ static int clariion_send_inquiry(struct scsi_device *sdev,
 
 retry:
 	err = send_inquiry_cmd(sdev, 0xC0, csdev);
-	if (err != SCSI_DH_OK && csdev->senselen) {
+	if (err != SCSI_DH_OK) {
 		struct scsi_sense_hdr sshdr;
 
+		if (!csdev->senselen)
+			goto done;
+
 		err = scsi_normalize_sense(csdev->sense, SCSI_SENSE_BUFFERSIZE,
 					   &sshdr);
 		if (!err)
@@ -527,6 +493,7 @@ retry:
 	} else {
 		err = parse_sp_info_reply(sdev, csdev);
 	}
+done:
 	return err;
 }
 
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 1cf4019..2d03c4d 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -116,16 +116,15 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
 	int ret;
 
 retry:
-	req = blk_get_request(sdev->request_queue, WRITE, GFP_NOIO);
-	if (IS_ERR(req))
-		return SCSI_DH_RES_TEMP_UNAVAIL;
+	ret = scsi_dh_get_request(&req, WRITE, GFP_NOIO, sdev, 0, 0,
+				  HP_SW_TIMEOUT, 0);
+	if (ret)
+		goto done;
 
-	req->cmd_type = REQ_TYPE_BLOCK_PC;
-	req->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
-			  REQ_FAILFAST_DRIVER;
-	req->cmd_len = COMMAND_SIZE(TEST_UNIT_READY);
+	/* Prepare the command. */
 	req->cmd[0] = TEST_UNIT_READY;
-	req->timeout = HP_SW_TIMEOUT;
+	req->cmd_len = COMMAND_SIZE(TEST_UNIT_READY);
+
 	req->sense = h->sense;
 	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	req->sense_len = 0;
@@ -154,7 +153,7 @@ retry:
 	}
 
 	blk_put_request(req);
-
+done:
 	return ret;
 }
 
@@ -245,25 +244,26 @@ done:
 static int hp_sw_start_stop(struct hp_sw_dh_data *h)
 {
 	struct request *req;
+	int ret;
 
-	req = blk_get_request(h->sdev->request_queue, WRITE, GFP_ATOMIC);
-	if (IS_ERR(req))
-		return SCSI_DH_RES_TEMP_UNAVAIL;
+	ret = scsi_dh_get_request(&req, WRITE, GFP_ATOMIC, h->sdev, 0, 0,
+				  HP_SW_TIMEOUT, 0);
+	if (ret)
+		goto done;
 
-	req->cmd_type = REQ_TYPE_BLOCK_PC;
-	req->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
-			  REQ_FAILFAST_DRIVER;
-	req->cmd_len = COMMAND_SIZE(START_STOP);
+	/* Prepare the command. */
 	req->cmd[0] = START_STOP;
 	req->cmd[4] = 1;	/* Start spin cycle */
-	req->timeout = HP_SW_TIMEOUT;
+	req->cmd_len = COMMAND_SIZE(START_STOP);
+
 	req->sense = h->sense;
 	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	req->sense_len = 0;
 	req->end_io_data = h;
 
 	blk_execute_rq_nowait(req->q, NULL, req, 1, start_stop_endio);
-	return SCSI_DH_OK;
+done:
+	return ret;
 }
 
 static int hp_sw_prep_fn(struct scsi_device *sdev, struct request *req)
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 009bd8f..26ed28f 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -266,44 +266,16 @@ static inline struct rdac_dh_data *get_rdac_data(struct scsi_device *sdev)
 	return ((struct rdac_dh_data *) scsi_dh_data->buf);
 }
 
-static struct request *get_rdac_req(struct scsi_device *sdev,
-			void *buffer, unsigned buflen, int rw)
-{
-	struct request *rq;
-	struct request_queue *q = sdev->request_queue;
-
-	rq = blk_get_request(q, rw, GFP_NOIO);
-
-	if (IS_ERR(rq)) {
-		sdev_printk(KERN_INFO, sdev,
-				"get_rdac_req: blk_get_request failed.\n");
-		return NULL;
-	}
-
-	if (buflen && blk_rq_map_kern(q, rq, buffer, buflen, GFP_NOIO)) {
-		blk_put_request(rq);
-		sdev_printk(KERN_INFO, sdev,
-				"get_rdac_req: blk_rq_map_kern failed.\n");
-		return NULL;
-	}
-
-	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-	rq->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
-			 REQ_FAILFAST_DRIVER;
-	rq->retries = RDAC_RETRIES;
-	rq->timeout = RDAC_TIMEOUT;
-
-	return rq;
-}
-
-static struct request *rdac_failover_get(struct scsi_device *sdev,
-			struct rdac_dh_data *h, struct list_head *list)
+static int rdac_failover_get(struct request **rqp,
+		struct scsi_device *sdev, struct rdac_dh_data *h,
+		struct list_head *list)
 {
 	struct request *rq;
 	struct rdac_mode_common *common;
 	unsigned data_size;
 	struct rdac_queue_data *qdata;
 	u8 *lun_table;
+	int err;
 
 	if (h->ctlr->use_ms10) {
 		struct rdac_pg_expanded *rdac_pg;
@@ -337,9 +309,11 @@ static struct request *rdac_failover_get(struct scsi_device *sdev,
 	}
 
 	/* get request for block layer packet command */
-	rq = get_rdac_req(sdev, &h->ctlr->mode_select, data_size, WRITE);
-	if (!rq)
-		return NULL;
+	err = scsi_dh_get_request(&rq, WRITE, GFP_NOIO, sdev,
+				  &h->ctlr->mode_select, data_size,
+				  RDAC_TIMEOUT, RDAC_RETRIES);
+	if (err)
+		goto done;
 
 	/* Prepare the command. */
 	if (h->ctlr->use_ms10) {
@@ -356,7 +330,9 @@ static struct request *rdac_failover_get(struct scsi_device *sdev,
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	rq->sense_len = 0;
 
-	return rq;
+	*rqp = rq;
+done:
+	return err;
 }
 
 static void release_controller(struct kref *kref)
@@ -408,10 +384,11 @@ static int submit_inquiry(struct scsi_device *sdev, int page_code,
 {
 	struct request *rq;
 	struct request_queue *q = sdev->request_queue;
-	int err = SCSI_DH_RES_TEMP_UNAVAIL;
+	int err;
 
-	rq = get_rdac_req(sdev, &h->inq, len, READ);
-	if (!rq)
+	err = scsi_dh_get_request(&rq, READ, GFP_NOIO, sdev, &h->inq, len,
+				  RDAC_TIMEOUT, RDAC_RETRIES);
+	if (err)
 		goto done;
 
 	/* Prepare the command. */
@@ -603,9 +580,8 @@ static void send_mode_select(struct work_struct *work)
 	spin_unlock(&ctlr->ms_lock);
 
 retry:
-	err = SCSI_DH_RES_TEMP_UNAVAIL;
-	rq = rdac_failover_get(sdev, h, &list);
-	if (!rq)
+	err = rdac_failover_get(&rq, sdev, h, &list);
+	if (err)
 		goto done;
 
 	RDAC_LOG(RDAC_LOG_FAILOVER, sdev, "array %s, ctlr %d, "
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 620c723..c3bc702 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -62,6 +62,9 @@ extern int scsi_dh_attach(struct request_queue *, const char *);
 extern void scsi_dh_detach(struct request_queue *);
 extern const char *scsi_dh_attached_handler_name(struct request_queue *, gfp_t);
 extern int scsi_dh_set_params(struct request_queue *, const char *);
+extern int scsi_dh_get_request(struct request **, int, gfp_t,
+			       struct scsi_device *, void *, unsigned len,
+			       unsigned timeout, int retries);
 #else
 static inline int scsi_dh_activate(struct request_queue *req,
 					activate_complete fn, void *data)
@@ -90,4 +93,11 @@ static inline int scsi_dh_set_params(struct request_queue *req, const char *para
 {
 	return -SCSI_DH_NOSYS;
 }
+static inline int scsi_dh_get_request(struct request **rqp, int rw,
+				      gfp_t gfp_mask, struct scsi_device *sdev,
+				      void *buf, unsigned len, unsigned timeout,
+				      int retries)
+{
+	return SCSI_DH_NOSYS;
+}
 #endif
-- 
1.8.1.4

Re: [PATCH 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

[Jeff Moyer]

Joe Lawrence <joe.lawrence@stratus.com> writes:

>> ITYM ret = PTR_ERR(next_rq), right?
>
> Oops, you are right of course.  I can include this change if there should be another version (Jens?)
>
>> > diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
>> > index 084062b..1cf4019 100644
>> > --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
>> > +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
>> > @@ -117,7 +117,7 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
>> >  
>> >  retry:
>> >  	req = blk_get_request(sdev->request_queue, WRITE, GFP_NOIO);
>> > -	if (!req)
>> > +	if (IS_ERR(req))
>> >  		return SCSI_DH_RES_TEMP_UNAVAIL;
>> >  
>> 
>> Don't we have an opportunity here to differentiate between enomem and
>> eio?  I have no idea what the right SCSI_DH error would be, I'm just
>> asking the question.

> This sounded familiar, so I dug through my mailbox and found the
> following patch.  When the blk_get_request patches stalled, I stopped
> working on the scsi_dh patch.  But I think you would be talking about
> the introduction of something like "errno_to_SCSI_DH" below...

Wow, that's a lot more churn than I expected.  ;-) I think you should go
ahead and repost this patch with the change mentioned above (you can add
my reviewed-by), and we can revisit this scsi one separately if people
are interested.

Thanks!
Jeff

Re: [PATCH 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

[Christoph Hellwig]

On Wed, Jun 04, 2014 at 02:07:02PM -0400, Jeff Moyer wrote:
> Wow, that's a lot more churn than I expected.  ;-) I think you should go
> ahead and repost this patch with the change mentioned above (you can add
> my reviewed-by), and we can revisit this scsi one separately if people
> are interested.

Joe, did you manage to get back to this?

Re: [PATCH 2/2] block,scsi: convert and handle ERR_PTR from blk_get_request

[Joe Lawrence]

On Tue, 17 Jun 2014 03:33:23 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, Jun 04, 2014 at 02:07:02PM -0400, Jeff Moyer wrote:
> > Wow, that's a lot more churn than I expected.  ;-) I think you should go
> > ahead and repost this patch with the change mentioned above (you can add
> > my reviewed-by), and we can revisit this scsi one separately if people
> > are interested.
> 
> Joe, did you manage to get back to this?

Hi Christoph,

v2 was posted with input from Jeff:

https://lkml.org/lkml/2014/6/4/482

-- Joe