* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command @ 2014-03-31 10:45 Christoph Hellwig 0 siblings, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2014-03-31 10:45 UTC (permalink / raw) To: Mike Christie; +Cc: James Bottomley, linux-scsi > The above call would free the cmnd->cmnd and set it to null. If then > scsi_io_completion was going to do some error processing it looks like > it could try to access the scsi_cmnd->cmnd field. > > With the current code that would not be a problem because the blk unprep > callback is not called until the block layer does its request cleanup in > blk_finish_request which as you know is after > scsi_io_completion/scsi_end_request is done with the cmnd. This incremental patches fixes the issue, and makes sure the uninit calls are nicely paired like the rest of the I/O completion routines after patch 2: diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 48c5c77..8e79612 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -490,8 +490,6 @@ static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd) struct request *req = cmd->request; unsigned long flags; - scsi_uninit_command(cmd); - spin_lock_irqsave(q->queue_lock, flags); blk_unprep_request(req); req->special = NULL; @@ -941,6 +939,7 @@ requeue: /* Unprep the request and put it back at the head of the queue. * A new command will be prepared and issued. */ + scsi_uninit_command(cmd); scsi_release_buffers(cmd); scsi_requeue_command(q, cmd); break; @@ -956,6 +955,7 @@ requeue: return; next_command: + scsi_uninit_command(cmd); scsi_release_buffers(cmd); scsi_next_command(cmd); } diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index d95c4fd..d99cb3f 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -1652,8 +1652,6 @@ static int sd_done(struct scsi_cmnd *SCpnt) unsigned char op = SCpnt->cmnd[0]; unsigned char unmap = SCpnt->cmnd[1] & 8; - sd_uninit_command(SCpnt); - if (req->cmd_flags & REQ_DISCARD || req->cmd_flags & REQ_WRITE_SAME) { if (!result) { good_bytes = blk_rq_bytes(req); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* scsi midlayer updates for 3.15 (resend) @ 2014-05-01 14:51 Christoph Hellwig 2014-05-01 14:51 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2014-05-01 14:51 UTC (permalink / raw) To: James Bottomley; +Cc: linux-scsi Various patches from the scsi multiqueue development that make sense on their own. Changes since the version from 5 weeks ago: - rebase on top of minor changes in 3.15-rc3 - add a struct request forward declaration to avoid a warning in the OSD ULD. - add the reviewed-by tags from Nic and Mike from the last round. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] scsi: reintroduce scsi_driver.init_command 2014-05-01 14:51 scsi midlayer updates for 3.15 (resend) Christoph Hellwig @ 2014-05-01 14:51 ` Christoph Hellwig 2014-05-08 7:11 ` Hannes Reinecke 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2014-05-01 14:51 UTC (permalink / raw) To: James Bottomley; +Cc: linux-scsi Instead of letting the ULD play games with the prep_fn move back to the model of a central prep_fn with a callback to the ULD. This already cleans up and shortens the code by itself, and will be required to properly support blk-mq in the SCSI midlayer. Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Nicholas Bellinger <nab@linux-iscsi.org> Reviewed-by: Mike Christie <michaelc@cs.wisc.edu> --- drivers/scsi/scsi_lib.c | 66 ++++++++++++++++++++++++++------------------ drivers/scsi/sd.c | 44 ++++++++++------------------- drivers/scsi/sr.c | 19 ++++--------- include/scsi/scsi_driver.h | 9 ++---- 4 files changed, 62 insertions(+), 76 deletions(-) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 045822b..9f841df 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1073,15 +1073,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev, int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) { - struct scsi_cmnd *cmd; - int ret = scsi_prep_state_check(sdev, req); - - if (ret != BLKPREP_OK) - return ret; - - cmd = scsi_get_cmd_from_req(sdev, req); - if (unlikely(!cmd)) - return BLKPREP_DEFER; + struct scsi_cmnd *cmd = req->special; /* * BLOCK_PC requests may transfer data, in which case they must @@ -1125,15 +1117,11 @@ EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd); */ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req) { - struct scsi_cmnd *cmd; - int ret = scsi_prep_state_check(sdev, req); - - if (ret != BLKPREP_OK) - return ret; + struct scsi_cmnd *cmd = req->special; if (unlikely(sdev->scsi_dh_data && sdev->scsi_dh_data->scsi_dh && sdev->scsi_dh_data->scsi_dh->prep_fn)) { - ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req); + int ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req); if (ret != BLKPREP_OK) return ret; } @@ -1143,16 +1131,13 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req) */ BUG_ON(!req->nr_phys_segments); - cmd = scsi_get_cmd_from_req(sdev, req); - if (unlikely(!cmd)) - return BLKPREP_DEFER; - memset(cmd->cmnd, 0, BLK_MAX_CDB); return scsi_init_io(cmd, GFP_ATOMIC); } EXPORT_SYMBOL(scsi_setup_fs_cmnd); -int scsi_prep_state_check(struct scsi_device *sdev, struct request *req) +static int +scsi_prep_state_check(struct scsi_device *sdev, struct request *req) { int ret = BLKPREP_OK; @@ -1204,9 +1189,9 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req) } return ret; } -EXPORT_SYMBOL(scsi_prep_state_check); -int scsi_prep_return(struct request_queue *q, struct request *req, int ret) +static int +scsi_prep_return(struct request_queue *q, struct request *req, int ret) { struct scsi_device *sdev = q->queuedata; @@ -1237,18 +1222,44 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret) return ret; } -EXPORT_SYMBOL(scsi_prep_return); -int scsi_prep_fn(struct request_queue *q, struct request *req) +static int scsi_prep_fn(struct request_queue *q, struct request *req) { struct scsi_device *sdev = q->queuedata; - int ret = BLKPREP_KILL; + struct scsi_cmnd *cmd; + int ret; - if (req->cmd_type == REQ_TYPE_BLOCK_PC) + ret = scsi_prep_state_check(sdev, req); + if (ret != BLKPREP_OK) + goto out; + + cmd = scsi_get_cmd_from_req(sdev, req); + if (unlikely(!cmd)) { + ret = BLKPREP_DEFER; + goto out; + } + + if (req->cmd_type == REQ_TYPE_FS) + ret = scsi_cmd_to_driver(cmd)->init_command(cmd); + else if (req->cmd_type == REQ_TYPE_BLOCK_PC) ret = scsi_setup_blk_pc_cmnd(sdev, req); + else + ret = BLKPREP_KILL; + +out: return scsi_prep_return(q, req, ret); } -EXPORT_SYMBOL(scsi_prep_fn); + +static void scsi_unprep_fn(struct request_queue *q, struct request *req) +{ + if (req->cmd_type == REQ_TYPE_FS) { + struct scsi_cmnd *cmd = req->special; + struct scsi_driver *drv = scsi_cmd_to_driver(cmd); + + if (drv->uninit_command) + drv->uninit_command(cmd); + } +} /* * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else @@ -1669,6 +1680,7 @@ struct request_queue *scsi_alloc_queue(struct scsi_device *sdev) return NULL; blk_queue_prep_rq(q, scsi_prep_fn); + blk_queue_unprep_rq(q, scsi_unprep_fn); blk_queue_softirq_done(q, scsi_softirq_done); blk_queue_rq_timed_out(q, scsi_times_out); blk_queue_lld_busy(q, scsi_lld_busy); diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index efcbcd1..1c1253b 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -109,6 +109,8 @@ static int sd_suspend_system(struct device *); static int sd_suspend_runtime(struct device *); static int sd_resume(struct device *); static void sd_rescan(struct device *); +static int sd_init_command(struct scsi_cmnd *SCpnt); +static void sd_uninit_command(struct scsi_cmnd *SCpnt); static int sd_done(struct scsi_cmnd *); static int sd_eh_action(struct scsi_cmnd *, int); static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer); @@ -503,6 +505,8 @@ static struct scsi_driver sd_template = { .pm = &sd_pm_ops, }, .rescan = sd_rescan, + .init_command = sd_init_command, + .uninit_command = sd_uninit_command, .done = sd_done, .eh_action = sd_eh_action, }; @@ -838,9 +842,9 @@ static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq) return scsi_setup_blk_pc_cmnd(sdp, rq); } -static void sd_unprep_fn(struct request_queue *q, struct request *rq) +static void sd_uninit_command(struct scsi_cmnd *SCpnt) { - struct scsi_cmnd *SCpnt = rq->special; + struct request *rq = SCpnt->request; if (rq->cmd_flags & REQ_DISCARD) { free_page((unsigned long)rq->buffer); @@ -853,18 +857,10 @@ static void sd_unprep_fn(struct request_queue *q, struct request *rq) } } -/** - * sd_prep_fn - build a scsi (read or write) command from - * information in the request structure. - * @SCpnt: pointer to mid-level's per scsi command structure that - * contains request and into which the scsi command is written - * - * Returns 1 if successful and 0 if error (or cannot be done now). - **/ -static int sd_prep_fn(struct request_queue *q, struct request *rq) +static int sd_init_command(struct scsi_cmnd *SCpnt) { - struct scsi_cmnd *SCpnt; - struct scsi_device *sdp = q->queuedata; + struct request *rq = SCpnt->request; + struct scsi_device *sdp = SCpnt->device; struct gendisk *disk = rq->rq_disk; struct scsi_disk *sdkp; sector_t block = blk_rq_pos(rq); @@ -886,12 +882,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) } else if (rq->cmd_flags & REQ_FLUSH) { ret = scsi_setup_flush_cmnd(sdp, rq); goto out; - } else if (rq->cmd_type == REQ_TYPE_BLOCK_PC) { - ret = scsi_setup_blk_pc_cmnd(sdp, rq); - goto out; - } else if (rq->cmd_type != REQ_TYPE_FS) { - ret = BLKPREP_KILL; - goto out; } ret = scsi_setup_fs_cmnd(sdp, rq); if (ret != BLKPREP_OK) @@ -903,11 +893,10 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) * is used for a killable error condition */ ret = BLKPREP_KILL; - SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt, - "sd_prep_fn: block=%llu, " - "count=%d\n", - (unsigned long long)block, - this_count)); + SCSI_LOG_HLQUEUE(1, + scmd_printk(KERN_INFO, SCpnt, + "%s: block=%llu, count=%d\n", + __func__, (unsigned long long)block, this_count)); if (!sdp || !scsi_device_online(sdp) || block + blk_rq_sectors(rq) > get_capacity(disk)) { @@ -1127,7 +1116,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) */ ret = BLKPREP_OK; out: - return scsi_prep_return(q, rq, ret); + return ret; } /** @@ -2878,9 +2867,6 @@ static void sd_probe_async(void *data, async_cookie_t cookie) sd_revalidate_disk(gd); - blk_queue_prep_rq(sdp->request_queue, sd_prep_fn); - blk_queue_unprep_rq(sdp->request_queue, sd_unprep_fn); - gd->driverfs_dev = &sdp->sdev_gendev; gd->flags = GENHD_FL_EXT_DEVT; if (sdp->removable) { @@ -3028,8 +3014,6 @@ static int sd_remove(struct device *dev) async_synchronize_full_domain(&scsi_sd_pm_domain); async_synchronize_full_domain(&scsi_sd_probe_domain); - blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn); - blk_queue_unprep_rq(sdkp->device->request_queue, NULL); device_del(&sdkp->dev); del_gendisk(sdkp->disk); sd_shutdown(dev); diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c index 40d8592..93cbd36 100644 --- a/drivers/scsi/sr.c +++ b/drivers/scsi/sr.c @@ -79,6 +79,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_WORM); static DEFINE_MUTEX(sr_mutex); static int sr_probe(struct device *); static int sr_remove(struct device *); +static int sr_init_command(struct scsi_cmnd *SCpnt); static int sr_done(struct scsi_cmnd *); static int sr_runtime_suspend(struct device *dev); @@ -94,6 +95,7 @@ static struct scsi_driver sr_template = { .remove = sr_remove, .pm = &sr_pm_ops, }, + .init_command = sr_init_command, .done = sr_done, }; @@ -378,21 +380,14 @@ static int sr_done(struct scsi_cmnd *SCpnt) return good_bytes; } -static int sr_prep_fn(struct request_queue *q, struct request *rq) +static int sr_init_command(struct scsi_cmnd *SCpnt) { int block = 0, this_count, s_size; struct scsi_cd *cd; - struct scsi_cmnd *SCpnt; - struct scsi_device *sdp = q->queuedata; + struct request *rq = SCpnt->request; + struct scsi_device *sdp = SCpnt->device; int ret; - if (rq->cmd_type == REQ_TYPE_BLOCK_PC) { - ret = scsi_setup_blk_pc_cmnd(sdp, rq); - goto out; - } else if (rq->cmd_type != REQ_TYPE_FS) { - ret = BLKPREP_KILL; - goto out; - } ret = scsi_setup_fs_cmnd(sdp, rq); if (ret != BLKPREP_OK) goto out; @@ -517,7 +512,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq) */ ret = BLKPREP_OK; out: - return scsi_prep_return(q, rq, ret); + return ret; } static int sr_block_open(struct block_device *bdev, fmode_t mode) @@ -718,7 +713,6 @@ static int sr_probe(struct device *dev) /* FIXME: need to handle a get_capabilities failure properly ?? */ get_capabilities(cd); - blk_queue_prep_rq(sdev->request_queue, sr_prep_fn); sr_vendor_init(cd); disk->driverfs_dev = &sdev->sdev_gendev; @@ -993,7 +987,6 @@ static int sr_remove(struct device *dev) scsi_autopm_get_device(cd->device); - blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn); del_gendisk(cd->disk); mutex_lock(&sr_ref_mutex); diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h index 20fdfc2..36c4114 100644 --- a/include/scsi/scsi_driver.h +++ b/include/scsi/scsi_driver.h @@ -4,17 +4,17 @@ #include <linux/device.h> struct module; +struct request; struct scsi_cmnd; struct scsi_device; -struct request; -struct request_queue; - struct scsi_driver { struct module *owner; struct device_driver gendrv; void (*rescan)(struct device *); + int (*init_command)(struct scsi_cmnd *); + void (*uninit_command)(struct scsi_cmnd *); int (*done)(struct scsi_cmnd *); int (*eh_action)(struct scsi_cmnd *, int); }; @@ -31,8 +31,5 @@ extern int scsi_register_interface(struct class_interface *); int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req); int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req); -int scsi_prep_state_check(struct scsi_device *sdev, struct request *req); -int scsi_prep_return(struct request_queue *q, struct request *req, int ret); -int scsi_prep_fn(struct request_queue *, struct request *); #endif /* _SCSI_SCSI_DRIVER_H */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command 2014-05-01 14:51 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig @ 2014-05-08 7:11 ` Hannes Reinecke 0 siblings, 0 replies; 9+ messages in thread From: Hannes Reinecke @ 2014-05-08 7:11 UTC (permalink / raw) To: Christoph Hellwig, James Bottomley; +Cc: linux-scsi On 05/01/2014 04:51 PM, Christoph Hellwig wrote: > Instead of letting the ULD play games with the prep_fn move back to > the model of a central prep_fn with a callback to the ULD. This > already cleans up and shortens the code by itself, and will be required > to properly support blk-mq in the SCSI midlayer. > ... And leaving the device handler to still provide a prep_fn(). But that's for another day. > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Nicholas Bellinger <nab@linux-iscsi.org> > Reviewed-by: Mike Christie <michaelc@cs.wisc.edu> Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 9+ messages in thread
* misc scsi midlayer updates @ 2014-03-27 16:14 Christoph Hellwig 2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2014-03-27 16:14 UTC (permalink / raw) To: James Bottomley; +Cc: linux-scsi Various patches from the scsi multiqueue development that make sense on their own. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] scsi: reintroduce scsi_driver.init_command 2014-03-27 16:14 misc scsi midlayer updates Christoph Hellwig @ 2014-03-27 16:14 ` Christoph Hellwig 2014-03-31 5:45 ` Nicholas A. Bellinger 2014-03-31 6:56 ` Mike Christie 0 siblings, 2 replies; 9+ messages in thread From: Christoph Hellwig @ 2014-03-27 16:14 UTC (permalink / raw) To: James Bottomley; +Cc: linux-scsi Move control of the prep_fn back from the ULDs into the scsi layer. Besides cleaning up the code and removing the only use of the unprep_fn requeuest_queue method this also prepares for usinng blk-mq, which doesn't have equivalent functionality to the prep_fn method and requires the driver to provide just a single I/O submission method. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/scsi/scsi_lib.c | 66 ++++++++++++++++++++++++++------------------ drivers/scsi/sd.c | 46 +++++++++++------------------- drivers/scsi/sr.c | 19 ++++--------- include/scsi/scsi_driver.h | 8 ++---- 4 files changed, 63 insertions(+), 76 deletions(-) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index a73751b..48c5c77 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -456,6 +456,16 @@ void scsi_requeue_run_queue(struct work_struct *work) scsi_run_queue(q); } +static void scsi_uninit_command(struct scsi_cmnd *cmd) +{ + if (cmd->request->cmd_type == REQ_TYPE_FS) { + struct scsi_driver *drv = scsi_cmd_to_driver(cmd); + + if (drv->uninit_command) + drv->uninit_command(cmd); + } +} + /* * Function: scsi_requeue_command() * @@ -480,6 +490,8 @@ static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd) struct request *req = cmd->request; unsigned long flags; + scsi_uninit_command(cmd); + spin_lock_irqsave(q->queue_lock, flags); blk_unprep_request(req); req->special = NULL; @@ -1071,15 +1083,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev, int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) { - struct scsi_cmnd *cmd; - int ret = scsi_prep_state_check(sdev, req); - - if (ret != BLKPREP_OK) - return ret; - - cmd = scsi_get_cmd_from_req(sdev, req); - if (unlikely(!cmd)) - return BLKPREP_DEFER; + struct scsi_cmnd *cmd = req->special; /* * BLOCK_PC requests may transfer data, in which case they must @@ -1123,15 +1127,11 @@ EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd); */ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req) { - struct scsi_cmnd *cmd; - int ret = scsi_prep_state_check(sdev, req); - - if (ret != BLKPREP_OK) - return ret; + struct scsi_cmnd *cmd = req->special; if (unlikely(sdev->scsi_dh_data && sdev->scsi_dh_data->scsi_dh && sdev->scsi_dh_data->scsi_dh->prep_fn)) { - ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req); + int ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req); if (ret != BLKPREP_OK) return ret; } @@ -1141,16 +1141,13 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req) */ BUG_ON(!req->nr_phys_segments); - cmd = scsi_get_cmd_from_req(sdev, req); - if (unlikely(!cmd)) - return BLKPREP_DEFER; - memset(cmd->cmnd, 0, BLK_MAX_CDB); return scsi_init_io(cmd, GFP_ATOMIC); } EXPORT_SYMBOL(scsi_setup_fs_cmnd); -int scsi_prep_state_check(struct scsi_device *sdev, struct request *req) +static int +scsi_prep_state_check(struct scsi_device *sdev, struct request *req) { int ret = BLKPREP_OK; @@ -1202,9 +1199,9 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req) } return ret; } -EXPORT_SYMBOL(scsi_prep_state_check); -int scsi_prep_return(struct request_queue *q, struct request *req, int ret) +static int +scsi_prep_return(struct request_queue *q, struct request *req, int ret) { struct scsi_device *sdev = q->queuedata; @@ -1235,18 +1232,33 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret) return ret; } -EXPORT_SYMBOL(scsi_prep_return); -int scsi_prep_fn(struct request_queue *q, struct request *req) +static int scsi_prep_fn(struct request_queue *q, struct request *req) { struct scsi_device *sdev = q->queuedata; - int ret = BLKPREP_KILL; + struct scsi_cmnd *cmd; + int ret; - if (req->cmd_type == REQ_TYPE_BLOCK_PC) + ret = scsi_prep_state_check(sdev, req); + if (ret != BLKPREP_OK) + goto out; + + cmd = scsi_get_cmd_from_req(sdev, req); + if (unlikely(!cmd)) { + ret = BLKPREP_DEFER; + goto out; + } + + if (req->cmd_type == REQ_TYPE_FS) + ret = scsi_cmd_to_driver(cmd)->init_command(cmd); + else if (req->cmd_type == REQ_TYPE_BLOCK_PC) ret = scsi_setup_blk_pc_cmnd(sdev, req); + else + ret = BLKPREP_KILL; + +out: return scsi_prep_return(q, req, ret); } -EXPORT_SYMBOL(scsi_prep_fn); /* * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 89e6c04..d95c4fd 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -109,6 +109,8 @@ static int sd_suspend_system(struct device *); static int sd_suspend_runtime(struct device *); static int sd_resume(struct device *); static void sd_rescan(struct device *); +static int sd_init_command(struct scsi_cmnd *SCpnt); +static void sd_uninit_command(struct scsi_cmnd *SCpnt); static int sd_done(struct scsi_cmnd *); static int sd_eh_action(struct scsi_cmnd *, int); static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer); @@ -503,6 +505,8 @@ static struct scsi_driver sd_template = { .pm = &sd_pm_ops, }, .rescan = sd_rescan, + .init_command = sd_init_command, + .uninit_command = sd_uninit_command, .done = sd_done, .eh_action = sd_eh_action, }; @@ -838,9 +842,9 @@ static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq) return scsi_setup_blk_pc_cmnd(sdp, rq); } -static void sd_unprep_fn(struct request_queue *q, struct request *rq) +static void sd_uninit_command(struct scsi_cmnd *SCpnt) { - struct scsi_cmnd *SCpnt = rq->special; + struct request *rq = SCpnt->request; if (rq->cmd_flags & REQ_DISCARD) { free_page((unsigned long)rq->buffer); @@ -853,18 +857,10 @@ static void sd_unprep_fn(struct request_queue *q, struct request *rq) } } -/** - * sd_prep_fn - build a scsi (read or write) command from - * information in the request structure. - * @SCpnt: pointer to mid-level's per scsi command structure that - * contains request and into which the scsi command is written - * - * Returns 1 if successful and 0 if error (or cannot be done now). - **/ -static int sd_prep_fn(struct request_queue *q, struct request *rq) +static int sd_init_command(struct scsi_cmnd *SCpnt) { - struct scsi_cmnd *SCpnt; - struct scsi_device *sdp = q->queuedata; + struct request *rq = SCpnt->request; + struct scsi_device *sdp = SCpnt->device; struct gendisk *disk = rq->rq_disk; struct scsi_disk *sdkp; sector_t block = blk_rq_pos(rq); @@ -886,12 +882,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) } else if (rq->cmd_flags & REQ_FLUSH) { ret = scsi_setup_flush_cmnd(sdp, rq); goto out; - } else if (rq->cmd_type == REQ_TYPE_BLOCK_PC) { - ret = scsi_setup_blk_pc_cmnd(sdp, rq); - goto out; - } else if (rq->cmd_type != REQ_TYPE_FS) { - ret = BLKPREP_KILL; - goto out; } ret = scsi_setup_fs_cmnd(sdp, rq); if (ret != BLKPREP_OK) @@ -903,11 +893,10 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) * is used for a killable error condition */ ret = BLKPREP_KILL; - SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt, - "sd_prep_fn: block=%llu, " - "count=%d\n", - (unsigned long long)block, - this_count)); + SCSI_LOG_HLQUEUE(1, + scmd_printk(KERN_INFO, SCpnt, + "%s: block=%llu, count=%d\n", + __func__, (unsigned long long)block, this_count)); if (!sdp || !scsi_device_online(sdp) || block + blk_rq_sectors(rq) > get_capacity(disk)) { @@ -1127,7 +1116,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) */ ret = BLKPREP_OK; out: - return scsi_prep_return(q, rq, ret); + return ret; } /** @@ -1663,6 +1652,8 @@ static int sd_done(struct scsi_cmnd *SCpnt) unsigned char op = SCpnt->cmnd[0]; unsigned char unmap = SCpnt->cmnd[1] & 8; + sd_uninit_command(SCpnt); + if (req->cmd_flags & REQ_DISCARD || req->cmd_flags & REQ_WRITE_SAME) { if (!result) { good_bytes = blk_rq_bytes(req); @@ -2878,9 +2869,6 @@ static void sd_probe_async(void *data, async_cookie_t cookie) sd_revalidate_disk(gd); - blk_queue_prep_rq(sdp->request_queue, sd_prep_fn); - blk_queue_unprep_rq(sdp->request_queue, sd_unprep_fn); - gd->driverfs_dev = &sdp->sdev_gendev; gd->flags = GENHD_FL_EXT_DEVT; if (sdp->removable) { @@ -3027,8 +3015,6 @@ static int sd_remove(struct device *dev) scsi_autopm_get_device(sdkp->device); async_synchronize_full_domain(&scsi_sd_probe_domain); - blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn); - blk_queue_unprep_rq(sdkp->device->request_queue, NULL); device_del(&sdkp->dev); del_gendisk(sdkp->disk); sd_shutdown(dev); diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c index 40d8592..93cbd36 100644 --- a/drivers/scsi/sr.c +++ b/drivers/scsi/sr.c @@ -79,6 +79,7 @@ MODULE_ALIAS_SCSI_DEVICE(TYPE_WORM); static DEFINE_MUTEX(sr_mutex); static int sr_probe(struct device *); static int sr_remove(struct device *); +static int sr_init_command(struct scsi_cmnd *SCpnt); static int sr_done(struct scsi_cmnd *); static int sr_runtime_suspend(struct device *dev); @@ -94,6 +95,7 @@ static struct scsi_driver sr_template = { .remove = sr_remove, .pm = &sr_pm_ops, }, + .init_command = sr_init_command, .done = sr_done, }; @@ -378,21 +380,14 @@ static int sr_done(struct scsi_cmnd *SCpnt) return good_bytes; } -static int sr_prep_fn(struct request_queue *q, struct request *rq) +static int sr_init_command(struct scsi_cmnd *SCpnt) { int block = 0, this_count, s_size; struct scsi_cd *cd; - struct scsi_cmnd *SCpnt; - struct scsi_device *sdp = q->queuedata; + struct request *rq = SCpnt->request; + struct scsi_device *sdp = SCpnt->device; int ret; - if (rq->cmd_type == REQ_TYPE_BLOCK_PC) { - ret = scsi_setup_blk_pc_cmnd(sdp, rq); - goto out; - } else if (rq->cmd_type != REQ_TYPE_FS) { - ret = BLKPREP_KILL; - goto out; - } ret = scsi_setup_fs_cmnd(sdp, rq); if (ret != BLKPREP_OK) goto out; @@ -517,7 +512,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq) */ ret = BLKPREP_OK; out: - return scsi_prep_return(q, rq, ret); + return ret; } static int sr_block_open(struct block_device *bdev, fmode_t mode) @@ -718,7 +713,6 @@ static int sr_probe(struct device *dev) /* FIXME: need to handle a get_capabilities failure properly ?? */ get_capabilities(cd); - blk_queue_prep_rq(sdev->request_queue, sr_prep_fn); sr_vendor_init(cd); disk->driverfs_dev = &sdev->sdev_gendev; @@ -993,7 +987,6 @@ static int sr_remove(struct device *dev) scsi_autopm_get_device(cd->device); - blk_queue_prep_rq(cd->device->request_queue, scsi_prep_fn); del_gendisk(cd->disk); mutex_lock(&sr_ref_mutex); diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h index 20fdfc2..b507729 100644 --- a/include/scsi/scsi_driver.h +++ b/include/scsi/scsi_driver.h @@ -6,15 +6,14 @@ struct module; struct scsi_cmnd; struct scsi_device; -struct request; -struct request_queue; - struct scsi_driver { struct module *owner; struct device_driver gendrv; void (*rescan)(struct device *); + int (*init_command)(struct scsi_cmnd *); + void (*uninit_command)(struct scsi_cmnd *); int (*done)(struct scsi_cmnd *); int (*eh_action)(struct scsi_cmnd *, int); }; @@ -31,8 +30,5 @@ extern int scsi_register_interface(struct class_interface *); int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req); int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req); -int scsi_prep_state_check(struct scsi_device *sdev, struct request *req); -int scsi_prep_return(struct request_queue *q, struct request *req, int ret); -int scsi_prep_fn(struct request_queue *, struct request *); #endif /* _SCSI_SCSI_DRIVER_H */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command 2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig @ 2014-03-31 5:45 ` Nicholas A. Bellinger 2014-03-31 6:08 ` Christoph Hellwig 2014-03-31 6:56 ` Mike Christie 1 sibling, 1 reply; 9+ messages in thread From: Nicholas A. Bellinger @ 2014-03-31 5:45 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi On Thu, 2014-03-27 at 17:14 +0100, Christoph Hellwig wrote: > Move control of the prep_fn back from the ULDs into the scsi layer. Besides > cleaning up the code and removing the only use of the unprep_fn > requeuest_queue method this also prepares for usinng blk-mq, which doesn't > have equivalent functionality to the prep_fn method and requires the driver > to provide just a single I/O submission method. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > drivers/scsi/scsi_lib.c | 66 ++++++++++++++++++++++++++------------------ > drivers/scsi/sd.c | 46 +++++++++++------------------- > drivers/scsi/sr.c | 19 ++++--------- > include/scsi/scsi_driver.h | 8 ++---- > 4 files changed, 63 insertions(+), 76 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index a73751b..48c5c77 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c <SNIP> > @@ -1071,15 +1083,7 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev, > > int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) > { > - struct scsi_cmnd *cmd; > - int ret = scsi_prep_state_check(sdev, req); > - > - if (ret != BLKPREP_OK) > - return ret; > - > - cmd = scsi_get_cmd_from_req(sdev, req); > - if (unlikely(!cmd)) > - return BLKPREP_DEFER; > + struct scsi_cmnd *cmd = req->special; > Mmm, I thought that req->special was only holding a pointer to a pre-allocated scsi_cmnd during mq operation, no..? > /* > * BLOCK_PC requests may transfer data, in which case they must > @@ -1123,15 +1127,11 @@ EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd); > */ > int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req) > { > - struct scsi_cmnd *cmd; > - int ret = scsi_prep_state_check(sdev, req); > - > - if (ret != BLKPREP_OK) > - return ret; > + struct scsi_cmnd *cmd = req->special; > Ditto here for REQ_TYPE_FS_CMND > if (unlikely(sdev->scsi_dh_data && sdev->scsi_dh_data->scsi_dh > && sdev->scsi_dh_data->scsi_dh->prep_fn)) { > - ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req); > + int ret = sdev->scsi_dh_data->scsi_dh->prep_fn(sdev, req); > if (ret != BLKPREP_OK) > return ret; > } > @@ -1141,16 +1141,13 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req) > */ > BUG_ON(!req->nr_phys_segments); > > - cmd = scsi_get_cmd_from_req(sdev, req); > - if (unlikely(!cmd)) > - return BLKPREP_DEFER; > - > memset(cmd->cmnd, 0, BLK_MAX_CDB); > return scsi_init_io(cmd, GFP_ATOMIC); > } > EXPORT_SYMBOL(scsi_setup_fs_cmnd); > > -int scsi_prep_state_check(struct scsi_device *sdev, struct request *req) > +static int > +scsi_prep_state_check(struct scsi_device *sdev, struct request *req) > { > int ret = BLKPREP_OK; > > @@ -1202,9 +1199,9 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req) > } > return ret; > } > -EXPORT_SYMBOL(scsi_prep_state_check); > > -int scsi_prep_return(struct request_queue *q, struct request *req, int ret) > +static int > +scsi_prep_return(struct request_queue *q, struct request *req, int ret) > { > struct scsi_device *sdev = q->queuedata; > > @@ -1235,18 +1232,33 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret) > > return ret; > } > -EXPORT_SYMBOL(scsi_prep_return); > > -int scsi_prep_fn(struct request_queue *q, struct request *req) > +static int scsi_prep_fn(struct request_queue *q, struct request *req) > { > struct scsi_device *sdev = q->queuedata; > - int ret = BLKPREP_KILL; > + struct scsi_cmnd *cmd; > + int ret; > > - if (req->cmd_type == REQ_TYPE_BLOCK_PC) > + ret = scsi_prep_state_check(sdev, req); > + if (ret != BLKPREP_OK) > + goto out; > + > + cmd = scsi_get_cmd_from_req(sdev, req); > + if (unlikely(!cmd)) { > + ret = BLKPREP_DEFER; > + goto out; > + } > + >From here the req->special pointer may or may not be set depending on mq operation, no..? > + if (req->cmd_type == REQ_TYPE_FS) > + ret = scsi_cmd_to_driver(cmd)->init_command(cmd); > + else if (req->cmd_type == REQ_TYPE_BLOCK_PC) > ret = scsi_setup_blk_pc_cmnd(sdev, req); > + else > + ret = BLKPREP_KILL; > + > +out: > return scsi_prep_return(q, req, ret); > } > -EXPORT_SYMBOL(scsi_prep_fn); > > /* > * scsi_dev_queue_ready: if we can send requests to sdev, return 1 else > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 89e6c04..d95c4fd 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c <SNIP> > @@ -886,12 +882,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) > } else if (rq->cmd_flags & REQ_FLUSH) { > ret = scsi_setup_flush_cmnd(sdp, rq); > goto out; > - } else if (rq->cmd_type == REQ_TYPE_BLOCK_PC) { > - ret = scsi_setup_blk_pc_cmnd(sdp, rq); > - goto out; > - } else if (rq->cmd_type != REQ_TYPE_FS) { > - ret = BLKPREP_KILL; > - goto out; > } > ret = scsi_setup_fs_cmnd(sdp, rq); > if (ret != BLKPREP_OK) And this currently assumes req->special is always set when calling scsi_setup_fs_cmnd() > @@ -903,11 +893,10 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) > * is used for a killable error condition */ > ret = BLKPREP_KILL; > > - SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt, > - "sd_prep_fn: block=%llu, " > - "count=%d\n", > - (unsigned long long)block, > - this_count)); > + SCSI_LOG_HLQUEUE(1, > + scmd_printk(KERN_INFO, SCpnt, > + "%s: block=%llu, count=%d\n", > + __func__, (unsigned long long)block, this_count)); > > if (!sdp || !scsi_device_online(sdp) || > block + blk_rq_sectors(rq) > get_capacity(disk)) { > @@ -1127,7 +1116,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) > */ > ret = BLKPREP_OK; > out: > - return scsi_prep_return(q, rq, ret); > + return ret; > } > > /** > @@ -1663,6 +1652,8 @@ static int sd_done(struct scsi_cmnd *SCpnt) > unsigned char op = SCpnt->cmnd[0]; > unsigned char unmap = SCpnt->cmnd[1] & 8; > > + sd_uninit_command(SCpnt); > + > if (req->cmd_flags & REQ_DISCARD || req->cmd_flags & REQ_WRITE_SAME) { > if (!result) { > good_bytes = blk_rq_bytes(req); > @@ -2878,9 +2869,6 @@ static void sd_probe_async(void *data, async_cookie_t cookie) > > sd_revalidate_disk(gd); > > - blk_queue_prep_rq(sdp->request_queue, sd_prep_fn); > - blk_queue_unprep_rq(sdp->request_queue, sd_unprep_fn); > - > gd->driverfs_dev = &sdp->sdev_gendev; > gd->flags = GENHD_FL_EXT_DEVT; > if (sdp->removable) { > @@ -3027,8 +3015,6 @@ static int sd_remove(struct device *dev) > scsi_autopm_get_device(sdkp->device); > > async_synchronize_full_domain(&scsi_sd_probe_domain); > - blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn); > - blk_queue_unprep_rq(sdkp->device->request_queue, NULL); > device_del(&sdkp->dev); > del_gendisk(sdkp->disk); > sd_shutdown(dev); > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c > index 40d8592..93cbd36 100644 > --- a/drivers/scsi/sr.c > +++ b/drivers/scsi/sr.c <SNIP> > @@ -378,21 +380,14 @@ static int sr_done(struct scsi_cmnd *SCpnt) > return good_bytes; > } > > -static int sr_prep_fn(struct request_queue *q, struct request *rq) > +static int sr_init_command(struct scsi_cmnd *SCpnt) > { > int block = 0, this_count, s_size; > struct scsi_cd *cd; > - struct scsi_cmnd *SCpnt; > - struct scsi_device *sdp = q->queuedata; > + struct request *rq = SCpnt->request; > + struct scsi_device *sdp = SCpnt->device; > int ret; > > - if (rq->cmd_type == REQ_TYPE_BLOCK_PC) { > - ret = scsi_setup_blk_pc_cmnd(sdp, rq); > - goto out; > - } else if (rq->cmd_type != REQ_TYPE_FS) { > - ret = BLKPREP_KILL; > - goto out; > - } > ret = scsi_setup_fs_cmnd(sdp, rq); > if (ret != BLKPREP_OK) > goto out; Ditto to the scsi_setup_fs_cmnd() call here as well. --nab ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command 2014-03-31 5:45 ` Nicholas A. Bellinger @ 2014-03-31 6:08 ` Christoph Hellwig 2014-03-31 6:20 ` Nicholas A. Bellinger 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2014-03-31 6:08 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: Christoph Hellwig, James Bottomley, linux-scsi On Sun, Mar 30, 2014 at 10:45:04PM -0700, Nicholas A. Bellinger wrote: > > int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) > > { > > - struct scsi_cmnd *cmd; > > - int ret = scsi_prep_state_check(sdev, req); > > - > > - if (ret != BLKPREP_OK) > > - return ret; > > - > > - cmd = scsi_get_cmd_from_req(sdev, req); > > - if (unlikely(!cmd)) > > - return BLKPREP_DEFER; > > + struct scsi_cmnd *cmd = req->special; > > > > Mmm, I thought that req->special was only holding a pointer to a > pre-allocated scsi_cmnd during mq operation, no..? For the mq case the block layer sets up req->special. For the old case scsi_get_cmd_from_req sets up req->special the first it is called, and leaves it in there until the command is done. > > + ret = scsi_prep_state_check(sdev, req); > > + if (ret != BLKPREP_OK) > > + goto out; > > + > > + cmd = scsi_get_cmd_from_req(sdev, req); > > + if (unlikely(!cmd)) { > > + ret = BLKPREP_DEFER; > > + goto out; > > + } > > + >From here the req->special pointer may or may not be set depending on mq operation, no..? a) there is no blk-mq support yet in James tree (+ these patches) b) even in my scsi-mq tree this code path won't be called for the mq case c) after scsi_get_cmd_from_req returns req->special will always be set up to point to the scsi_cmnd: static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev, struct request *req) { struct scsi_cmnd *cmd; if (!req->special) { ... req->special = cmd; } else { cmd = req->special; } /* pull a tag out of the request if we have one */ cmd->tag = req->tag; cmd->request = req; cmd->cmnd = req->cmd; cmd->prot_op = SCSI_PROT_NORMAL; return cmd; } > > } > > ret = scsi_setup_fs_cmnd(sdp, rq); > > if (ret != BLKPREP_OK) > > And this currently assumes req->special is always set when calling > scsi_setup_fs_cmnd() That is the case both before and after this patch. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command 2014-03-31 6:08 ` Christoph Hellwig @ 2014-03-31 6:20 ` Nicholas A. Bellinger 0 siblings, 0 replies; 9+ messages in thread From: Nicholas A. Bellinger @ 2014-03-31 6:20 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi On Mon, 2014-03-31 at 08:08 +0200, Christoph Hellwig wrote: > On Sun, Mar 30, 2014 at 10:45:04PM -0700, Nicholas A. Bellinger wrote: > > > int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) > > > { > > > - struct scsi_cmnd *cmd; > > > - int ret = scsi_prep_state_check(sdev, req); > > > - > > > - if (ret != BLKPREP_OK) > > > - return ret; > > > - > > > - cmd = scsi_get_cmd_from_req(sdev, req); > > > - if (unlikely(!cmd)) > > > - return BLKPREP_DEFER; > > > + struct scsi_cmnd *cmd = req->special; > > > > > > > Mmm, I thought that req->special was only holding a pointer to a > > pre-allocated scsi_cmnd during mq operation, no..? > > For the mq case the block layer sets up req->special. For the old case > scsi_get_cmd_from_req sets up req->special the first it is called, and > leaves it in there until the command is done. Er, yes of course. Reviewed-by: Nicholas Bellinger <nab@linux-iscsi.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command 2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig 2014-03-31 5:45 ` Nicholas A. Bellinger @ 2014-03-31 6:56 ` Mike Christie 2014-03-31 8:09 ` Christoph Hellwig 1 sibling, 1 reply; 9+ messages in thread From: Mike Christie @ 2014-03-31 6:56 UTC (permalink / raw) To: Christoph Hellwig; +Cc: James Bottomley, linux-scsi On 03/27/2014 11:14 AM, Christoph Hellwig wrote: > @@ -1663,6 +1652,8 @@ static int sd_done(struct scsi_cmnd *SCpnt) > unsigned char op = SCpnt->cmnd[0]; > unsigned char unmap = SCpnt->cmnd[1] & 8; > > + sd_uninit_command(SCpnt); > + The above call would free the cmnd->cmnd and set it to null. If then scsi_io_completion was going to do some error processing it looks like it could try to access the scsi_cmnd->cmnd field. With the current code that would not be a problem because the blk unprep callback is not called until the block layer does its request cleanup in blk_finish_request which as you know is after scsi_io_completion/scsi_end_request is done with the cmnd. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] scsi: reintroduce scsi_driver.init_command 2014-03-31 6:56 ` Mike Christie @ 2014-03-31 8:09 ` Christoph Hellwig 0 siblings, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2014-03-31 8:09 UTC (permalink / raw) To: Mike Christie; +Cc: Christoph Hellwig, James Bottomley, linux-scsi On Mon, Mar 31, 2014 at 01:56:13AM -0500, Mike Christie wrote: > The above call would free the cmnd->cmnd and set it to null. If then > scsi_io_completion was going to do some error processing it looks like > it could try to access the scsi_cmnd->cmnd field. > > With the current code that would not be a problem because the blk unprep > callback is not called until the block layer does its request cleanup in > blk_finish_request which as you know is after > scsi_io_completion/scsi_end_request is done with the cmnd. Right, we should probabl call ->uninit_command at that place as well instead of calling it internall in ->done. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-05-08 7:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-31 10:45 [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig -- strict thread matches above, loose matches on Subject: below -- 2014-05-01 14:51 scsi midlayer updates for 3.15 (resend) Christoph Hellwig 2014-05-01 14:51 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig 2014-05-08 7:11 ` Hannes Reinecke 2014-03-27 16:14 misc scsi midlayer updates Christoph Hellwig 2014-03-27 16:14 ` [PATCH 3/4] scsi: reintroduce scsi_driver.init_command Christoph Hellwig 2014-03-31 5:45 ` Nicholas A. Bellinger 2014-03-31 6:08 ` Christoph Hellwig 2014-03-31 6:20 ` Nicholas A. Bellinger 2014-03-31 6:56 ` Mike Christie 2014-03-31 8:09 ` 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.