All of lore.kernel.org
 help / color / mirror / Atom feed
* a few passthrough request improvements
@ 2016-06-13 15:21 Christoph Hellwig
  2016-06-13 15:21 ` [PATCH 1/7] memstick: don't allow REQ_TYPE_BLOCK_PC requests Christoph Hellwig
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-13 15:21 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, bhalevy, nab, linux-block

These days we have a lot more users of struct request for driver internal
use than just REQ_TYPE_BLOCK_PC, so let's polish this infrastructure
up a bit.

^ permalink raw reply	[flat|nested] 21+ messages in thread
* [PATCH 1/7] memstick: don't allow REQ_TYPE_BLOCK_PC requests
  2016-06-13 15:21 a few passthrough request improvements Christoph Hellwig
@ 2016-06-13 15:21 ` Christoph Hellwig
  2016-06-13 15:21 ` [PATCH 2/7] virtio_blk: use blk_rq_map_kern Christoph Hellwig
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-13 15:21 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, bhalevy, nab, linux-block

There is no code to issue or handle REQ_TYPE_BLOCK_PC request in the
memstick drivers, so remove the bogus conditional.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/memstick/core/ms_block.c    | 3 +--
 drivers/memstick/core/mspro_block.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c
index 3cd6815..21cc231 100644
--- a/drivers/memstick/core/ms_block.c
+++ b/drivers/memstick/core/ms_block.c
@@ -2002,8 +2002,7 @@ static int msb_bd_getgeo(struct block_device *bdev,
 
 static int msb_prepare_req(struct request_queue *q, struct request *req)
 {
-	if (req->cmd_type != REQ_TYPE_FS &&
-				req->cmd_type != REQ_TYPE_BLOCK_PC) {
+	if (req->cmd_type != REQ_TYPE_FS) {
 		blk_dump_rq_flags(req, "MS unsupported request");
 		return BLKPREP_KILL;
 	}
diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c
index 0fb27d3..586fb8d 100644
--- a/drivers/memstick/core/mspro_block.c
+++ b/drivers/memstick/core/mspro_block.c
@@ -829,8 +829,7 @@ static void mspro_block_start(struct memstick_dev *card)
 
 static int mspro_block_prepare_req(struct request_queue *q, struct request *req)
 {
-	if (req->cmd_type != REQ_TYPE_FS &&
-	    req->cmd_type != REQ_TYPE_BLOCK_PC) {
+	if (req->cmd_type != REQ_TYPE_FS) {
 		blk_dump_rq_flags(req, "MSPro unsupported request");
 		return BLKPREP_KILL;
 	}
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 2/7] virtio_blk: use blk_rq_map_kern
  2016-06-13 15:21 a few passthrough request improvements Christoph Hellwig
  2016-06-13 15:21 ` [PATCH 1/7] memstick: don't allow REQ_TYPE_BLOCK_PC requests Christoph Hellwig
@ 2016-06-13 15:21 ` Christoph Hellwig
  2016-06-13 15:21 ` [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized Christoph Hellwig
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-13 15:21 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, bhalevy, nab, linux-block

Similar to how SCSI and NVMe prepare passthrough requests.  This avoids
poking into request internals too much.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/virtio_blk.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 18e4069..963a130 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -236,25 +236,23 @@ static int virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
 static int virtblk_get_id(struct gendisk *disk, char *id_str)
 {
 	struct virtio_blk *vblk = disk->private_data;
+	struct request_queue *q = vblk->disk->queue;
 	struct request *req;
-	struct bio *bio;
 	int err;
 
-	bio = bio_map_kern(vblk->disk->queue, id_str, VIRTIO_BLK_ID_BYTES,
-			   GFP_KERNEL);
-	if (IS_ERR(bio))
-		return PTR_ERR(bio);
-
-	req = blk_make_request(vblk->disk->queue, bio, GFP_KERNEL);
-	if (IS_ERR(req)) {
-		bio_put(bio);
+	req = blk_get_request(q, READ, GFP_KERNEL);
+	if (IS_ERR(req))
 		return PTR_ERR(req);
-	}
-
+	blk_rq_set_block_pc(req);
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
+
+	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
+	if (err)
+		goto out;
+
 	err = blk_execute_rq(vblk->disk->queue, vblk->disk, req, false);
+out:
 	blk_put_request(req);
-
 	return err;
 }
 
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-13 15:21 a few passthrough request improvements Christoph Hellwig
  2016-06-13 15:21 ` [PATCH 1/7] memstick: don't allow REQ_TYPE_BLOCK_PC requests Christoph Hellwig
  2016-06-13 15:21 ` [PATCH 2/7] virtio_blk: use blk_rq_map_kern Christoph Hellwig
@ 2016-06-13 15:21 ` Christoph Hellwig
  2016-06-14  2:17   ` Jens Axboe
  2016-06-15  9:57   ` Boaz Harrosh
  2016-06-13 15:21 ` [PATCH 4/7] block: simplify and export blk_rq_append_bio Christoph Hellwig
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-13 15:21 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, bhalevy, nab, linux-block

blk_get_request is used for BLOCK_PC and similar pass through requests.
Currently we always need to call blk_rq_set_block_pc or an opencoded
version of it to allow appending bios using the request mapping helpers
later on, which is a somewhat awkward API.  Instead move the
initialization part of blk_rq_set_block_pc into blk_get_request, so that
we always have a safe to use request.  blk_rq_set_block_pc now goes away
in favor of just setting cmd_type to REQ_TYPE_BLOCK_PC, or nothing in case
it was overriden with a different type a little later (or earlier in case
of the SCSI OSD code..)

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c                            | 27 +++++++++------------------
 block/blk-mq.c                              |  5 +++++
 block/bsg.c                                 |  2 +-
 block/scsi_ioctl.c                          |  6 +++---
 drivers/block/pktcdvd.c                     |  2 +-
 drivers/block/virtio_blk.c                  |  1 -
 drivers/cdrom/cdrom.c                       |  2 +-
 drivers/nvme/host/core.c                    |  4 ----
 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            |  3 +--
 drivers/scsi/osst.c                         |  2 +-
 drivers/scsi/scsi_error.c                   |  2 +-
 drivers/scsi/scsi_lib.c                     |  2 +-
 drivers/scsi/sg.c                           |  2 +-
 drivers/scsi/st.c                           |  2 +-
 drivers/target/target_core_pscsi.c          |  2 +-
 fs/nfsd/blocklayout.c                       |  2 +-
 include/linux/blkdev.h                      |  1 -
 20 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index c94c7ad..2b179cd 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1294,10 +1294,16 @@ static struct request *blk_old_get_request(struct request_queue *q, int rw,
 
 	spin_lock_irq(q->queue_lock);
 	rq = get_request(q, rw, 0, NULL, gfp_mask);
-	if (IS_ERR(rq))
+	if (IS_ERR(rq)) {
 		spin_unlock_irq(q->queue_lock);
-	/* q->queue_lock is unlocked at this point */
+		return rq;
+	}
 
+	/* q->queue_lock is unlocked at this point */
+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
+	memset(rq->__cmd, 0, sizeof(rq->__cmd));
 	return rq;
 }
 
@@ -1351,7 +1357,7 @@ struct request *blk_make_request(struct request_queue *q, struct bio *bio,
 	if (IS_ERR(rq))
 		return rq;
 
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	for_each_bio(bio) {
 		struct bio *bounce_bio = bio;
@@ -1370,21 +1376,6 @@ struct request *blk_make_request(struct request_queue *q, struct bio *bio,
 EXPORT_SYMBOL(blk_make_request);
 
 /**
- * blk_rq_set_block_pc - initialize a request to type BLOCK_PC
- * @rq:		request to be initialized
- *
- */
-void blk_rq_set_block_pc(struct request *rq)
-{
-	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-	rq->__data_len = 0;
-	rq->__sector = (sector_t) -1;
-	rq->bio = rq->biotail = NULL;
-	memset(rq->__cmd, 0, sizeof(rq->__cmd));
-}
-EXPORT_SYMBOL(blk_rq_set_block_pc);
-
-/**
  * blk_requeue_request - put a request back on queue
  * @q:		request queue where request should be inserted
  * @rq:		request to be inserted
diff --git a/block/blk-mq.c b/block/blk-mq.c
index bc7166d..80dce26 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -263,6 +263,11 @@ struct request *blk_mq_alloc_request(struct request_queue *q, int rw,
 		blk_queue_exit(q);
 		return ERR_PTR(-EWOULDBLOCK);
 	}
+
+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
+	memset(rq->__cmd, 0, sizeof(rq->__cmd));
 	return rq;
 }
 EXPORT_SYMBOL(blk_mq_alloc_request);
diff --git a/block/bsg.c b/block/bsg.c
index d214e92..c0199d7 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -236,7 +236,7 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 	rq = blk_get_request(q, rw, GFP_KERNEL);
 	if (IS_ERR(rq))
 		return rq;
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	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 0774799..3750ddb 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -318,7 +318,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
 	rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);
 	if (IS_ERR(rq))
 		return PTR_ERR(rq);
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	if (hdr->cmd_len > BLK_MAX_CDB) {
 		rq->cmd = kzalloc(hdr->cmd_len, GFP_KERNEL);
@@ -449,7 +449,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
 		err = PTR_ERR(rq);
 		goto error_free_buffer;
 	}
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	cmdlen = COMMAND_SIZE(opcode);
 
@@ -539,7 +539,7 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	rq = blk_get_request(q, WRITE, __GFP_RECLAIM);
 	if (IS_ERR(rq))
 		return PTR_ERR(rq);
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
 	rq->cmd[0] = cmd;
 	rq->cmd[4] = data;
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 9393bc7..66ad416 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -707,7 +707,7 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
 			     WRITE : READ, __GFP_RECLAIM);
 	if (IS_ERR(rq))
 		return PTR_ERR(rq);
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	if (cgc->buflen) {
 		ret = blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen,
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 963a130..0be7dd6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -243,7 +243,6 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 	req = blk_get_request(q, READ, GFP_KERNEL);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
-	blk_rq_set_block_pc(req);
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
 
 	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 1b257ea..6d20659 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2185,7 +2185,7 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
 			ret = PTR_ERR(rq);
 			break;
 		}
-		blk_rq_set_block_pc(rq);
+		rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 		ret = blk_rq_map_user(q, rq, NULL, ubuf, len, GFP_KERNEL);
 		if (ret) {
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9d7cee4..cb8ca1b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -201,10 +201,6 @@ struct request *nvme_alloc_request(struct request_queue *q,
 
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
 	req->cmd_flags |= REQ_FAILFAST_DRIVER;
-	req->__data_len = 0;
-	req->__sector = (sector_t) -1;
-	req->bio = req->biotail = NULL;
-
 	req->cmd = (unsigned char *)cmd;
 	req->cmd_len = sizeof(struct nvme_command);
 
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 375d818..f6ebf6f 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -277,7 +277,7 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 		return NULL;
 	}
 
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->cmd_len = COMMAND_SIZE(cmd);
 	rq->cmd[0] = cmd;
 
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 9406d5f..4ae6c15 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -113,7 +113,7 @@ retry:
 	if (IS_ERR(req))
 		return SCSI_DH_RES_TEMP_UNAVAIL;
 
-	blk_rq_set_block_pc(req);
+	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);
@@ -243,7 +243,7 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h)
 	if (IS_ERR(req))
 		return SCSI_DH_RES_TEMP_UNAVAIL;
 
-	blk_rq_set_block_pc(req);
+	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);
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 06fbd0b..1b84b4a 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -275,7 +275,7 @@ static struct request *get_rdac_req(struct scsi_device *sdev,
 				"get_rdac_req: blk_get_request failed.\n");
 		return NULL;
 	}
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	if (buflen && blk_rq_map_kern(q, rq, buffer, buflen, GFP_NOIO)) {
 		blk_put_request(rq);
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index daa4dc1..868ae29d 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1567,7 +1567,7 @@ static struct request *_make_request(struct request_queue *q, bool has_write,
 		if (IS_ERR(req))
 			return req;
 
-		blk_rq_set_block_pc(req);
+		req->cmd_type = REQ_TYPE_BLOCK_PC;
 		return req;
 	}
 }
@@ -1605,7 +1605,6 @@ static int _init_blk_request(struct osd_request *or,
 				ret = PTR_ERR(req);
 				goto out;
 			}
-			blk_rq_set_block_pc(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 5033223..6816089 100644
--- a/drivers/scsi/osst.c
+++ b/drivers/scsi/osst.c
@@ -367,7 +367,7 @@ static int osst_execute(struct osst_request *SRpnt, const unsigned char *cmd,
 	if (IS_ERR(req))
 		return DRIVER_ERROR << 24;
 
-	blk_rq_set_block_pc(req);
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
 	req->cmd_flags |= REQ_QUIET;
 
 	SRpnt->bio = NULL;
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a8b610e..b6b6d92 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1978,7 +1978,7 @@ static void scsi_eh_lock_door(struct scsi_device *sdev)
 	if (IS_ERR(req))
 		return;
 
-	blk_rq_set_block_pc(req);
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	req->cmd[0] = ALLOW_MEDIUM_REMOVAL;
 	req->cmd[1] = 0;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c71344a..292f713 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -191,7 +191,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 	req = blk_get_request(sdev->request_queue, write, __GFP_RECLAIM);
 	if (IS_ERR(req))
 		return ret;
-	blk_rq_set_block_pc(req);
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	if (bufflen &&	blk_rq_map_kern(sdev->request_queue, req,
 					buffer, bufflen, __GFP_RECLAIM))
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index ae7d9bd..398c04e 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1708,7 +1708,7 @@ sg_start_req(Sg_request *srp, unsigned char *cmd)
 		return PTR_ERR(rq);
 	}
 
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	if (hp->cmd_len > BLK_MAX_CDB)
 		rq->cmd = long_cmdp;
diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
index 7af5226..8179ee3 100644
--- a/drivers/scsi/st.c
+++ b/drivers/scsi/st.c
@@ -545,7 +545,7 @@ static int st_scsi_execute(struct st_request *SRpnt, const unsigned char *cmd,
 	if (IS_ERR(req))
 		return DRIVER_ERROR << 24;
 
-	blk_rq_set_block_pc(req);
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
 	req->cmd_flags |= REQ_QUIET;
 
 	mdata->null_mapped = 1;
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 81564c8..1f30179 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -1022,7 +1022,7 @@ pscsi_execute_cmd(struct se_cmd *cmd)
 			goto fail;
 		}
 
-		blk_rq_set_block_pc(req);
+		req->cmd_type = REQ_TYPE_BLOCK_PC;
 	} else {
 		BUG_ON(!cmd->data_length);
 
diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c
index e55b524..cdadcf8 100644
--- a/fs/nfsd/blocklayout.c
+++ b/fs/nfsd/blocklayout.c
@@ -224,7 +224,7 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev,
 		error = -ENOMEM;
 		goto out_free_buf;
 	}
-	blk_rq_set_block_pc(rq);
+	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	error = blk_rq_map_kern(q, rq, buf, bufflen, GFP_KERNEL);
 	if (error)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 9d1e0a4..472b527 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -789,7 +789,6 @@ extern void __blk_put_request(struct request_queue *, struct request *);
 extern struct request *blk_get_request(struct request_queue *, int, gfp_t);
 extern struct request *blk_make_request(struct request_queue *, struct bio *,
 					gfp_t);
-extern void blk_rq_set_block_pc(struct request *);
 extern void blk_requeue_request(struct request_queue *, struct request *);
 extern void blk_add_request_payload(struct request *rq, struct page *page,
 		int offset, unsigned int len);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 4/7] block: simplify and export blk_rq_append_bio
  2016-06-13 15:21 a few passthrough request improvements Christoph Hellwig
                   ` (2 preceding siblings ...)
  2016-06-13 15:21 ` [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized Christoph Hellwig
@ 2016-06-13 15:21 ` Christoph Hellwig
  2016-06-13 15:21 ` [PATCH 5/7] target: stop using blk_make_request Christoph Hellwig
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-13 15:21 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, bhalevy, nab, linux-block

The target SCSI passthrough backend is much better served with the low-level
blk_rq_append_bio construct then the helpers built on top of it, so export it.

Also use the opportunity to remove the pointless request_queue argument and
make the code flow a little more readable.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c       |  2 +-
 block/blk-map.c        | 25 +++++++++++++++----------
 block/blk.h            |  2 --
 include/linux/blkdev.h |  1 +
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2b179cd..ceefa48 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1364,7 +1364,7 @@ struct request *blk_make_request(struct request_queue *q, struct bio *bio,
 		int ret;
 
 		blk_queue_bounce(q, &bounce_bio);
-		ret = blk_rq_append_bio(q, rq, bounce_bio);
+		ret = blk_rq_append_bio(rq, bounce_bio);
 		if (unlikely(ret)) {
 			blk_put_request(rq);
 			return ERR_PTR(ret);
diff --git a/block/blk-map.c b/block/blk-map.c
index 61733a6..b8657fa 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -9,21 +9,26 @@
 
 #include "blk.h"
 
-int blk_rq_append_bio(struct request_queue *q, struct request *rq,
-		      struct bio *bio)
+/*
+ * Append a bio to a passthrough request.  Only works can be merged into
+ * the request based on the driver constraints.
+ */
+int blk_rq_append_bio(struct request *rq, struct bio *bio)
 {
-	if (!rq->bio)
-		blk_rq_bio_prep(q, rq, bio);
-	else if (!ll_back_merge_fn(q, rq, bio))
-		return -EINVAL;
-	else {
+	if (!rq->bio) {
+		blk_rq_bio_prep(rq->q, rq, bio);
+	} else {
+		if (!ll_back_merge_fn(rq->q, rq, bio))
+			return -EINVAL;
+
 		rq->biotail->bi_next = bio;
 		rq->biotail = bio;
-
 		rq->__data_len += bio->bi_iter.bi_size;
 	}
+
 	return 0;
 }
+EXPORT_SYMBOL(blk_rq_append_bio);
 
 static int __blk_rq_unmap_user(struct bio *bio)
 {
@@ -71,7 +76,7 @@ static int __blk_rq_map_user_iov(struct request *rq,
 	 */
 	bio_get(bio);
 
-	ret = blk_rq_append_bio(q, rq, bio);
+	ret = blk_rq_append_bio(rq, bio);
 	if (ret) {
 		bio_endio(bio);
 		__blk_rq_unmap_user(orig_bio);
@@ -229,7 +234,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 	if (do_copy)
 		rq->cmd_flags |= REQ_COPY_USER;
 
-	ret = blk_rq_append_bio(q, rq, bio);
+	ret = blk_rq_append_bio(rq, bio);
 	if (unlikely(ret)) {
 		/* request is too big */
 		bio_put(bio);
diff --git a/block/blk.h b/block/blk.h
index 70e4aee..c37492f 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -64,8 +64,6 @@ void blk_exit_rl(struct request_list *rl);
 void init_request_from_bio(struct request *req, struct bio *bio);
 void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 			struct bio *bio);
-int blk_rq_append_bio(struct request_queue *q, struct request *rq,
-		      struct bio *bio);
 void blk_queue_bypass_start(struct request_queue *q);
 void blk_queue_bypass_end(struct request_queue *q);
 void blk_dequeue_request(struct request *rq);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 472b527..998fbe0 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -800,6 +800,7 @@ extern int blk_rq_prep_clone(struct request *rq, struct request *rq_src,
 extern void blk_rq_unprep_clone(struct request *rq);
 extern int blk_insert_cloned_request(struct request_queue *q,
 				     struct request *rq);
+extern int blk_rq_append_bio(struct request *rq, struct bio *bio);
 extern void blk_delay_queue(struct request_queue *, unsigned long);
 extern void blk_queue_split(struct request_queue *, struct bio **,
 			    struct bio_set *);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 5/7] target: stop using blk_make_request
  2016-06-13 15:21 a few passthrough request improvements Christoph Hellwig
                   ` (3 preceding siblings ...)
  2016-06-13 15:21 ` [PATCH 4/7] block: simplify and export blk_rq_append_bio Christoph Hellwig
@ 2016-06-13 15:21 ` Christoph Hellwig
  2016-06-13 15:21 ` [PATCH 6/7] scsi/osd: open code blk_make_request Christoph Hellwig
  2016-06-13 15:21 ` [PATCH 7/7] block: unexport various bio mapping helpers Christoph Hellwig
  6 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-13 15:21 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, bhalevy, nab, linux-block

Using blk_rq_append_bio allows to append the bios to the request
directly instead of having to build up a list first, and also
allows to have a single code path for requests with or without
data attached to them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/target/target_core_pscsi.c | 87 +++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 53 deletions(-)

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 1f30179..93da16d 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -876,19 +876,19 @@ static inline struct bio *pscsi_get_bio(int nr_vecs)
 
 static sense_reason_t
 pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
-		enum dma_data_direction data_direction, struct bio **hbio)
+		struct request *req)
 {
 	struct pscsi_dev_virt *pdv = PSCSI_DEV(cmd->se_dev);
-	struct bio *bio = NULL, *tbio = NULL;
+	struct bio *bio = NULL;
 	struct page *page;
 	struct scatterlist *sg;
 	u32 data_len = cmd->data_length, i, len, bytes, off;
 	int nr_pages = (cmd->data_length + sgl[0].offset +
 			PAGE_SIZE - 1) >> PAGE_SHIFT;
 	int nr_vecs = 0, rc;
-	int rw = (data_direction == DMA_TO_DEVICE);
+	int rw = (cmd->data_direction == DMA_TO_DEVICE);
 
-	*hbio = NULL;
+	BUG_ON(!cmd->data_length);
 
 	pr_debug("PSCSI: nr_pages: %d\n", nr_pages);
 
@@ -927,16 +927,6 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 				pr_debug("PSCSI: Allocated bio: %p,"
 					" dir: %s nr_vecs: %d\n", bio,
 					(rw) ? "rw" : "r", nr_vecs);
-				/*
-				 * Set *hbio pointer to handle the case:
-				 * nr_pages > BIO_MAX_PAGES, where additional
-				 * bios need to be added to complete a given
-				 * command.
-				 */
-				if (!*hbio)
-					*hbio = tbio = bio;
-				else
-					tbio = tbio->bi_next = bio;
 			}
 
 			pr_debug("PSCSI: Calling bio_add_pc_page() i: %d"
@@ -955,11 +945,16 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 				pr_debug("PSCSI: Reached bio->bi_vcnt max:"
 					" %d i: %d bio: %p, allocating another"
 					" bio\n", bio->bi_vcnt, i, bio);
+
+				rc = blk_rq_append_bio(req, bio);
+				if (rc) {
+					pr_err("pSCSI: failed to append bio\n");
+					goto fail;
+				}
+
 				/*
 				 * Clear the pointer so that another bio will
-				 * be allocated with pscsi_get_bio() above, the
-				 * current bio has already been set *tbio and
-				 * bio->bi_next.
+				 * be allocated with pscsi_get_bio() above.
 				 */
 				bio = NULL;
 			}
@@ -968,13 +963,16 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 		}
 	}
 
+	if (bio) {
+		rc = blk_rq_append_bio(req, bio);
+		if (rc) {
+			pr_err("pSCSI: failed to append bio\n");
+			goto fail;
+		}
+	}
+
 	return 0;
 fail:
-	while (*hbio) {
-		bio = *hbio;
-		*hbio = (*hbio)->bi_next;
-		bio_endio(bio);
-	}
 	return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 }
 
@@ -992,11 +990,9 @@ pscsi_execute_cmd(struct se_cmd *cmd)
 {
 	struct scatterlist *sgl = cmd->t_data_sg;
 	u32 sgl_nents = cmd->t_data_nents;
-	enum dma_data_direction data_direction = cmd->data_direction;
 	struct pscsi_dev_virt *pdv = PSCSI_DEV(cmd->se_dev);
 	struct pscsi_plugin_task *pt;
 	struct request *req;
-	struct bio *hbio;
 	sense_reason_t ret;
 
 	/*
@@ -1012,31 +1008,21 @@ pscsi_execute_cmd(struct se_cmd *cmd)
 	memcpy(pt->pscsi_cdb, cmd->t_task_cdb,
 		scsi_command_size(cmd->t_task_cdb));
 
-	if (!sgl) {
-		req = blk_get_request(pdv->pdv_sd->request_queue,
-				(data_direction == DMA_TO_DEVICE),
-				GFP_KERNEL);
-		if (IS_ERR(req)) {
-			pr_err("PSCSI: blk_get_request() failed\n");
-			ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-			goto fail;
-		}
+	req = blk_get_request(pdv->pdv_sd->request_queue,
+			(cmd->data_direction == DMA_TO_DEVICE),
+			GFP_KERNEL);
+	if (IS_ERR(req)) {
+		pr_err("PSCSI: blk_get_request() failed\n");
+		ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+		goto fail;
+	}
 
-		req->cmd_type = REQ_TYPE_BLOCK_PC;
-	} else {
-		BUG_ON(!cmd->data_length);
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
 
-		ret = pscsi_map_sg(cmd, sgl, sgl_nents, data_direction, &hbio);
+	if (sgl) {
+		ret = pscsi_map_sg(cmd, sgl, sgl_nents, req);
 		if (ret)
-			goto fail;
-
-		req = blk_make_request(pdv->pdv_sd->request_queue, hbio,
-				       GFP_KERNEL);
-		if (IS_ERR(req)) {
-			pr_err("pSCSI: blk_make_request() failed\n");
-			ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-			goto fail_free_bio;
-		}
+			goto fail_put_request;
 	}
 
 	req->end_io = pscsi_req_done;
@@ -1057,13 +1043,8 @@ pscsi_execute_cmd(struct se_cmd *cmd)
 
 	return 0;
 
-fail_free_bio:
-	while (hbio) {
-		struct bio *bio = hbio;
-		hbio = hbio->bi_next;
-		bio_endio(bio);
-	}
-	ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+fail_put_request:
+	blk_put_request(req);
 fail:
 	kfree(pt);
 	return ret;
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 6/7] scsi/osd: open code blk_make_request
  2016-06-13 15:21 a few passthrough request improvements Christoph Hellwig
                   ` (4 preceding siblings ...)
  2016-06-13 15:21 ` [PATCH 5/7] target: stop using blk_make_request Christoph Hellwig
@ 2016-06-13 15:21 ` Christoph Hellwig
  2016-06-15 10:42   ` Boaz Harrosh
  2016-06-13 15:21 ` [PATCH 7/7] block: unexport various bio mapping helpers Christoph Hellwig
  6 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-13 15:21 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, bhalevy, nab, linux-block

I wish the OSD code could simply use blk_rq_map_* helpers like
everyone else, but the complex nature of deciding if we have
DATA IN and/or DATA OUT buffers might make this impossible
(at least for a mere human like me).

But using blk_rq_append_bio at least allows sharing the setup code
between request with or without dat a buffers, and given that this
is the last user of blk_make_request it allows getting rid of that
somewhat awkward interface.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c                 | 57 ----------------------------------------
 drivers/scsi/osd/osd_initiator.c | 25 +++++++++++-------
 include/linux/blkdev.h           |  2 --
 3 files changed, 16 insertions(+), 68 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index ceefa48..22b72ab 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1319,63 +1319,6 @@ struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
 EXPORT_SYMBOL(blk_get_request);
 
 /**
- * blk_make_request - given a bio, allocate a corresponding struct request.
- * @q: target request queue
- * @bio:  The bio describing the memory mappings that will be submitted for IO.
- *        It may be a chained-bio properly constructed by block/bio layer.
- * @gfp_mask: gfp flags to be used for memory allocation
- *
- * blk_make_request is the parallel of generic_make_request for BLOCK_PC
- * type commands. Where the struct request needs to be farther initialized by
- * the caller. It is passed a &struct bio, which describes the memory info of
- * the I/O transfer.
- *
- * The caller of blk_make_request must make sure that bi_io_vec
- * are set to describe the memory buffers. That bio_data_dir() will return
- * the needed direction of the request. (And all bio's in the passed bio-chain
- * are properly set accordingly)
- *
- * If called under none-sleepable conditions, mapped bio buffers must not
- * need bouncing, by calling the appropriate masked or flagged allocator,
- * suitable for the target device. Otherwise the call to blk_queue_bounce will
- * BUG.
- *
- * WARNING: When allocating/cloning a bio-chain, careful consideration should be
- * given to how you allocate bios. In particular, you cannot use
- * __GFP_DIRECT_RECLAIM for anything but the first bio in the chain. Otherwise
- * you risk waiting for IO completion of a bio that hasn't been submitted yet,
- * thus resulting in a deadlock. Alternatively bios should be allocated using
- * bio_kmalloc() instead of bio_alloc(), as that avoids the mempool deadlock.
- * If possible a big IO should be split into smaller parts when allocation
- * fails. Partial allocation should not be an error, or you risk a live-lock.
- */
-struct request *blk_make_request(struct request_queue *q, struct bio *bio,
-				 gfp_t gfp_mask)
-{
-	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
-
-	if (IS_ERR(rq))
-		return rq;
-
-	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-
-	for_each_bio(bio) {
-		struct bio *bounce_bio = bio;
-		int ret;
-
-		blk_queue_bounce(q, &bounce_bio);
-		ret = blk_rq_append_bio(rq, bounce_bio);
-		if (unlikely(ret)) {
-			blk_put_request(rq);
-			return ERR_PTR(ret);
-		}
-	}
-
-	return rq;
-}
-EXPORT_SYMBOL(blk_make_request);
-
-/**
  * blk_requeue_request - put a request back on queue
  * @q:		request queue where request should be inserted
  * @rq:		request to be inserted
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 868ae29d..95f456f 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1558,18 +1558,25 @@ static int _osd_req_finalize_data_integrity(struct osd_request *or,
 static struct request *_make_request(struct request_queue *q, bool has_write,
 			      struct _osd_io_info *oii, gfp_t flags)
 {
-	if (oii->bio)
-		return blk_make_request(q, oii->bio, flags);
-	else {
-		struct request *req;
-
-		req = blk_get_request(q, has_write ? WRITE : READ, flags);
-		if (IS_ERR(req))
-			return req;
+	struct request *req;
+	struct bio *bio = oii->bio;
+	int ret;
 
-		req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req = blk_get_request(q, has_write ? WRITE : READ, flags);
+	if (IS_ERR(req))
 		return req;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+
+	for_each_bio(bio) {
+		struct bio *bounce_bio = bio;
+
+		blk_queue_bounce(req->q, &bounce_bio);
+		ret = blk_rq_append_bio(req, bounce_bio);
+		if (ret)
+			return ERR_PTR(ret);
 	}
+
+	return req;
 }
 
 static int _init_blk_request(struct osd_request *or,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 998fbe0..8a984a7 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -787,8 +787,6 @@ extern void blk_rq_init(struct request_queue *q, struct request *rq);
 extern void blk_put_request(struct request *);
 extern void __blk_put_request(struct request_queue *, struct request *);
 extern struct request *blk_get_request(struct request_queue *, int, gfp_t);
-extern struct request *blk_make_request(struct request_queue *, struct bio *,
-					gfp_t);
 extern void blk_requeue_request(struct request_queue *, struct request *);
 extern void blk_add_request_payload(struct request *rq, struct page *page,
 		int offset, unsigned int len);
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 7/7] block: unexport various bio mapping helpers
  2016-06-13 15:21 a few passthrough request improvements Christoph Hellwig
                   ` (5 preceding siblings ...)
  2016-06-13 15:21 ` [PATCH 6/7] scsi/osd: open code blk_make_request Christoph Hellwig
@ 2016-06-13 15:21 ` Christoph Hellwig
  6 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-13 15:21 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, bhalevy, nab, linux-block

They are unused and potential new users really should use the
blk_rq_map* versions.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/bio.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 848cd35..b88ed1c 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1097,7 +1097,6 @@ int bio_uncopy_user(struct bio *bio)
 	bio_put(bio);
 	return ret;
 }
-EXPORT_SYMBOL(bio_uncopy_user);
 
 /**
  *	bio_copy_user_iov	-	copy user data to bio
@@ -1392,7 +1391,6 @@ void bio_unmap_user(struct bio *bio)
 	__bio_unmap_user(bio);
 	bio_put(bio);
 }
-EXPORT_SYMBOL(bio_unmap_user);
 
 static void bio_map_kern_endio(struct bio *bio)
 {
@@ -1538,7 +1536,6 @@ cleanup:
 	bio_put(bio);
 	return ERR_PTR(-ENOMEM);
 }
-EXPORT_SYMBOL(bio_copy_kern);
 
 /*
  * bio_set_pages_dirty() and bio_check_pages_dirty() are support functions
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-13 15:21 ` [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized Christoph Hellwig
@ 2016-06-14  2:17   ` Jens Axboe
  2016-06-14 13:43     ` Christoph Hellwig
  2016-06-15  9:57   ` Boaz Harrosh
  1 sibling, 1 reply; 21+ messages in thread
From: Jens Axboe @ 2016-06-14  2:17 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: mst, ooo, bhalevy, nab, linux-block

On 06/13/2016 09:21 AM, Christoph Hellwig wrote:
> blk_get_request is used for BLOCK_PC and similar pass through requests.
> Currently we always need to call blk_rq_set_block_pc or an opencoded
> version of it to allow appending bios using the request mapping helpers
> later on, which is a somewhat awkward API.  Instead move the
> initialization part of blk_rq_set_block_pc into blk_get_request, so that
> we always have a safe to use request.  blk_rq_set_block_pc now goes away
> in favor of just setting cmd_type to REQ_TYPE_BLOCK_PC, or nothing in case
> it was overriden with a different type a little later (or earlier in case
> of the SCSI OSD code..)

It may be awkward, but we have those to avoid doing things like this:

+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
+	memset(rq->__cmd, 0, sizeof(rq->__cmd));

for every request we allocate, when we don't use ->cmd at all. Honest, 
I'd rather have

struct request *blk_get_pc_request();

and similar helpers around this, so we don't have to do extra 
initialization when we don't need it.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-14  2:17   ` Jens Axboe
@ 2016-06-14 13:43     ` Christoph Hellwig
  2016-06-14 15:04       ` Jens Axboe
  0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-14 13:43 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Christoph Hellwig, mst, ooo, bhalevy, nab, linux-block

On Mon, Jun 13, 2016 at 08:17:38PM -0600, Jens Axboe wrote:
> It may be awkward, but we have those to avoid doing things like this:
>
> +	rq->__data_len = 0;
> +	rq->__sector = (sector_t) -1;
> +	rq->bio = rq->biotail = NULL;
> +	memset(rq->__cmd, 0, sizeof(rq->__cmd));
>
> for every request we allocate, when we don't use ->cmd at all. Honest, I'd 
> rather have
>
> struct request *blk_get_pc_request();
>
> and similar helpers around this, so we don't have to do extra 
> initialization when we don't need it.

I'm working on resurrecting my patch to remove rq->cmd and friends from
the common request structure, but the full patch is something I'd rather
do after the initial NVMe over Fabrics merge.  If you're fine with the
rest of the series I'll respin it to keep blk_set_block_pc for now which
will only do the zeroing and setting cmd_type.

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-14 13:43     ` Christoph Hellwig
@ 2016-06-14 15:04       ` Jens Axboe
  0 siblings, 0 replies; 21+ messages in thread
From: Jens Axboe @ 2016-06-14 15:04 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: mst, ooo, bhalevy, nab, linux-block

On 06/14/2016 07:43 AM, Christoph Hellwig wrote:
> On Mon, Jun 13, 2016 at 08:17:38PM -0600, Jens Axboe wrote:
>> It may be awkward, but we have those to avoid doing things like this:
>>
>> +	rq->__data_len = 0;
>> +	rq->__sector = (sector_t) -1;
>> +	rq->bio = rq->biotail = NULL;
>> +	memset(rq->__cmd, 0, sizeof(rq->__cmd));
>>
>> for every request we allocate, when we don't use ->cmd at all. Honest, I'd
>> rather have
>>
>> struct request *blk_get_pc_request();
>>
>> and similar helpers around this, so we don't have to do extra
>> initialization when we don't need it.
>
> I'm working on resurrecting my patch to remove rq->cmd and friends from
> the common request structure,

That'd be great! And then we can unify the request setup after that.

> but the full patch is something I'd rather
> do after the initial NVMe over Fabrics merge.  If you're fine with the
> rest of the series I'll respin it to keep blk_set_block_pc for now which
> will only do the zeroing and setting cmd_type.

Yes that sounds good, the rest looks fine to me.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-13 15:21 ` [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized Christoph Hellwig
  2016-06-14  2:17   ` Jens Axboe
@ 2016-06-15  9:57   ` Boaz Harrosh
  1 sibling, 0 replies; 21+ messages in thread
From: Boaz Harrosh @ 2016-06-15  9:57 UTC (permalink / raw)
  To: Christoph Hellwig, axboe; +Cc: mst, bhalevy, nab, linux-block

On 06/13/2016 06:21 PM, Christoph Hellwig wrote:
> blk_get_request is used for BLOCK_PC and similar pass through requests.
> Currently we always need to call blk_rq_set_block_pc or an opencoded
> version of it to allow appending bios using the request mapping helpers
> later on, which is a somewhat awkward API.  Instead move the
> initialization part of blk_rq_set_block_pc into blk_get_request, so that
> we always have a safe to use request.  blk_rq_set_block_pc now goes away
> in favor of just setting cmd_type to REQ_TYPE_BLOCK_PC, or nothing in case
> it was overriden with a different type a little later (or earlier in case
> of the SCSI OSD code..)
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Is good thanks.
Review-by: Boaz harrosh <ooo@electrozaur.com>

<>
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index daa4dc1..868ae29d 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -1567,7 +1567,7 @@ static struct request *_make_request(struct request_queue *q, bool has_write,
>  		if (IS_ERR(req))
>  			return req;
>  
> -		blk_rq_set_block_pc(req);
> +		req->cmd_type = REQ_TYPE_BLOCK_PC;
>  		return req;
>  	}
>  }
> @@ -1605,7 +1605,6 @@ static int _init_blk_request(struct osd_request *or,
>  				ret = PTR_ERR(req);
>  				goto out;
>  			}
> -			blk_rq_set_block_pc(req);
>  			or->in.req = or->request->next_rq = req;

Yes this is the bidi part of the request it need not be of any type.
Only properly initialized.

>  		}
>  	} else if (has_in)
<>

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 6/7] scsi/osd: open code blk_make_request
  2016-06-13 15:21 ` [PATCH 6/7] scsi/osd: open code blk_make_request Christoph Hellwig
@ 2016-06-15 10:42   ` Boaz Harrosh
  2016-06-15 10:52     ` Boaz Harrosh
  0 siblings, 1 reply; 21+ messages in thread
From: Boaz Harrosh @ 2016-06-15 10:42 UTC (permalink / raw)
  To: Christoph Hellwig, axboe; +Cc: mst, bhalevy, nab, linux-block

On 06/13/2016 06:21 PM, Christoph Hellwig wrote:
> I wish the OSD code could simply use blk_rq_map_* helpers like
> everyone else, but the complex nature of deciding if we have
> DATA IN and/or DATA OUT buffers might make this impossible
> (at least for a mere human like me).
> 
> But using blk_rq_append_bio at least allows sharing the setup code
> between request with or without dat a buffers, and given that this
> is the last user of blk_make_request it allows getting rid of that
> somewhat awkward interface.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I tested this and it works. But I have some comments. I think there
might be a bug in the original code

ACK-by: Boaz Harrosh <ooo@electrozaur.com>

> ---
>  block/blk-core.c                 | 57 ----------------------------------------
>  drivers/scsi/osd/osd_initiator.c | 25 +++++++++++-------
>  include/linux/blkdev.h           |  2 --
>  3 files changed, 16 insertions(+), 68 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index ceefa48..22b72ab 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1319,63 +1319,6 @@ struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
>  EXPORT_SYMBOL(blk_get_request);
>  
>  /**
> - * blk_make_request - given a bio, allocate a corresponding struct request.
> - * @q: target request queue
> - * @bio:  The bio describing the memory mappings that will be submitted for IO.
> - *        It may be a chained-bio properly constructed by block/bio layer.
> - * @gfp_mask: gfp flags to be used for memory allocation
> - *
> - * blk_make_request is the parallel of generic_make_request for BLOCK_PC
> - * type commands. Where the struct request needs to be farther initialized by
> - * the caller. It is passed a &struct bio, which describes the memory info of
> - * the I/O transfer.
> - *
> - * The caller of blk_make_request must make sure that bi_io_vec
> - * are set to describe the memory buffers. That bio_data_dir() will return
> - * the needed direction of the request. (And all bio's in the passed bio-chain
> - * are properly set accordingly)
> - *
> - * If called under none-sleepable conditions, mapped bio buffers must not
> - * need bouncing, by calling the appropriate masked or flagged allocator,
> - * suitable for the target device. Otherwise the call to blk_queue_bounce will
> - * BUG.
> - *
> - * WARNING: When allocating/cloning a bio-chain, careful consideration should be
> - * given to how you allocate bios. In particular, you cannot use
> - * __GFP_DIRECT_RECLAIM for anything but the first bio in the chain. Otherwise
> - * you risk waiting for IO completion of a bio that hasn't been submitted yet,
> - * thus resulting in a deadlock. Alternatively bios should be allocated using
> - * bio_kmalloc() instead of bio_alloc(), as that avoids the mempool deadlock.
> - * If possible a big IO should be split into smaller parts when allocation
> - * fails. Partial allocation should not be an error, or you risk a live-lock.
> - */
> -struct request *blk_make_request(struct request_queue *q, struct bio *bio,
> -				 gfp_t gfp_mask)
> -{
> -	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
> -
> -	if (IS_ERR(rq))
> -		return rq;
> -
> -	rq->cmd_type = REQ_TYPE_BLOCK_PC;
> -
> -	for_each_bio(bio) {
> -		struct bio *bounce_bio = bio;
> -		int ret;
> -
> -		blk_queue_bounce(q, &bounce_bio);
> -		ret = blk_rq_append_bio(rq, bounce_bio);
> -		if (unlikely(ret)) {
> -			blk_put_request(rq);
> -			return ERR_PTR(ret);
> -		}
> -	}
> -
> -	return rq;
> -}
> -EXPORT_SYMBOL(blk_make_request);
> -
> -/**
>   * blk_requeue_request - put a request back on queue
>   * @q:		request queue where request should be inserted
>   * @rq:		request to be inserted
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index 868ae29d..95f456f 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -1558,18 +1558,25 @@ static int _osd_req_finalize_data_integrity(struct osd_request *or,
>  static struct request *_make_request(struct request_queue *q, bool has_write,
>  			      struct _osd_io_info *oii, gfp_t flags)
>  {
> -	if (oii->bio)
> -		return blk_make_request(q, oii->bio, flags);
> -	else {
> -		struct request *req;
> -
> -		req = blk_get_request(q, has_write ? WRITE : READ, flags);
> -		if (IS_ERR(req))
> -			return req;
> +	struct request *req;
> +	struct bio *bio = oii->bio;
> +	int ret;
>  
> -		req->cmd_type = REQ_TYPE_BLOCK_PC;
> +	req = blk_get_request(q, has_write ? WRITE : READ, flags);
> +	if (IS_ERR(req))
>  		return req;
> +	req->cmd_type = REQ_TYPE_BLOCK_PC;
> +
> +	for_each_bio(bio) {
> +		struct bio *bounce_bio = bio;
> +
> +		blk_queue_bounce(req->q, &bounce_bio);
> +		ret = blk_rq_append_bio(req, bounce_bio);

So you know how blk_rq_append_bio() puts the new bio on tail->bi_next
and then for_each_bio() goes to look for bio->bi_next. This is just crap
code that sets the bi_next pointers to what they were before.

But in the case where blk_queue_bounce() decides to return a new cloned
bio, we will get a short list and not hang all BIOs on the request list.

BUT since this is an old BUG not introduced here I don't care.
(See ACK above)

The only case where this can hit with current bidi supporting devices
is in 32bit & highmem. But I suspect last I tested many years ago that
32bit is not supported because of some other bugs.

Open coding for_each_bio() or introducing for_each_bio_safe would fix
this:
	while(bio) {
		struct bio *bounce_bio = bio;

		blk_queue_bounce(req->q, &bounce_bio);
		ret = blk_rq_append_bio(req, bounce_bio);
		if (ret)
			return ERR_PTR(ret);

		bio = bio->bi_next;
	}

> +		if (ret)
> +			return ERR_PTR(ret);
>  	}
> +
> +	return req;
>  }
>  

Thanks
Boaz

>  static int _init_blk_request(struct osd_request *or,
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 998fbe0..8a984a7 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -787,8 +787,6 @@ extern void blk_rq_init(struct request_queue *q, struct request *rq);
>  extern void blk_put_request(struct request *);
>  extern void __blk_put_request(struct request_queue *, struct request *);
>  extern struct request *blk_get_request(struct request_queue *, int, gfp_t);
> -extern struct request *blk_make_request(struct request_queue *, struct bio *,
> -					gfp_t);
>  extern void blk_requeue_request(struct request_queue *, struct request *);
>  extern void blk_add_request_payload(struct request *rq, struct page *page,
>  		int offset, unsigned int len);
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 6/7] scsi/osd: open code blk_make_request
  2016-06-15 10:42   ` Boaz Harrosh
@ 2016-06-15 10:52     ` Boaz Harrosh
  0 siblings, 0 replies; 21+ messages in thread
From: Boaz Harrosh @ 2016-06-15 10:52 UTC (permalink / raw)
  To: Christoph Hellwig, axboe; +Cc: mst, bhalevy, nab, linux-block

On 06/15/2016 01:42 PM, Boaz Harrosh wrote:
> On 06/13/2016 06:21 PM, Christoph Hellwig wrote:
>> I wish the OSD code could simply use blk_rq_map_* helpers like
>> everyone else, but the complex nature of deciding if we have
>> DATA IN and/or DATA OUT buffers might make this impossible
>> (at least for a mere human like me).
>>
>> But using blk_rq_append_bio at least allows sharing the setup code
>> between request with or without dat a buffers, and given that this
>> is the last user of blk_make_request it allows getting rid of that
>> somewhat awkward interface.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> I tested this and it works. But I have some comments. I think there
> might be a bug in the original code
> 
> ACK-by: Boaz Harrosh <ooo@electrozaur.com>
> 
>> ---
>>  block/blk-core.c                 | 57 ----------------------------------------
>>  drivers/scsi/osd/osd_initiator.c | 25 +++++++++++-------
>>  include/linux/blkdev.h           |  2 --
>>  3 files changed, 16 insertions(+), 68 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index ceefa48..22b72ab 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1319,63 +1319,6 @@ struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
>>  EXPORT_SYMBOL(blk_get_request);
>>  
>>  /**
>> - * blk_make_request - given a bio, allocate a corresponding struct request.
>> - * @q: target request queue
>> - * @bio:  The bio describing the memory mappings that will be submitted for IO.
>> - *        It may be a chained-bio properly constructed by block/bio layer.
>> - * @gfp_mask: gfp flags to be used for memory allocation
>> - *
>> - * blk_make_request is the parallel of generic_make_request for BLOCK_PC
>> - * type commands. Where the struct request needs to be farther initialized by
>> - * the caller. It is passed a &struct bio, which describes the memory info of
>> - * the I/O transfer.
>> - *
>> - * The caller of blk_make_request must make sure that bi_io_vec
>> - * are set to describe the memory buffers. That bio_data_dir() will return
>> - * the needed direction of the request. (And all bio's in the passed bio-chain
>> - * are properly set accordingly)
>> - *
>> - * If called under none-sleepable conditions, mapped bio buffers must not
>> - * need bouncing, by calling the appropriate masked or flagged allocator,
>> - * suitable for the target device. Otherwise the call to blk_queue_bounce will
>> - * BUG.
>> - *
>> - * WARNING: When allocating/cloning a bio-chain, careful consideration should be
>> - * given to how you allocate bios. In particular, you cannot use
>> - * __GFP_DIRECT_RECLAIM for anything but the first bio in the chain. Otherwise
>> - * you risk waiting for IO completion of a bio that hasn't been submitted yet,
>> - * thus resulting in a deadlock. Alternatively bios should be allocated using
>> - * bio_kmalloc() instead of bio_alloc(), as that avoids the mempool deadlock.
>> - * If possible a big IO should be split into smaller parts when allocation
>> - * fails. Partial allocation should not be an error, or you risk a live-lock.
>> - */
>> -struct request *blk_make_request(struct request_queue *q, struct bio *bio,
>> -				 gfp_t gfp_mask)
>> -{
>> -	struct request *rq = blk_get_request(q, bio_data_dir(bio), gfp_mask);
>> -
>> -	if (IS_ERR(rq))
>> -		return rq;
>> -
>> -	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>> -
>> -	for_each_bio(bio) {
>> -		struct bio *bounce_bio = bio;
>> -		int ret;
>> -
>> -		blk_queue_bounce(q, &bounce_bio);
>> -		ret = blk_rq_append_bio(rq, bounce_bio);
>> -		if (unlikely(ret)) {
>> -			blk_put_request(rq);
>> -			return ERR_PTR(ret);
>> -		}
>> -	}
>> -
>> -	return rq;
>> -}
>> -EXPORT_SYMBOL(blk_make_request);
>> -
>> -/**
>>   * blk_requeue_request - put a request back on queue
>>   * @q:		request queue where request should be inserted
>>   * @rq:		request to be inserted
>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
>> index 868ae29d..95f456f 100644
>> --- a/drivers/scsi/osd/osd_initiator.c
>> +++ b/drivers/scsi/osd/osd_initiator.c
>> @@ -1558,18 +1558,25 @@ static int _osd_req_finalize_data_integrity(struct osd_request *or,
>>  static struct request *_make_request(struct request_queue *q, bool has_write,
>>  			      struct _osd_io_info *oii, gfp_t flags)
>>  {
>> -	if (oii->bio)
>> -		return blk_make_request(q, oii->bio, flags);
>> -	else {
>> -		struct request *req;
>> -
>> -		req = blk_get_request(q, has_write ? WRITE : READ, flags);
>> -		if (IS_ERR(req))
>> -			return req;
>> +	struct request *req;
>> +	struct bio *bio = oii->bio;
>> +	int ret;
>>  
>> -		req->cmd_type = REQ_TYPE_BLOCK_PC;
>> +	req = blk_get_request(q, has_write ? WRITE : READ, flags);
>> +	if (IS_ERR(req))
>>  		return req;
>> +	req->cmd_type = REQ_TYPE_BLOCK_PC;
>> +
>> +	for_each_bio(bio) {
>> +		struct bio *bounce_bio = bio;
>> +
>> +		blk_queue_bounce(req->q, &bounce_bio);
>> +		ret = blk_rq_append_bio(req, bounce_bio);
> 
> So you know how blk_rq_append_bio() puts the new bio on tail->bi_next
> and then for_each_bio() goes to look for bio->bi_next. This is just crap
> code that sets the bi_next pointers to what they were before.
> 
> But in the case where blk_queue_bounce() decides to return a new cloned
> bio, we will get a short list and not hang all BIOs on the request list.
> 
> BUT since this is an old BUG not introduced here I don't care.
> (See ACK above)
> 
> The only case where this can hit with current bidi supporting devices
> is in 32bit & highmem. But I suspect last I tested many years ago that
> 32bit is not supported because of some other bugs.
> 
> Open coding for_each_bio() or introducing for_each_bio_safe would fix
> this:
> 	while(bio) {
> 		struct bio *bounce_bio = bio;
> 
> 		blk_queue_bounce(req->q, &bounce_bio);
> 		ret = blk_rq_append_bio(req, bounce_bio);
> 		if (ret)
> 			return ERR_PTR(ret);
> 
> 		bio = bio->bi_next;

OK sorry is exactly the same. No emails before coffee right?

Sorry for the noise it works just fine
Thanks - Boaz

> 	}
> 
>> +		if (ret)
>> +			return ERR_PTR(ret);
>>  	}
>> +
>> +	return req;
>>  }
>>  
> 
> Thanks
> Boaz
> 
>>  static int _init_blk_request(struct osd_request *or,
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 998fbe0..8a984a7 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -787,8 +787,6 @@ extern void blk_rq_init(struct request_queue *q, struct request *rq);
>>  extern void blk_put_request(struct request *);
>>  extern void __blk_put_request(struct request_queue *, struct request *);
>>  extern struct request *blk_get_request(struct request_queue *, int, gfp_t);
>> -extern struct request *blk_make_request(struct request_queue *, struct bio *,
>> -					gfp_t);
>>  extern void blk_requeue_request(struct request_queue *, struct request *);
>>  extern void blk_add_request_payload(struct request *rq, struct page *page,
>>  		int offset, unsigned int len);
>>
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread
* [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-07-19  9:31 resend: passthrough request improvements V3 Christoph Hellwig
@ 2016-07-19  9:31 ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-07-19  9:31 UTC (permalink / raw)
  To: axboe; +Cc: linux-block

blk_get_request is used for BLOCK_PC and similar passthrough requests.
Currently we always need to call blk_rq_set_block_pc or an open coded
version of it to allow appending bios using the request mapping helpers
later on, which is a somewhat awkward API.  Instead move the
initialization part of blk_rq_set_block_pc into blk_get_request, so that
we always have a safe to use request.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c           | 12 +++++++-----
 block/blk-mq.c             |  4 ++++
 drivers/block/virtio_blk.c |  1 -
 drivers/nvme/host/core.c   |  4 ----
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index d66509f..3d22fa2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1294,10 +1294,15 @@ static struct request *blk_old_get_request(struct request_queue *q, int rw,
 
 	spin_lock_irq(q->queue_lock);
 	rq = get_request(q, rw, 0, NULL, gfp_mask);
-	if (IS_ERR(rq))
+	if (IS_ERR(rq)) {
 		spin_unlock_irq(q->queue_lock);
-	/* q->queue_lock is unlocked at this point */
+		return rq;
+	}
 
+	/* q->queue_lock is unlocked at this point */
+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
 	return rq;
 }
 
@@ -1377,9 +1382,6 @@ EXPORT_SYMBOL(blk_make_request);
 void blk_rq_set_block_pc(struct request *rq)
 {
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-	rq->__data_len = 0;
-	rq->__sector = (sector_t) -1;
-	rq->bio = rq->biotail = NULL;
 	memset(rq->__cmd, 0, sizeof(rq->__cmd));
 }
 EXPORT_SYMBOL(blk_rq_set_block_pc);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 5e0fe60..3a52b69 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -263,6 +263,10 @@ struct request *blk_mq_alloc_request(struct request_queue *q, int rw,
 		blk_queue_exit(q);
 		return ERR_PTR(-EWOULDBLOCK);
 	}
+
+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
 	return rq;
 }
 EXPORT_SYMBOL(blk_mq_alloc_request);
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index a85a143..1523e05 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -243,7 +243,6 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 	req = blk_get_request(q, READ, GFP_KERNEL);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
-	blk_rq_set_block_pc(req);
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
 
 	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cf4b750..9f0ec3b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -222,10 +222,6 @@ struct request *nvme_alloc_request(struct request_queue *q,
 
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
 	req->cmd_flags |= REQ_FAILFAST_DRIVER;
-	req->__data_len = 0;
-	req->__sector = (sector_t) -1;
-	req->bio = req->biotail = NULL;
-
 	req->cmd = (unsigned char *)cmd;
 	req->cmd_len = sizeof(struct nvme_command);
 
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-16  9:14 passthrough request improvements V3 Christoph Hellwig
@ 2016-06-16  9:14 ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-16  9:14 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, nab, linux-block

blk_get_request is used for BLOCK_PC and similar passthrough requests.
Currently we always need to call blk_rq_set_block_pc or an open coded
version of it to allow appending bios using the request mapping helpers
later on, which is a somewhat awkward API.  Instead move the
initialization part of blk_rq_set_block_pc into blk_get_request, so that
we always have a safe to use request.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c           | 12 +++++++-----
 block/blk-mq.c             |  4 ++++
 drivers/block/virtio_blk.c |  1 -
 drivers/nvme/host/core.c   |  4 ----
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index c94c7ad..8416c4a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1294,10 +1294,15 @@ static struct request *blk_old_get_request(struct request_queue *q, int rw,
 
 	spin_lock_irq(q->queue_lock);
 	rq = get_request(q, rw, 0, NULL, gfp_mask);
-	if (IS_ERR(rq))
+	if (IS_ERR(rq)) {
 		spin_unlock_irq(q->queue_lock);
-	/* q->queue_lock is unlocked at this point */
+		return rq;
+	}
 
+	/* q->queue_lock is unlocked at this point */
+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
 	return rq;
 }
 
@@ -1377,9 +1382,6 @@ EXPORT_SYMBOL(blk_make_request);
 void blk_rq_set_block_pc(struct request *rq)
 {
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-	rq->__data_len = 0;
-	rq->__sector = (sector_t) -1;
-	rq->bio = rq->biotail = NULL;
 	memset(rq->__cmd, 0, sizeof(rq->__cmd));
 }
 EXPORT_SYMBOL(blk_rq_set_block_pc);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index bc7166d..b5606dc 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -263,6 +263,10 @@ struct request *blk_mq_alloc_request(struct request_queue *q, int rw,
 		blk_queue_exit(q);
 		return ERR_PTR(-EWOULDBLOCK);
 	}
+
+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
 	return rq;
 }
 EXPORT_SYMBOL(blk_mq_alloc_request);
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 963a130..0be7dd6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -243,7 +243,6 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 	req = blk_get_request(q, READ, GFP_KERNEL);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
-	blk_rq_set_block_pc(req);
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
 
 	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9d7cee4..cb8ca1b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -201,10 +201,6 @@ struct request *nvme_alloc_request(struct request_queue *q,
 
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
 	req->cmd_flags |= REQ_FAILFAST_DRIVER;
-	req->__data_len = 0;
-	req->__sector = (sector_t) -1;
-	req->bio = req->biotail = NULL;
-
 	req->cmd = (unsigned char *)cmd;
 	req->cmd_len = sizeof(struct nvme_command);
 
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-15 10:17       ` Jens Axboe
@ 2016-06-15 10:25         ` Christoph Hellwig
  0 siblings, 0 replies; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-15 10:25 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Christoph Hellwig, mst, ooo, nab, linux-block

On Wed, Jun 15, 2016 at 12:17:47PM +0200, Jens Axboe wrote:
> I guess you are right, the fs path jumps in via __blk_mq_alloc_request(), 
> so not fs fast path. But it'd still be nice to properly fix this. Can it 
> wait until the rq->pc mess is resolved?

I'll resend the series removing the memset in the blk_get_request path,
which I think is very useful for now as the requests currently returned
from blk_get_request / blk_mq_alloc_request are inherently dangerous.
req->cmd zeroing is really just a workaround broken devices in practice,
as spec complicant scsi device never should look at the cmd array
beyond the command length.

I'll try to expedite the pc_request split, but I'd really like to get
all the NVMe over Fabrics work in first, as we'd be in merge hell otherwise.

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-15 10:07     ` Christoph Hellwig
@ 2016-06-15 10:17       ` Jens Axboe
  2016-06-15 10:25         ` Christoph Hellwig
  0 siblings, 1 reply; 21+ messages in thread
From: Jens Axboe @ 2016-06-15 10:17 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: mst, ooo, nab, linux-block

On 06/15/2016 12:07 PM, Christoph Hellwig wrote:
> On Wed, Jun 15, 2016 at 11:02:18AM +0200, Jens Axboe wrote:
>> On 06/14/2016 07:16 PM, Christoph Hellwig wrote:
>>> blk_get_request is used for BLOCK_PC and similar passthrough requests.
>>> Currently we always need to call blk_rq_set_block_pc or an open coded
>>> version of it to allow appending bios using the request mapping helpers
>>> later on, which is a somewhat awkward API.  Instead move the
>>> initialization part of blk_rq_set_block_pc into blk_get_request, so that
>>> we always have a safe to use request.
>>
>> This still puts the pc related memset() into the normal fast path...
>
> Oops, I missed removing it from the alloc path when adding back
> blk_rq_set_block_pc, will fix it up.
>
> But I have to object to this actually being a fast path - it's not
> used for any fs request, just BLOCK_PC, NVMe passthrough and various
> driver specific little hacks.

I guess you are right, the fs path jumps in via 
__blk_mq_alloc_request(), so not fs fast path. But it'd still be nice to 
properly fix this. Can it wait until the rq->pc mess is resolved?

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-15  9:02   ` Jens Axboe
@ 2016-06-15 10:07     ` Christoph Hellwig
  2016-06-15 10:17       ` Jens Axboe
  0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-15 10:07 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Christoph Hellwig, mst, ooo, nab, linux-block

On Wed, Jun 15, 2016 at 11:02:18AM +0200, Jens Axboe wrote:
> On 06/14/2016 07:16 PM, Christoph Hellwig wrote:
>> blk_get_request is used for BLOCK_PC and similar passthrough requests.
>> Currently we always need to call blk_rq_set_block_pc or an open coded
>> version of it to allow appending bios using the request mapping helpers
>> later on, which is a somewhat awkward API.  Instead move the
>> initialization part of blk_rq_set_block_pc into blk_get_request, so that
>> we always have a safe to use request.
>
> This still puts the pc related memset() into the normal fast path...

Oops, I missed removing it from the alloc path when adding back
blk_rq_set_block_pc, will fix it up.

But I have to object to this actually being a fast path - it's not
used for any fs request, just BLOCK_PC, NVMe passthrough and various
driver specific little hacks.

^ permalink raw reply	[flat|nested] 21+ messages in thread
* Re: [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-14 17:16 ` [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized Christoph Hellwig
@ 2016-06-15  9:02   ` Jens Axboe
  2016-06-15 10:07     ` Christoph Hellwig
  0 siblings, 1 reply; 21+ messages in thread
From: Jens Axboe @ 2016-06-15  9:02 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: mst, ooo, nab, linux-block

On 06/14/2016 07:16 PM, Christoph Hellwig wrote:
> blk_get_request is used for BLOCK_PC and similar passthrough requests.
> Currently we always need to call blk_rq_set_block_pc or an open coded
> version of it to allow appending bios using the request mapping helpers
> later on, which is a somewhat awkward API.  Instead move the
> initialization part of blk_rq_set_block_pc into blk_get_request, so that
> we always have a safe to use request.

This still puts the pc related memset() into the normal fast path...

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 21+ messages in thread
* [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized
  2016-06-14 17:15 a few passthrough request improvements V2 Christoph Hellwig
@ 2016-06-14 17:16 ` Christoph Hellwig
  2016-06-15  9:02   ` Jens Axboe
  0 siblings, 1 reply; 21+ messages in thread
From: Christoph Hellwig @ 2016-06-14 17:16 UTC (permalink / raw)
  To: axboe; +Cc: mst, ooo, nab, linux-block

blk_get_request is used for BLOCK_PC and similar passthrough requests.
Currently we always need to call blk_rq_set_block_pc or an open coded
version of it to allow appending bios using the request mapping helpers
later on, which is a somewhat awkward API.  Instead move the
initialization part of blk_rq_set_block_pc into blk_get_request, so that
we always have a safe to use request.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-core.c           | 13 ++++++++-----
 block/blk-mq.c             |  5 +++++
 drivers/block/virtio_blk.c |  1 -
 drivers/nvme/host/core.c   |  4 ----
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index c94c7ad..37852ec 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1294,10 +1294,16 @@ static struct request *blk_old_get_request(struct request_queue *q, int rw,
 
 	spin_lock_irq(q->queue_lock);
 	rq = get_request(q, rw, 0, NULL, gfp_mask);
-	if (IS_ERR(rq))
+	if (IS_ERR(rq)) {
 		spin_unlock_irq(q->queue_lock);
-	/* q->queue_lock is unlocked at this point */
+		return rq;
+	}
 
+	/* q->queue_lock is unlocked at this point */
+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
+	memset(rq->__cmd, 0, sizeof(rq->__cmd));
 	return rq;
 }
 
@@ -1377,9 +1383,6 @@ EXPORT_SYMBOL(blk_make_request);
 void blk_rq_set_block_pc(struct request *rq)
 {
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-	rq->__data_len = 0;
-	rq->__sector = (sector_t) -1;
-	rq->bio = rq->biotail = NULL;
 	memset(rq->__cmd, 0, sizeof(rq->__cmd));
 }
 EXPORT_SYMBOL(blk_rq_set_block_pc);
diff --git a/block/blk-mq.c b/block/blk-mq.c
index bc7166d..80dce26 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -263,6 +263,11 @@ struct request *blk_mq_alloc_request(struct request_queue *q, int rw,
 		blk_queue_exit(q);
 		return ERR_PTR(-EWOULDBLOCK);
 	}
+
+	rq->__data_len = 0;
+	rq->__sector = (sector_t) -1;
+	rq->bio = rq->biotail = NULL;
+	memset(rq->__cmd, 0, sizeof(rq->__cmd));
 	return rq;
 }
 EXPORT_SYMBOL(blk_mq_alloc_request);
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 963a130..0be7dd6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -243,7 +243,6 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 	req = blk_get_request(q, READ, GFP_KERNEL);
 	if (IS_ERR(req))
 		return PTR_ERR(req);
-	blk_rq_set_block_pc(req);
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
 
 	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9d7cee4..cb8ca1b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -201,10 +201,6 @@ struct request *nvme_alloc_request(struct request_queue *q,
 
 	req->cmd_type = REQ_TYPE_DRV_PRIV;
 	req->cmd_flags |= REQ_FAILFAST_DRIVER;
-	req->__data_len = 0;
-	req->__sector = (sector_t) -1;
-	req->bio = req->biotail = NULL;
-
 	req->cmd = (unsigned char *)cmd;
 	req->cmd_len = sizeof(struct nvme_command);
 
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-07-19  9:31 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-13 15:21 a few passthrough request improvements Christoph Hellwig
2016-06-13 15:21 ` [PATCH 1/7] memstick: don't allow REQ_TYPE_BLOCK_PC requests Christoph Hellwig
2016-06-13 15:21 ` [PATCH 2/7] virtio_blk: use blk_rq_map_kern Christoph Hellwig
2016-06-13 15:21 ` [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized Christoph Hellwig
2016-06-14  2:17   ` Jens Axboe
2016-06-14 13:43     ` Christoph Hellwig
2016-06-14 15:04       ` Jens Axboe
2016-06-15  9:57   ` Boaz Harrosh
2016-06-13 15:21 ` [PATCH 4/7] block: simplify and export blk_rq_append_bio Christoph Hellwig
2016-06-13 15:21 ` [PATCH 5/7] target: stop using blk_make_request Christoph Hellwig
2016-06-13 15:21 ` [PATCH 6/7] scsi/osd: open code blk_make_request Christoph Hellwig
2016-06-15 10:42   ` Boaz Harrosh
2016-06-15 10:52     ` Boaz Harrosh
2016-06-13 15:21 ` [PATCH 7/7] block: unexport various bio mapping helpers Christoph Hellwig
2016-06-14 17:15 a few passthrough request improvements V2 Christoph Hellwig
2016-06-14 17:16 ` [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized Christoph Hellwig
2016-06-15  9:02   ` Jens Axboe
2016-06-15 10:07     ` Christoph Hellwig
2016-06-15 10:17       ` Jens Axboe
2016-06-15 10:25         ` Christoph Hellwig
2016-06-16  9:14 passthrough request improvements V3 Christoph Hellwig
2016-06-16  9:14 ` [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized Christoph Hellwig
2016-07-19  9:31 resend: passthrough request improvements V3 Christoph Hellwig
2016-07-19  9:31 ` [PATCH 3/7] block: ensure bios return from blk_get_request are properly initialized Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.