diff mbox

[05/18] block: allow specifying size for extra command data

Message ID 1485365126-23210-6-git-send-email-hch@lst.de (mailing list archive)
State New, archived
Headers show

Commit Message

Christoph Hellwig Jan. 25, 2017, 5:25 p.m. UTC 
This mirrors the blk-mq capabilities to allocate extra drivers-specific
data behind struct request by setting a cmd_size field, as well as having
a constructor / destructor for it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Hannes Reinecke <hare@suse.com>
---
 block/blk-core.c       | 59 ++++++++++++++++++++++++++++++++++++++++----------
 block/blk-flush.c      |  5 ++---
 block/blk-sysfs.c      |  7 ++++--
 include/linux/blkdev.h |  7 ++++++
 4 files changed, 61 insertions(+), 17 deletions(-)

Comments

Martin K. Petersen Jan. 26, 2017, 3:15 a.m. UTC | #1 
>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph,

Christoph> This mirrors the blk-mq capabilities to allocate extra
Christoph> drivers-specific data behind struct request by setting a
Christoph> cmd_size field, as well as having a constructor / destructor
Christoph> for it.

Nice!

A couple of minor nits:

+static void *alloc_request_size(gfp_t gfp_mask, void *data)

I like alloc_request_simple() but alloc_request_size() seems a bit
contrived. _reserve? _extra? _special? Don't have any good suggestions,
I'm afraid.

Also a bit heavy on the else brackets a couple of places. But no biggie.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Christoph Hellwig Jan. 27, 2017, 4:12 p.m. UTC | #2 
On Wed, Jan 25, 2017 at 10:15:55PM -0500, Martin K. Petersen wrote:
> +static void *alloc_request_size(gfp_t gfp_mask, void *data)
> 
> I like alloc_request_simple() but alloc_request_size() seems a bit
> contrived. _reserve? _extra? _special? Don't have any good suggestions,
> I'm afraid.

Not that I'm a fan of _size, but I like the other suggestions even less.
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Jan. 27, 2017, 5:21 p.m. UTC | #3 
On Fri, 2017-01-27 at 17:12 +0100, Christoph Hellwig wrote:
> On Wed, Jan 25, 2017 at 10:15:55PM -0500, Martin K. Petersen wrote:
> > +static void *alloc_request_size(gfp_t gfp_mask, void *data)
> > 
> > I like alloc_request_simple() but alloc_request_size() seems a bit
> > contrived. _reserve? _extra? _special? Don't have any good suggestions,
> > I'm afraid.
> 
> Not that I'm a fan of _size, but I like the other suggestions even less.

Hello Christoph and Martin,

How about using the function names alloc_full_request() / free_full_request()
together with a comment that mentions that cmd_size is set by the LLD?

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jens Axboe Jan. 27, 2017, 5:26 p.m. UTC | #4 
On 01/27/2017 10:21 AM, Bart Van Assche wrote:
> On Fri, 2017-01-27 at 17:12 +0100, Christoph Hellwig wrote:
>> On Wed, Jan 25, 2017 at 10:15:55PM -0500, Martin K. Petersen wrote:
>>> +static void *alloc_request_size(gfp_t gfp_mask, void *data)
>>>
>>> I like alloc_request_simple() but alloc_request_size() seems a bit
>>> contrived. _reserve? _extra? _special? Don't have any good suggestions,
>>> I'm afraid.
>>
>> Not that I'm a fan of _size, but I like the other suggestions even less.
> 
> Hello Christoph and Martin,
> 
> How about using the function names alloc_full_request() / free_full_request()
> together with a comment that mentions that cmd_size is set by the LLD?

Since we use pdu in other places, how about alloc_request_pdu() or
alloc_request_with_pdu()?

And since it's all queued up, any bike shedding changes will have to be
incremental.
Bart Van Assche Jan. 27, 2017, 5:30 p.m. UTC | #5 
On Fri, 2017-01-27 at 10:26 -0700, Jens Axboe wrote:
> On 01/27/2017 10:21 AM, Bart Van Assche wrote:
> > On Fri, 2017-01-27 at 17:12 +0100, Christoph Hellwig wrote:
> > > On Wed, Jan 25, 2017 at 10:15:55PM -0500, Martin K. Petersen wrote:
> > > > +static void *alloc_request_size(gfp_t gfp_mask, void *data)
> > > > 
> > > > I like alloc_request_simple() but alloc_request_size() seems a bit
> > > > contrived. _reserve? _extra? _special? Don't have any good suggestions,
> > > > I'm afraid.
> > > 
> > > Not that I'm a fan of _size, but I like the other suggestions even less.
> > 
> > Hello Christoph and Martin,
> > 
> > How about using the function names alloc_full_request() / free_full_request()
> > together with a comment that mentions that cmd_size is set by the LLD?
> 
> Since we use pdu in other places, how about alloc_request_pdu() or
> alloc_request_with_pdu()?
> 
> And since it's all queued up, any bike shedding changes will have to be
> incremental.

Hello Jens,

Other Linux subsystems use the term "private data" instead of PDU. How about
modifying the block layer such that it uses the same terminology? I'm
referring to function names like blk_mq_rq_from_pdu() and blk_mq_rq_to_pdu()

Thanks,

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jens Axboe Jan. 27, 2017, 5:33 p.m. UTC | #6 
On 01/27/2017 10:30 AM, Bart Van Assche wrote:
> On Fri, 2017-01-27 at 10:26 -0700, Jens Axboe wrote:
>> On 01/27/2017 10:21 AM, Bart Van Assche wrote:
>>> On Fri, 2017-01-27 at 17:12 +0100, Christoph Hellwig wrote:
>>>> On Wed, Jan 25, 2017 at 10:15:55PM -0500, Martin K. Petersen wrote:
>>>>> +static void *alloc_request_size(gfp_t gfp_mask, void *data)
>>>>>
>>>>> I like alloc_request_simple() but alloc_request_size() seems a bit
>>>>> contrived. _reserve? _extra? _special? Don't have any good suggestions,
>>>>> I'm afraid.
>>>>
>>>> Not that I'm a fan of _size, but I like the other suggestions even less.
>>>
>>> Hello Christoph and Martin,
>>>
>>> How about using the function names alloc_full_request() / free_full_request()
>>> together with a comment that mentions that cmd_size is set by the LLD?
>>
>> Since we use pdu in other places, how about alloc_request_pdu() or
>> alloc_request_with_pdu()?
>>
>> And since it's all queued up, any bike shedding changes will have to be
>> incremental.
> 
> Hello Jens,
> 
> Other Linux subsystems use the term "private data" instead of PDU. How about
> modifying the block layer such that it uses the same terminology? I'm
> referring to function names like blk_mq_rq_from_pdu() and blk_mq_rq_to_pdu()

It's been pdu since it was introduced in 3.13, I really don't see a good
reason to change it. At least pdu or payload means something, where as
private is just... Well, not a big fan.
diff mbox

Patch

diff --git a/block/blk-core.c b/block/blk-core.c
index 54b5512..7de7164 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -606,17 +606,41 @@  void blk_cleanup_queue(struct request_queue *q)
 EXPORT_SYMBOL(blk_cleanup_queue);
 
 /* Allocate memory local to the request queue */
-static void *alloc_request_struct(gfp_t gfp_mask, void *data)
+static void *alloc_request_simple(gfp_t gfp_mask, void *data)
 {
-	int nid = (int)(long)data;
-	return kmem_cache_alloc_node(request_cachep, gfp_mask, nid);
+	struct request_queue *q = data;
+
+	return kmem_cache_alloc_node(request_cachep, gfp_mask, q->node);
 }
 
-static void free_request_struct(void *element, void *unused)
+static void free_request_simple(void *element, void *data)
 {
 	kmem_cache_free(request_cachep, element);
 }
 
+static void *alloc_request_size(gfp_t gfp_mask, void *data)
+{
+	struct request_queue *q = data;
+	struct request *rq;
+
+	rq = kmalloc_node(sizeof(struct request) + q->cmd_size, gfp_mask,
+			q->node);
+	if (rq && q->init_rq_fn && q->init_rq_fn(q, rq, gfp_mask) < 0) {
+		kfree(rq);
+		rq = NULL;
+	}
+	return rq;
+}
+
+static void free_request_size(void *element, void *data)
+{
+	struct request_queue *q = data;
+
+	if (q->exit_rq_fn)
+		q->exit_rq_fn(q, element);
+	kfree(element);
+}
+
 int blk_init_rl(struct request_list *rl, struct request_queue *q,
 		gfp_t gfp_mask)
 {
@@ -629,10 +653,15 @@  int blk_init_rl(struct request_list *rl, struct request_queue *q,
 	init_waitqueue_head(&rl->wait[BLK_RW_SYNC]);
 	init_waitqueue_head(&rl->wait[BLK_RW_ASYNC]);
 
-	rl->rq_pool = mempool_create_node(BLKDEV_MIN_RQ, alloc_request_struct,
-					  free_request_struct,
-					  (void *)(long)q->node, gfp_mask,
-					  q->node);
+	if (q->cmd_size) {
+		rl->rq_pool = mempool_create_node(BLKDEV_MIN_RQ,
+				alloc_request_size, free_request_size,
+				q, gfp_mask, q->node);
+	} else {
+		rl->rq_pool = mempool_create_node(BLKDEV_MIN_RQ,
+				alloc_request_simple, free_request_simple,
+				q, gfp_mask, q->node);
+	}
 	if (!rl->rq_pool)
 		return -ENOMEM;
 
@@ -846,12 +875,15 @@  static blk_qc_t blk_queue_bio(struct request_queue *q, struct bio *bio);
 
 int blk_init_allocated_queue(struct request_queue *q)
 {
-	q->fq = blk_alloc_flush_queue(q, NUMA_NO_NODE, 0);
+	q->fq = blk_alloc_flush_queue(q, NUMA_NO_NODE, q->cmd_size);
 	if (!q->fq)
 		return -ENOMEM;
 
+	if (q->init_rq_fn && q->init_rq_fn(q, q->fq->flush_rq, GFP_KERNEL))
+		goto out_free_flush_queue;
+
 	if (blk_init_rl(&q->root_rl, q, GFP_KERNEL))
-		goto fail;
+		goto out_exit_flush_rq;
 
 	INIT_WORK(&q->timeout_work, blk_timeout_work);
 	q->queue_flags		|= QUEUE_FLAG_DEFAULT;
@@ -869,13 +901,16 @@  int blk_init_allocated_queue(struct request_queue *q)
 	/* init elevator */
 	if (elevator_init(q, NULL)) {
 		mutex_unlock(&q->sysfs_lock);
-		goto fail;
+		goto out_exit_flush_rq;
 	}
 
 	mutex_unlock(&q->sysfs_lock);
 	return 0;
 
-fail:
+out_exit_flush_rq:
+	if (q->exit_rq_fn)
+		q->exit_rq_fn(q, q->fq->flush_rq);
+out_free_flush_queue:
 	blk_free_flush_queue(q->fq);
 	wbt_exit(q);
 	return -ENOMEM;
diff --git a/block/blk-flush.c b/block/blk-flush.c
index d7de34e..bf3ba3c 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -547,11 +547,10 @@  struct blk_flush_queue *blk_alloc_flush_queue(struct request_queue *q,
 	if (!fq)
 		goto fail;
 
-	if (q->mq_ops) {
+	if (q->mq_ops)
 		spin_lock_init(&fq->mq_flush_lock);
-		rq_sz = round_up(rq_sz + cmd_size, cache_line_size());
-	}
 
+	rq_sz = round_up(rq_sz + cmd_size, cache_line_size());
 	fq->flush_rq = kzalloc_node(rq_sz, GFP_KERNEL, node);
 	if (!fq->flush_rq)
 		goto fail_rq;
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 1dbce05..894f773 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -814,10 +814,13 @@  static void blk_release_queue(struct kobject *kobj)
 	if (q->queue_tags)
 		__blk_queue_free_tags(q);
 
-	if (!q->mq_ops)
+	if (!q->mq_ops) {
+		if (q->exit_rq_fn)
+			q->exit_rq_fn(q, q->fq->flush_rq);
 		blk_free_flush_queue(q->fq);
-	else
+	} else {
 		blk_mq_release(q);
+	}
 
 	blk_trace_shutdown(q);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index a036c4a..648ecf5 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -273,6 +273,8 @@  typedef void (softirq_done_fn)(struct request *);
 typedef int (dma_drain_needed_fn)(struct request *);
 typedef int (lld_busy_fn) (struct request_queue *q);
 typedef int (bsg_job_fn) (struct bsg_job *);
+typedef int (init_rq_fn)(struct request_queue *, struct request *, gfp_t);
+typedef void (exit_rq_fn)(struct request_queue *, struct request *);
 
 enum blk_eh_timer_return {
 	BLK_EH_NOT_HANDLED,
@@ -408,6 +410,8 @@  struct request_queue {
 	rq_timed_out_fn		*rq_timed_out_fn;
 	dma_drain_needed_fn	*dma_drain_needed;
 	lld_busy_fn		*lld_busy_fn;
+	init_rq_fn		*init_rq_fn;
+	exit_rq_fn		*exit_rq_fn;
 
 	const struct blk_mq_ops	*mq_ops;
 
@@ -572,6 +576,9 @@  struct request_queue {
 	struct bio_set		*bio_split;
 
 	bool			mq_sysfs_init_done;
+
+	size_t			cmd_size;
+	void			*rq_alloc_data;
 };
 
 #define QUEUE_FLAG_QUEUED	1	/* uses generic tag queueing */