* [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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ 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; 14+ 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] 14+ messages in thread