| Message ID | 20170525184327.23570-9-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 25, 2017 at 11:43:16AM -0700, Bart Van Assche wrote: > Several block drivers need to initialize the driver-private data > after having called blk_get_request() and before .prep_rq_fn() is > called, e.g. when submitting a REQ_OP_SCSI_* request. Avoid that > that initialization code has to be repeated after every > blk_get_request() call by adding a new callback function to struct > request_queue. I still think we should do this at the end of blk_mq_alloc_request / blk_old_get_request to exactly keep the old semantics and keep the calls out of the hot path.
On Fri, 2017-05-26 at 08:34 +0200, Christoph Hellwig wrote: > On Thu, May 25, 2017 at 11:43:16AM -0700, Bart Van Assche wrote: > > Several block drivers need to initialize the driver-private data > > after having called blk_get_request() and before .prep_rq_fn() is > > called, e.g. when submitting a REQ_OP_SCSI_* request. Avoid that > > that initialization code has to be repeated after every > > blk_get_request() call by adding a new callback function to struct > > request_queue. > > I still think we should do this at the end of blk_mq_alloc_request / > blk_old_get_request to exactly keep the old semantics and keep the > calls out of the hot path. Hello Christoph, I have tried to move that call into blk_mq_alloc_request() but that resulted in a kernel oops during boot due to scsi_add_cmd_to_list() dereferencing scsi_cmnd.device and due to that pointer being invalid. I think that pointer was invalid because moving the initialize_rq_fn() call into blk_mq_alloc_request() caused request initialization to be skipped for the following code path: submit_bio() -> generic_make_request() -> .make_request_fn == blk_mq_make_request() -> blk_mq_sched_get_request() -> __blk_mq_alloc_request() -> blk_mq_rq_ctx_init() This is why I would like to keep the .initialize_rq_fn() call in blk_mq_rq_ctx_init(). Bart.
On Fri, May 26, 2017 at 11:56:30PM +0000, Bart Van Assche wrote: > I have tried to move that call into blk_mq_alloc_request() but that > resulted in a kernel oops during boot due to scsi_add_cmd_to_list() > dereferencing scsi_cmnd.device and due to that pointer being invalid. > I think that pointer was invalid because moving the initialize_rq_fn() > call into blk_mq_alloc_request() caused request initialization to be > skipped for the following code path: > submit_bio() > -> generic_make_request() > -> .make_request_fn == blk_mq_make_request() > -> blk_mq_sched_get_request() > -> __blk_mq_alloc_request() > -> blk_mq_rq_ctx_init() > > This is why I would like to keep the .initialize_rq_fn() call in > blk_mq_rq_ctx_init(). But we don't call scsi_req_init for this path either with the current code. So not having the call should be fine as long as you ensure we still manually initialize everything for the non-passthrough path in the later patches. I'll keep an eye on that issue while reviewing the remaining patches.
Oh, and btw - for the mq case we don't want to use the function pointer directly in the queue, but in blk_mq_ops, so that we have all the mq methods in one place.
On Sun, 2017-05-28 at 10:34 +0200, hch@lst.de wrote: > On Fri, May 26, 2017 at 11:56:30PM +0000, Bart Van Assche wrote: > > I have tried to move that call into blk_mq_alloc_request() but that > > resulted in a kernel oops during boot due to scsi_add_cmd_to_list() > > dereferencing scsi_cmnd.device and due to that pointer being invalid. > > I think that pointer was invalid because moving the initialize_rq_fn() > > call into blk_mq_alloc_request() caused request initialization to be > > skipped for the following code path: > > submit_bio() > > -> generic_make_request() > > -> .make_request_fn == blk_mq_make_request() > > -> blk_mq_sched_get_request() > > -> __blk_mq_alloc_request() > > -> blk_mq_rq_ctx_init() > > > > This is why I would like to keep the .initialize_rq_fn() call in > > blk_mq_rq_ctx_init(). > > But we don't call scsi_req_init for this path either with the current > code. So not having the call should be fine as long as you ensure > we still manually initialize everything for the non-passthrough path > in the later patches. I'll keep an eye on that issue while reviewing > the remaining patches. Hello Christoph, Do you see an alternative to introducing an .initialize_rq_fn() call in blk_rq_init() / blk_mq_rq_ctx_init() to ensure that jiffies_at_alloc is only set once and not every time a request is requeued? Thanks, Bart.
On Sun, 2017-05-28 at 10:37 +0200, Christoph Hellwig wrote: > Oh, and btw - for the mq case we don't want to use the function > pointer directly in the queue, but in blk_mq_ops, so that we have > all the mq methods in one place. Hello Christoph, Are you sure of this? All other function pointers that apply to both blk-sq and blk-mq occur in struct request, e.g. .make_request_fn(), .init_rq_fn() and .exit_rq_fn(). If the .initialize_rq_fn() will be added to struct request all I have to add to blk_get_request is the following: if (!IS_ERR(req) && q->initialize_rq_fn) q->initialize_rq_fn(req); However, if for the mq case the .initialize_rq_fn() would be added to struct blk_mq_ops then that code would look as follows: if (!IS_ERR(req)) if (q->mq_ops) q->mq_ops->initialize_rq_fn(req); else q->initialize_rq_fn(req); Personally I prefer the first alternative. Bart.
diff --git a/block/blk-core.c b/block/blk-core.c index 9416f6f495d4..fa453ed95973 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -126,6 +126,9 @@ void blk_rq_init(struct request_queue *q, struct request *rq) rq->start_time = jiffies; set_start_time_ns(rq); rq->part = NULL; + + if (q->initialize_rq_fn) + q->initialize_rq_fn(rq); } EXPORT_SYMBOL(blk_rq_init); diff --git a/block/blk-mq.c b/block/blk-mq.c index 9ae1d9ccf4df..a1620b36b95c 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -241,6 +241,9 @@ void blk_mq_rq_ctx_init(struct request_queue *q, struct blk_mq_ctx *ctx, rq->end_io_data = NULL; rq->next_rq = NULL; + if (q->initialize_rq_fn) + q->initialize_rq_fn(rq); + ctx->rq_dispatched[op_is_sync(op)]++; } EXPORT_SYMBOL_GPL(blk_mq_rq_ctx_init); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 94fd2600584d..8a223a0c95d5 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -410,8 +410,12 @@ struct request_queue { rq_timed_out_fn *rq_timed_out_fn; dma_drain_needed_fn *dma_drain_needed; lld_busy_fn *lld_busy_fn; + /* Called just after a request is allocated */ init_rq_fn *init_rq_fn; + /* Called just before a request is freed */ exit_rq_fn *exit_rq_fn; + /* Called from inside blk_get_request() */ + void (*initialize_rq_fn)(struct request *rq); const struct blk_mq_ops *mq_ops;