[5/8] nowait aio: return on congested block device

Commit Message

Sagi Grimberg March 8, 2017, 7:03 a.m. UTC

> -		if (likely(blk_queue_enter(q, false) == 0)) {
> +		if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT)) == 0)) {
>  			ret = q->make_request_fn(q, bio);

I think that for ->make_request to not block we'd need to set
BLK_MQ_REQ_NOWAIT in blk_mq_alloc_data to avoid blocking on a tag
allocation.

Something like the untested addition below:
--
--
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Goldwyn Rodrigues March 8, 2017, 3 p.m. UTC | #1

On 03/08/2017 01:03 AM, Sagi Grimberg wrote:
> 
>> -        if (likely(blk_queue_enter(q, false) == 0)) {
>> +        if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT))
>> == 0)) {
>>              ret = q->make_request_fn(q, bio);
> 
> I think that for ->make_request to not block we'd need to set
> BLK_MQ_REQ_NOWAIT in blk_mq_alloc_data to avoid blocking on a tag
> allocation.
> 
> Something like the untested addition below:

I did that in the first series, but there are too many reasons to block
in blk-mq [1]. I dropped blk-mq work in v2.

[1] https://patchwork.kernel.org/patch/9571051/

Jan Kara March 8, 2017, 3:28 p.m. UTC | #2

On Wed 08-03-17 09:00:09, Goldwyn Rodrigues wrote:
> 
> 
> On 03/08/2017 01:03 AM, Sagi Grimberg wrote:
> > 
> >> -        if (likely(blk_queue_enter(q, false) == 0)) {
> >> +        if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT))
> >> == 0)) {
> >>              ret = q->make_request_fn(q, bio);
> > 
> > I think that for ->make_request to not block we'd need to set
> > BLK_MQ_REQ_NOWAIT in blk_mq_alloc_data to avoid blocking on a tag
> > allocation.
> > 
> > Something like the untested addition below:
> 
> I did that in the first series, but there are too many reasons to block
> in blk-mq [1]. I dropped blk-mq work in v2.
> 
> [1] https://patchwork.kernel.org/patch/9571051/

Well, that's not really good. If we cannot support this for both blk-mq and
legacy block layer the feature will not be usable. So please work on blk-mq
support as well.

								Honza

Christoph Hellwig March 8, 2017, 3:51 p.m. UTC | #3

On Wed, Mar 08, 2017 at 04:28:06PM +0100, Jan Kara wrote:
> Well, that's not really good. If we cannot support this for both blk-mq and
> legacy block layer the feature will not be usable. So please work on blk-mq
> support as well.

Exactly.  In addition to that anything implementing a feature for the
legacy request request code but not blk-mq is grounds for an instant
NAK.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jens Axboe March 8, 2017, 4:17 p.m. UTC | #4

On 03/08/2017 08:00 AM, Goldwyn Rodrigues wrote:
> 
> 
> On 03/08/2017 01:03 AM, Sagi Grimberg wrote:
>>
>>> -        if (likely(blk_queue_enter(q, false) == 0)) {
>>> +        if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT))
>>> == 0)) {
>>>              ret = q->make_request_fn(q, bio);
>>
>> I think that for ->make_request to not block we'd need to set
>> BLK_MQ_REQ_NOWAIT in blk_mq_alloc_data to avoid blocking on a tag
>> allocation.
>>
>> Something like the untested addition below:
> 
> I did that in the first series, but there are too many reasons to block
> in blk-mq [1]. I dropped blk-mq work in v2.

That's complete nonsense, there are no more places in blk-mq that will
block that in the legacy path. Most of the examples from your URL:

> [1] https://patchwork.kernel.org/patch/9571051/

are not blk-mq, but writeback throttling, and drivers that explicitly
hook into ->make_request_fn.

As others have mentioned, it's a total non-starter to focus on the
deprecated IO path and just ignore the new one. Back to the drawing
board.

Goldwyn Rodrigues March 9, 2017, 2:18 a.m. UTC | #5

On 03/08/2017 10:17 AM, Jens Axboe wrote:
> On 03/08/2017 08:00 AM, Goldwyn Rodrigues wrote:
>>
>>
>> On 03/08/2017 01:03 AM, Sagi Grimberg wrote:
>>>
>>>> -        if (likely(blk_queue_enter(q, false) == 0)) {
>>>> +        if (likely(blk_queue_enter(q, bio_flagged(bio, BIO_NOWAIT))
>>>> == 0)) {
>>>>              ret = q->make_request_fn(q, bio);
>>>
>>> I think that for ->make_request to not block we'd need to set
>>> BLK_MQ_REQ_NOWAIT in blk_mq_alloc_data to avoid blocking on a tag
>>> allocation.
>>>
>>> Something like the untested addition below:
>>
>> I did that in the first series, but there are too many reasons to block
>> in blk-mq [1]. I dropped blk-mq work in v2.
> 
> That's complete nonsense, there are no more places in blk-mq that will
> block that in the legacy path. Most of the examples from your URL:
> 
>> [1] https://patchwork.kernel.org/patch/9571051/
> 
> are not blk-mq, but writeback throttling, and drivers that explicitly
> hook into ->make_request_fn.
> 
> As others have mentioned, it's a total non-starter to focus on the
> deprecated IO path and just ignore the new one. Back to the drawing
> board.
> 

Thanks, I understood what I was doing wrong. I will include blk-mq in
the patch.

Patch

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 09af8ff18719..40e78b57fb44 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -119,6 +119,9 @@  struct request *blk_mq_sched_get_request(struct 
request_queue *q,
         if (likely(!data->hctx))
                 data->hctx = blk_mq_map_queue(q, data->ctx->cpu);

+       if (likely(bio) && bio_flagged(bio, BIO_NOWAIT))
+               data->flags |= BLK_MQ_REQ_NOWAIT;
+
         if (e) {
                 data->flags |= BLK_MQ_REQ_INTERNAL;