[PATCH 0/2] Enable polling on stackable devices

[PATCH 0/2] Enable polling on stackable devices

[Andrzej Jakowski]

IO polling is available on blk-mq devices. Currently it is not possible to
perform IO polling on stackable devices like MD. 

We have built a prototype exposing new function for bio devices to do IO
polling on them. That function is in available on request_queue so bio based
drivers can provide handler for it. 

IO polling has been provided for RAID-0 level for MD.

Andrzej Jakowski (2):
  block: introduce polling on bio level
  md: enable io polling

 block/blk-core.c       |  3 ++-
 block/blk-mq.c         | 26 ++++++++++++++++++++++++++
 drivers/md/md.c        | 39 +++++++++++++++++++++++++++++++++++----
 include/linux/blkdev.h |  2 ++
 4 files changed, 65 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH 1/2] block: introduce polling on bio level

[Andrzej Jakowski]

In current implementation it is possible to perform IO polling if
underlying device is block-mq device. Is is not possible however to do
polled IO on stackable block devices like MD.

We have explored two paths for enabling IO polling on bio devices. First
idea revolved around rewriting MD to block-mq interface but it proved to
be complex. In the second idea we have built a prototype which
introduced new operation on request_queue - bio_poll_fn. bio_poll_fn if
provided by stackable block driver is called when user polls for IO
completion. bio_poll_fn approach was initially discussed and confirmed
with Jens.

We managed to collect performance data on RAID-0 volume built on top of
2xP4800X devices with polling on and off. Here are the results:
Polling      QD       Latency         IOPS
----------------------------------------------
off           1       12.03us         78800
off           2       13.27us        144000
off           4       15.83us        245000
off           8       31.14us        253000
off          16       63.03us        252000
off          32      128.89us        247000
off          64      259.10us        246000
off         128      517.27us        247000
on            1        9.00us        108000
on            2        9.07us        214000
on            4       12.00us        327000
on            8       21.43us        369000
on           16       43.18us        369000
on           32       85.75us        372000
on           64      169.87us        376000
on          128      346.15us        370000

Signed-off-by: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
 block/blk-core.c       |  3 ++-
 block/blk-mq.c         | 26 ++++++++++++++++++++++++++
 include/linux/blkdev.h |  2 ++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index e0a094fddee5..5d8706dfebe0 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -889,7 +889,8 @@ generic_make_request_checks(struct bio *bio)
 	 * if queue is not a request based queue.
 	 */
 	if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_mq(q))
-		goto not_supported;
+		if (!(bio->bi_opf & REQ_HIPRI))
+			goto not_supported;
 
 	if (should_fail_bio(bio))
 		goto end_io;
diff --git a/block/blk-mq.c b/block/blk-mq.c
index a12b1763508d..ebbb88a336a7 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3533,6 +3533,32 @@ int blk_poll(struct request_queue *q, blk_qc_t cookie, bool spin)
 	if (current->plug)
 		blk_flush_plug_list(current->plug, false);
 
+	if (q->bio_poll_fn != NULL) {
+		state = current->state;
+		do {
+			int ret;
+
+			ret = q->bio_poll_fn(q, cookie);
+			if (ret > 0) {
+				__set_current_state(TASK_RUNNING);
+				return ret;
+			}
+
+			if (signal_pending_state(state, current))
+				__set_current_state(TASK_RUNNING);
+
+			if (current->state == TASK_RUNNING)
+				return 1;
+			if (ret < 0 || !spin)
+				break;
+			cpu_relax();
+		} while (!need_resched());
+
+		__set_current_state(TASK_RUNNING);
+
+		return 0;
+	}
+
 	hctx = q->queue_hw_ctx[blk_qc_t_to_queue_num(cookie)];
 
 	/*
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 47eb22a3b7f9..1c21c3ff3ad1 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -287,6 +287,7 @@ static inline unsigned short req_get_ioprio(struct request *req)
 
 struct blk_queue_ctx;
 
+typedef int (bio_poll_fn) (struct request_queue *q, blk_qc_t cookie);
 typedef blk_qc_t (make_request_fn) (struct request_queue *q, struct bio *bio);
 
 struct bio_vec;
@@ -398,6 +399,7 @@ struct request_queue {
 	struct blk_queue_stats	*stats;
 	struct rq_qos		*rq_qos;
 
+	bio_poll_fn		*bio_poll_fn;
 	make_request_fn		*make_request_fn;
 	dma_drain_needed_fn	*dma_drain_needed;
 
-- 
2.20.1

[PATCH 2/2] md: enable io polling

[Andrzej Jakowski]

Provide a callback for polling the mddev which in turn polls the active
member devices. Activate it only if all members support polling.

Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
Signed-off-by: Andrzej Jakowski <andrzej.jakowski@linux.intel.com>
---
 drivers/md/md.c | 39 +++++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 4e7c9f398bc6..95173cd4f8fd 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5421,6 +5421,27 @@ int mddev_init_writes_pending(struct mddev *mddev)
 }
 EXPORT_SYMBOL_GPL(mddev_init_writes_pending);
 
+static int md_poll(struct request_queue *q, blk_qc_t cookie)
+{
+	struct mddev *mddev = q->queuedata;
+	struct md_rdev *rdev;
+	int ret = 0;
+	int rv;
+
+	rdev_for_each(rdev, mddev) {
+		if (rdev->raid_disk >= 0 && !test_bit(Faulty, &rdev->flags)) {
+			rv = blk_poll(bdev_get_queue(rdev->bdev), cookie, false);
+			if (rv < 0) {
+				ret = rv;
+				break;
+			}
+			ret += rv;
+		}
+	}
+
+	return ret;
+}
+
 static int md_alloc(dev_t dev, char *name)
 {
 	/*
@@ -5485,6 +5506,7 @@ static int md_alloc(dev_t dev, char *name)
 
 	blk_queue_make_request(mddev->queue, md_make_request);
 	blk_set_stacking_limits(&mddev->queue->limits);
+	mddev->queue->bio_poll_fn = md_poll;
 
 	disk = alloc_disk(1 << shift);
 	if (!disk) {
@@ -5789,12 +5811,17 @@ int md_run(struct mddev *mddev)
 
 	if (mddev->queue) {
 		bool nonrot = true;
+		bool poll = true;
 
 		rdev_for_each(rdev, mddev) {
-			if (rdev->raid_disk >= 0 &&
-			    !blk_queue_nonrot(bdev_get_queue(rdev->bdev))) {
-				nonrot = false;
-				break;
+			if (rdev->raid_disk >= 0) {
+				struct request_queue *q;
+
+				q = bdev_get_queue(rdev->bdev);
+				if (!blk_queue_nonrot(q))
+					nonrot = false;
+				if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
+					poll = false;
 			}
 		}
 		if (mddev->degraded)
@@ -5803,6 +5830,10 @@ int md_run(struct mddev *mddev)
 			blk_queue_flag_set(QUEUE_FLAG_NONROT, mddev->queue);
 		else
 			blk_queue_flag_clear(QUEUE_FLAG_NONROT, mddev->queue);
+		if (poll)
+			blk_queue_flag_set(QUEUE_FLAG_POLL, mddev->queue);
+		else
+			blk_queue_flag_clear(QUEUE_FLAG_POLL, mddev->queue);
 		mddev->queue->backing_dev_info->congested_data = mddev;
 		mddev->queue->backing_dev_info->congested_fn = md_congested;
 	}
-- 
2.20.1

Re: [PATCH 1/2] block: introduce polling on bio level

[Jens Axboe]

On 1/25/20 9:41 PM, Andrzej Jakowski wrote:
> In current implementation it is possible to perform IO polling if
> underlying device is block-mq device. Is is not possible however to do
> polled IO on stackable block devices like MD.
> 
> We have explored two paths for enabling IO polling on bio devices. First
> idea revolved around rewriting MD to block-mq interface but it proved to
> be complex. In the second idea we have built a prototype which
> introduced new operation on request_queue - bio_poll_fn. bio_poll_fn if
> provided by stackable block driver is called when user polls for IO
> completion. bio_poll_fn approach was initially discussed and confirmed
> with Jens.
> 
> We managed to collect performance data on RAID-0 volume built on top of
> 2xP4800X devices with polling on and off. Here are the results:
> Polling      QD       Latency         IOPS
> ----------------------------------------------
> off           1       12.03us         78800
> off           2       13.27us        144000
> off           4       15.83us        245000
> off           8       31.14us        253000
> off          16       63.03us        252000
> off          32      128.89us        247000
> off          64      259.10us        246000
> off         128      517.27us        247000
> on            1        9.00us        108000
> on            2        9.07us        214000
> on            4       12.00us        327000
> on            8       21.43us        369000
> on           16       43.18us        369000
> on           32       85.75us        372000
> on           64      169.87us        376000
> on          128      346.15us        370000

blk_poll() used to be a pointer in the queue, but since we just had
one implementation, we got rid of it. Might make sense to
reintroduce that, and just make it an optimized indirect call. I
think that would be prettier than add the bio hack in the middle of
it, and you're having to add a queue pointer anyway.

Alternatively, do it like you did, but have it be:

if (q->poll_fn)
	return q->poll_fn(...);

instead of duplicating most of the core of the function with essentially
the same thing, just calling ->bio_poll_fn() instead of mq_ops->poll().

In other words, I like the feature, but I think the implementation
currently leaves a lot to be desired. It should be nicely integrated,
not some hack off to the side.

-- 
Jens Axboe

Re: [PATCH 1/2] block: introduce polling on bio level

[Christoph Hellwig]

On Sat, Jan 25, 2020 at 09:41:37PM -0700, Andrzej Jakowski wrote:
>  	if ((bio->bi_opf & REQ_NOWAIT) && !queue_is_mq(q))
> -		goto not_supported;
> +		if (!(bio->bi_opf & REQ_HIPRI))
> +			goto not_supported;

Can you explain this check?  This looks weird to me  I think we need
a generalized check if a make_request based driver supports REQ_NOWAIT
instead (and as a separate patch / patchset).

> +	if (q->bio_poll_fn != NULL) {
> +		state = current->state;
> +		do {
> +			int ret;
> +
> +			ret = q->bio_poll_fn(q, cookie);
> +			if (ret > 0) {
> +				__set_current_state(TASK_RUNNING);
> +				return ret;
> +			}
> +
> +			if (signal_pending_state(state, current))
> +				__set_current_state(TASK_RUNNING);
> +
> +			if (current->state == TASK_RUNNING)
> +				return 1;
> +			if (ret < 0 || !spin)
> +				break;
> +			cpu_relax();
> +		} while (!need_resched());
> +
> +		__set_current_state(TASK_RUNNING);
> +
> +		return 0;
> +	}

This needs to be factored out into a helper at least.

Re: [PATCH 1/2] block: introduce polling on bio level

[Christoph Hellwig]

On Wed, Jan 29, 2020 at 09:01:49PM -0700, Jens Axboe wrote:
> blk_poll() used to be a pointer in the queue, but since we just had
> one implementation, we got rid of it. Might make sense to
> reintroduce that, and just make it an optimized indirect call. I
> think that would be prettier than add the bio hack in the middle of
> it, and you're having to add a queue pointer anyway.

Well, the other reason is to avoid an indirect call for the blk-mq
case, which are fairly expensive.  In fact I'm pretty sure we avoid
indirect calls from the bio layer into blk-mq entirely for the fast
path at the moment, and it would be great to keep it that way.

Re: [PATCH 2/2] md: enable io polling

[Christoph Hellwig]

> +	rdev_for_each(rdev, mddev) {
> +		if (rdev->raid_disk >= 0 && !test_bit(Faulty, &rdev->flags)) {
> +			rv = blk_poll(bdev_get_queue(rdev->bdev), cookie, false);

This adds a > 80 char line.  But if you just use a continue to skip
the not allicable ones you even clean up the code while avoiding that.

Re: [PATCH 1/2] block: introduce polling on bio level

[Andrzej Jakowski]

On 1/30/20 11:34 PM, Christoph Hellwig wrote:
> Can you explain this check?  This looks weird to me  I think we need
> a generalized check if a make_request based driver supports REQ_NOWAIT
> instead (and as a separate patch / patchset).

Original check used to reject polled IO for stackable block devices as 
"not supported". To solve that situation I introduced additional check 
to reject all non REQ_HIPRI requests. That check is not intended to 
generalize, like you indicated, but to conservatively select which 
requests to accept.
Perhaps there is better way to do that. Any suggestions?

Re: [PATCH 1/2] block: introduce polling on bio level

[Jens Axboe]

On 1/30/20 11:35 PM, Christoph Hellwig wrote:
> On Wed, Jan 29, 2020 at 09:01:49PM -0700, Jens Axboe wrote:
>> blk_poll() used to be a pointer in the queue, but since we just had
>> one implementation, we got rid of it. Might make sense to
>> reintroduce that, and just make it an optimized indirect call. I
>> think that would be prettier than add the bio hack in the middle of
>> it, and you're having to add a queue pointer anyway.
> 
> Well, the other reason is to avoid an indirect call for the blk-mq
> case, which are fairly expensive.  In fact I'm pretty sure we avoid
> indirect calls from the bio layer into blk-mq entirely for the fast
> path at the moment, and it would be great to keep it that way.

Sure, my suggestion was to provide it as an alternative - if set,
then call that. Though with the optimized indirect calls, at least
it's just a branch, actual call is the same.

This patch sets a bit in no mans land right now. It's duplicating
the main loop, and it's shoved in the middle of the function. This
has to get cleaned up.

-- 
Jens Axboe

Re: [PATCH 1/2] block: introduce polling on bio level

[Christoph Hellwig]

On Fri, Jan 31, 2020 at 11:51:43AM -0700, Andrzej Jakowski wrote:
> On 1/30/20 11:34 PM, Christoph Hellwig wrote:
> > Can you explain this check?  This looks weird to me  I think we need
> > a generalized check if a make_request based driver supports REQ_NOWAIT
> > instead (and as a separate patch / patchset).
> 
> Original check used to reject polled IO for stackable block devices as "not
> supported". To solve that situation I introduced additional check to reject
> all non REQ_HIPRI requests. That check is not intended to generalize, like
> you indicated, but to conservatively select which requests to accept.
> Perhaps there is better way to do that. Any suggestions?

REQ_NOWAIT and REQ_HIPRI are completley unrelated concepts and they need
to be dealt with entirely separately.