Re: [PATCH 07/18] block: Introduce request_queue.initialize_rq_fn()

[Bart Van Assche]

On Sun, 2017-05-21 at 08:34 +0200, Christoph Hellwig wrote:
> On Fri, May 19, 2017 at 11:30:05AM -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.
> > 
> > Signed-off-by: Bart Van Assche 
> > Cc: Jens Axboe 
> > Cc: Christoph Hellwig 
> > Cc: Omar Sandoval 
> > Cc: Hannes Reinecke 
> > Cc: linux-bl...@vger.kernel.org
> > ---
> >  block/blk-core.c   | 3 +++
> >  block/blk-mq.c | 3 +++
> >  include/linux/blkdev.h | 4 
> >  3 files changed, 10 insertions(+)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index a69d420b7ff0..f2540d164679 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);
> 
> Can we keep this out of the fast path and only do it from the
> blk_get_request / blk_mq_alloc_request path?  And while were at it
> I think those two should be merged as far as the public interface
> goes, that is we should also expose the flags argument for the
> legacy path.

Hello Christoph,

For blk-mq I could move the .initialize_rq_fn() call from
blk_mq_rq_ctx_init() into blk_mq_sched_get_request(). I can't move it
higher up in the request allocation call chain because otherwise
blk_mq_alloc_request_hctx() wouldn't call .initialize_rq_fn().

For blk-sq I'm not sure that the .initialize_rq_fn() call can be
moved because I'd like all blk_rq_init() callers to call
.initialize_rq_fn(), including ide_prep_sense().

Are you sure it would help to move the .initialize_rq_fn() calls?

Thanks,

Bart.

Re: [PATCH 07/18] block: Introduce request_queue.initialize_rq_fn()

[Christoph Hellwig]

On Fri, May 19, 2017 at 11:30:05AM -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.
> 
> Signed-off-by: Bart Van Assche 
> Cc: Jens Axboe 
> Cc: Christoph Hellwig 
> Cc: Omar Sandoval 
> Cc: Hannes Reinecke 
> Cc: linux-bl...@vger.kernel.org
> ---
>  block/blk-core.c   | 3 +++
>  block/blk-mq.c | 3 +++
>  include/linux/blkdev.h | 4 
>  3 files changed, 10 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index a69d420b7ff0..f2540d164679 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);

Can we keep this out of the fast path and only do it from the
blk_get_request / blk_mq_alloc_request path?  And while were at it
I think those two should be merged as far as the public interface
goes, that is we should also expose the flags argument for the
legacy path.

[PATCH 07/18] block: Introduce request_queue.initialize_rq_fn()

[Bart Van Assche]

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.

Signed-off-by: Bart Van Assche 
Cc: Jens Axboe 
Cc: Christoph Hellwig 
Cc: Omar Sandoval 
Cc: Hannes Reinecke 
Cc: linux-bl...@vger.kernel.org
---
 block/blk-core.c   | 3 +++
 block/blk-mq.c | 3 +++
 include/linux/blkdev.h | 4 
 3 files changed, 10 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index a69d420b7ff0..f2540d164679 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 a69ad122ed66..2af43d4e5b96 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 75b71374e1ba..2ee8da93619d 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;
 
-- 
2.12.2