[PATCH] null_blk: add 'requeue' fault attribute

[PATCH] null_blk: add 'requeue' fault attribute

[Jens Axboe]

Similarly to the support we have for testing/faking timeouts for
null_blk, this adds support for triggering a requeue condition.
Considering the issues around restart we've been seeing, this should be
a useful addition to the testing arsenal to ensure that we are handling
requeue conditions correctly.

This works for queue mode 1 (legacy request_fn based path) and 2 (blk-mq
path), as there's no good way to do requeue with a bio based driver.
This is similar to the timeout path.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

 null_blk.c |   55 +++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
index 287a09611c0f..363536572e19 100644
--- a/drivers/block/null_blk.c
+++ b/drivers/block/null_blk.c
@@ -29,6 +29,7 @@
 
 #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
 static DECLARE_FAULT_ATTR(null_timeout_attr);
+static DECLARE_FAULT_ATTR(null_requeue_attr);
 #endif
 
 static inline u64 mb_per_tick(int mbps)
@@ -170,6 +171,9 @@ MODULE_PARM_DESC(home_node, "Home node for the device");
 #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
 static char g_timeout_str[80];
 module_param_string(timeout, g_timeout_str, sizeof(g_timeout_str), S_IRUGO);
+
+static char g_requeue_str[80];
+module_param_string(requeue, g_requeue_str, sizeof(g_requeue_str), S_IRUGO);
 #endif
 
 static int g_queue_mode = NULL_Q_MQ;
@@ -1380,7 +1384,15 @@ static bool should_timeout_request(struct request *rq)
 	if (g_timeout_str[0])
 		return should_fail(&null_timeout_attr, 1);
 #endif
+	return false;
+}
 
+static bool should_requeue_request(struct request *rq)
+{
+#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
+	if (g_requeue_str[0])
+		return should_fail(&null_requeue_attr, 1);
+#endif
 	return false;
 }
 
@@ -1391,11 +1403,17 @@ static void null_request_fn(struct request_queue *q)
 	while ((rq = blk_fetch_request(q)) != NULL) {
 		struct nullb_cmd *cmd = rq->special;
 
-		if (!should_timeout_request(rq)) {
-			spin_unlock_irq(q->queue_lock);
-			null_handle_cmd(cmd);
-			spin_lock_irq(q->queue_lock);
+		/* just ignore the request */
+		if (should_timeout_request(rq))
+			continue;
+		if (should_requeue_request(rq)) {
+			blk_requeue_request(q, rq);
+			continue;
 		}
+
+		spin_unlock_irq(q->queue_lock);
+		null_handle_cmd(cmd);
+		spin_lock_irq(q->queue_lock);
 	}
 }
 
@@ -1422,10 +1440,12 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 	blk_mq_start_request(bd->rq);
 
-	if (!should_timeout_request(bd->rq))
-		return null_handle_cmd(cmd);
+	if (should_requeue_request(bd->rq))
+		return BLK_STS_RESOURCE;
+	if (should_timeout_request(bd->rq))
+		return BLK_STS_OK;
 
-	return BLK_STS_OK;
+	return null_handle_cmd(cmd);
 }
 
 static const struct blk_mq_ops null_mq_ops = {
@@ -1659,16 +1679,27 @@ static void null_validate_conf(struct nullb_device *dev)
 		dev->mbps = 0;
 }
 
-static bool null_setup_fault(void)
-{
 #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
-	if (!g_timeout_str[0])
+static bool __null_setup_fault(struct fault_attr *attr, char *str)
+{
+	if (!str[0])
 		return true;
 
-	if (!setup_fault_attr(&null_timeout_attr, g_timeout_str))
+	if (!setup_fault_attr(attr, str))
 		return false;
 
-	null_timeout_attr.verbose = 0;
+	attr->verbose = 0;
+	return true;
+}
+#endif
+
+static bool null_setup_fault(void)
+{
+#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
+	if (!__null_setup_fault(&null_timeout_attr, g_timeout_str))
+		return false;
+	if (!__null_setup_fault(&null_requeue_attr, g_requeue_str))
+		return false;
 #endif
 	return true;
 }

-- 
Jens Axboe

Re: [PATCH] null_blk: add 'requeue' fault attribute

[Bart Van Assche]

T24gVHVlLCAyMDE4LTAyLTI3IGF0IDE1OjM0IC0wNzAwLCBKZW5zIEF4Ym9lIHdyb3RlOg0KPiBT
aW1pbGFybHkgdG8gdGhlIHN1cHBvcnQgd2UgaGF2ZSBmb3IgdGVzdGluZy9mYWtpbmcgdGltZW91
dHMgZm9yDQo+IG51bGxfYmxrLCB0aGlzIGFkZHMgc3VwcG9ydCBmb3IgdHJpZ2dlcmluZyBhIHJl
cXVldWUgY29uZGl0aW9uLg0KPiBDb25zaWRlcmluZyB0aGUgaXNzdWVzIGFyb3VuZCByZXN0YXJ0
IHdlJ3ZlIGJlZW4gc2VlaW5nLCB0aGlzIHNob3VsZCBiZQ0KPiBhIHVzZWZ1bCBhZGRpdGlvbiB0
byB0aGUgdGVzdGluZyBhcnNlbmFsIHRvIGVuc3VyZSB0aGF0IHdlIGFyZSBoYW5kbGluZw0KPiBy
ZXF1ZXVlIGNvbmRpdGlvbnMgY29ycmVjdGx5Lg0KPiANCj4gVGhpcyB3b3JrcyBmb3IgcXVldWUg
bW9kZSAxIChsZWdhY3kgcmVxdWVzdF9mbiBiYXNlZCBwYXRoKSBhbmQgMiAoYmxrLW1xDQo+IHBh
dGgpLCBhcyB0aGVyZSdzIG5vIGdvb2Qgd2F5IHRvIGRvIHJlcXVldWUgd2l0aCBhIGJpbyBiYXNl
ZCBkcml2ZXIuDQo+IFRoaXMgaXMgc2ltaWxhciB0byB0aGUgdGltZW91dCBwYXRoLg0KDQpSZXZp
ZXdlZC1ieTogQmFydCBWYW4gQXNzY2hlIDxiYXJ0LnZhbmFzc2NoZUB3ZGMuY29tPg0KDQoNCg==

Re: [PATCH] null_blk: add 'requeue' fault attribute

[Omar Sandoval]

On Tue, Feb 27, 2018 at 03:34:53PM -0700, Jens Axboe wrote:
> Similarly to the support we have for testing/faking timeouts for
> null_blk, this adds support for triggering a requeue condition.
> Considering the issues around restart we've been seeing, this should be
> a useful addition to the testing arsenal to ensure that we are handling
> requeue conditions correctly.
> 
> This works for queue mode 1 (legacy request_fn based path) and 2 (blk-mq
> path), as there's no good way to do requeue with a bio based driver.
> This is similar to the timeout path.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
>  null_blk.c |   55 +++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
> index 287a09611c0f..363536572e19 100644
> --- a/drivers/block/null_blk.c
> +++ b/drivers/block/null_blk.c

[snip]

> @@ -1422,10 +1440,12 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
>  
>  	blk_mq_start_request(bd->rq);
>  
> -	if (!should_timeout_request(bd->rq))
> -		return null_handle_cmd(cmd);
> +	if (should_requeue_request(bd->rq))
> +		return BLK_STS_RESOURCE;

Hm, this goes through the less interesting requeue path, add to the
dispatch list and __blk_mq_requeue_request(). blk_mq_requeue_request()
is the one that I wanted to test since that's the one that needs to call
the scheduler hook.

Re: [PATCH] null_blk: add 'requeue' fault attribute

[Jens Axboe]

On 2/28/18 1:51 AM, Omar Sandoval wrote:
> On Tue, Feb 27, 2018 at 03:34:53PM -0700, Jens Axboe wrote:
>> Similarly to the support we have for testing/faking timeouts for
>> null_blk, this adds support for triggering a requeue condition.
>> Considering the issues around restart we've been seeing, this should be
>> a useful addition to the testing arsenal to ensure that we are handling
>> requeue conditions correctly.
>>
>> This works for queue mode 1 (legacy request_fn based path) and 2 (blk-mq
>> path), as there's no good way to do requeue with a bio based driver.
>> This is similar to the timeout path.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>>  null_blk.c |   55 +++++++++++++++++++++++++++++++++++++++++++------------
>>  1 file changed, 43 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
>> index 287a09611c0f..363536572e19 100644
>> --- a/drivers/block/null_blk.c
>> +++ b/drivers/block/null_blk.c
> 
> [snip]
> 
>> @@ -1422,10 +1440,12 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
>>  
>>  	blk_mq_start_request(bd->rq);
>>  
>> -	if (!should_timeout_request(bd->rq))
>> -		return null_handle_cmd(cmd);
>> +	if (should_requeue_request(bd->rq))
>> +		return BLK_STS_RESOURCE;
> 
> Hm, this goes through the less interesting requeue path, add to the
> dispatch list and __blk_mq_requeue_request(). blk_mq_requeue_request()
> is the one that I wanted to test since that's the one that needs to call
> the scheduler hook.

Until recently, it would have :-)

Both of them are interesting to test, though. Most of the core stall
cases would have been triggered by going through the STS_RESOURCE case.
How about we just make it exercise both? The below patch alternates
between them when we have chosen to requeue.


diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
index 287a09611c0f..d12d7a8325ad 100644
--- a/drivers/block/null_blk.c
+++ b/drivers/block/null_blk.c
@@ -29,6 +29,7 @@
 
 #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
 static DECLARE_FAULT_ATTR(null_timeout_attr);
+static DECLARE_FAULT_ATTR(null_requeue_attr);
 #endif
 
 static inline u64 mb_per_tick(int mbps)
@@ -53,6 +54,7 @@ struct nullb_queue {
 	wait_queue_head_t wait;
 	unsigned int queue_depth;
 	struct nullb_device *dev;
+	unsigned int requeue_selection;
 
 	struct nullb_cmd *cmds;
 };
@@ -170,6 +172,9 @@ MODULE_PARM_DESC(home_node, "Home node for the device");
 #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
 static char g_timeout_str[80];
 module_param_string(timeout, g_timeout_str, sizeof(g_timeout_str), S_IRUGO);
+
+static char g_requeue_str[80];
+module_param_string(requeue, g_requeue_str, sizeof(g_requeue_str), S_IRUGO);
 #endif
 
 static int g_queue_mode = NULL_Q_MQ;
@@ -1380,7 +1385,15 @@ static bool should_timeout_request(struct request *rq)
 	if (g_timeout_str[0])
 		return should_fail(&null_timeout_attr, 1);
 #endif
+	return false;
+}
 
+static bool should_requeue_request(struct request *rq)
+{
+#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
+	if (g_requeue_str[0])
+		return should_fail(&null_requeue_attr, 1);
+#endif
 	return false;
 }
 
@@ -1391,11 +1404,17 @@ static void null_request_fn(struct request_queue *q)
 	while ((rq = blk_fetch_request(q)) != NULL) {
 		struct nullb_cmd *cmd = rq->special;
 
-		if (!should_timeout_request(rq)) {
-			spin_unlock_irq(q->queue_lock);
-			null_handle_cmd(cmd);
-			spin_lock_irq(q->queue_lock);
+		/* just ignore the request */
+		if (should_timeout_request(rq))
+			continue;
+		if (should_requeue_request(rq)) {
+			blk_requeue_request(q, rq);
+			continue;
 		}
+
+		spin_unlock_irq(q->queue_lock);
+		null_handle_cmd(cmd);
+		spin_lock_irq(q->queue_lock);
 	}
 }
 
@@ -1422,10 +1441,23 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 	blk_mq_start_request(bd->rq);
 
-	if (!should_timeout_request(bd->rq))
-		return null_handle_cmd(cmd);
+	if (should_requeue_request(bd->rq)) {
+		/*
+		 * Alternate between hitting the core BUSY path, and the
+		 * driver driven requeue path
+		 */
+		nq->requeue_selection++;
+		if (nq->requeue_selection & 1)
+			return BLK_STS_RESOURCE;
+		else {
+			blk_mq_requeue_request(bd->rq, true);
+			return BLK_STS_OK;
+		}
+	}
+	if (should_timeout_request(bd->rq))
+		return BLK_STS_OK;
 
-	return BLK_STS_OK;
+	return null_handle_cmd(cmd);
 }
 
 static const struct blk_mq_ops null_mq_ops = {
@@ -1659,16 +1691,27 @@ static void null_validate_conf(struct nullb_device *dev)
 		dev->mbps = 0;
 }
 
-static bool null_setup_fault(void)
-{
 #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
-	if (!g_timeout_str[0])
+static bool __null_setup_fault(struct fault_attr *attr, char *str)
+{
+	if (!str[0])
 		return true;
 
-	if (!setup_fault_attr(&null_timeout_attr, g_timeout_str))
+	if (!setup_fault_attr(attr, str))
 		return false;
 
-	null_timeout_attr.verbose = 0;
+	attr->verbose = 0;
+	return true;
+}
+#endif
+
+static bool null_setup_fault(void)
+{
+#ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
+	if (!__null_setup_fault(&null_timeout_attr, g_timeout_str))
+		return false;
+	if (!__null_setup_fault(&null_requeue_attr, g_requeue_str))
+		return false;
 #endif
 	return true;
 }

-- 
Jens Axboe

Re: [PATCH] null_blk: add 'requeue' fault attribute

[Omar Sandoval]

On Wed, Feb 28, 2018 at 08:28:25AM -0700, Jens Axboe wrote:
> On 2/28/18 1:51 AM, Omar Sandoval wrote:
> > On Tue, Feb 27, 2018 at 03:34:53PM -0700, Jens Axboe wrote:
> >> Similarly to the support we have for testing/faking timeouts for
> >> null_blk, this adds support for triggering a requeue condition.
> >> Considering the issues around restart we've been seeing, this should be
> >> a useful addition to the testing arsenal to ensure that we are handling
> >> requeue conditions correctly.
> >>
> >> This works for queue mode 1 (legacy request_fn based path) and 2 (blk-mq
> >> path), as there's no good way to do requeue with a bio based driver.
> >> This is similar to the timeout path.
> >>
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >>
> >> ---
> >>
> >>  null_blk.c |   55 +++++++++++++++++++++++++++++++++++++++++++------------
> >>  1 file changed, 43 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
> >> index 287a09611c0f..363536572e19 100644
> >> --- a/drivers/block/null_blk.c
> >> +++ b/drivers/block/null_blk.c
> > 
> > [snip]
> > 
> >> @@ -1422,10 +1440,12 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
> >>  
> >>  	blk_mq_start_request(bd->rq);
> >>  
> >> -	if (!should_timeout_request(bd->rq))
> >> -		return null_handle_cmd(cmd);
> >> +	if (should_requeue_request(bd->rq))
> >> +		return BLK_STS_RESOURCE;
> > 
> > Hm, this goes through the less interesting requeue path, add to the
> > dispatch list and __blk_mq_requeue_request(). blk_mq_requeue_request()
> > is the one that I wanted to test since that's the one that needs to call
> > the scheduler hook.
> 
> Until recently, it would have :-)
> 
> Both of them are interesting to test, though. Most of the core stall
> cases would have been triggered by going through the STS_RESOURCE case.
> How about we just make it exercise both? The below patch alternates
> between them when we have chosen to requeue.

Works for me. One idle thought, if we set this up to always requeue,
then it won't make any progress. Maybe we should limit the number of
times each request can be requeued so people (me) don't lock up their
test systems? Either way,

Reviewed-by: Omar Sandoval <osandov@fb.com>

Re: [PATCH] null_blk: add 'requeue' fault attribute

[Jens Axboe]

On 2/28/18 9:14 AM, Omar Sandoval wrote:
> On Wed, Feb 28, 2018 at 08:28:25AM -0700, Jens Axboe wrote:
>> On 2/28/18 1:51 AM, Omar Sandoval wrote:
>>> On Tue, Feb 27, 2018 at 03:34:53PM -0700, Jens Axboe wrote:
>>>> Similarly to the support we have for testing/faking timeouts for
>>>> null_blk, this adds support for triggering a requeue condition.
>>>> Considering the issues around restart we've been seeing, this should be
>>>> a useful addition to the testing arsenal to ensure that we are handling
>>>> requeue conditions correctly.
>>>>
>>>> This works for queue mode 1 (legacy request_fn based path) and 2 (blk-mq
>>>> path), as there's no good way to do requeue with a bio based driver.
>>>> This is similar to the timeout path.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>
>>>> ---
>>>>
>>>>  null_blk.c |   55 +++++++++++++++++++++++++++++++++++++++++++------------
>>>>  1 file changed, 43 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
>>>> index 287a09611c0f..363536572e19 100644
>>>> --- a/drivers/block/null_blk.c
>>>> +++ b/drivers/block/null_blk.c
>>>
>>> [snip]
>>>
>>>> @@ -1422,10 +1440,12 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>>  
>>>>  	blk_mq_start_request(bd->rq);
>>>>  
>>>> -	if (!should_timeout_request(bd->rq))
>>>> -		return null_handle_cmd(cmd);
>>>> +	if (should_requeue_request(bd->rq))
>>>> +		return BLK_STS_RESOURCE;
>>>
>>> Hm, this goes through the less interesting requeue path, add to the
>>> dispatch list and __blk_mq_requeue_request(). blk_mq_requeue_request()
>>> is the one that I wanted to test since that's the one that needs to call
>>> the scheduler hook.
>>
>> Until recently, it would have :-)
>>
>> Both of them are interesting to test, though. Most of the core stall
>> cases would have been triggered by going through the STS_RESOURCE case.
>> How about we just make it exercise both? The below patch alternates
>> between them when we have chosen to requeue.
> 
> Works for me. One idle thought, if we set this up to always requeue,
> then it won't make any progress. Maybe we should limit the number of
> times each request can be requeued so people (me) don't lock up their
> test systems? Either way,

Dunno, that gets into the "doctor it hurts when I shoot myself in the
foot" territory. Same can be said for the timeout setting. I think we
should just ignore that.

-- 
Jens Axboe

Re: [PATCH] null_blk: add 'requeue' fault attribute

[Omar Sandoval]

On Wed, Feb 28, 2018 at 09:15:37AM -0700, Jens Axboe wrote:
> On 2/28/18 9:14 AM, Omar Sandoval wrote:
> > On Wed, Feb 28, 2018 at 08:28:25AM -0700, Jens Axboe wrote:
> >> On 2/28/18 1:51 AM, Omar Sandoval wrote:
> >>> On Tue, Feb 27, 2018 at 03:34:53PM -0700, Jens Axboe wrote:
> >>>> Similarly to the support we have for testing/faking timeouts for
> >>>> null_blk, this adds support for triggering a requeue condition.
> >>>> Considering the issues around restart we've been seeing, this should be
> >>>> a useful addition to the testing arsenal to ensure that we are handling
> >>>> requeue conditions correctly.
> >>>>
> >>>> This works for queue mode 1 (legacy request_fn based path) and 2 (blk-mq
> >>>> path), as there's no good way to do requeue with a bio based driver.
> >>>> This is similar to the timeout path.
> >>>>
> >>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >>>>
> >>>> ---
> >>>>
> >>>>  null_blk.c |   55 +++++++++++++++++++++++++++++++++++++++++++------------
> >>>>  1 file changed, 43 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
> >>>> index 287a09611c0f..363536572e19 100644
> >>>> --- a/drivers/block/null_blk.c
> >>>> +++ b/drivers/block/null_blk.c
> >>>
> >>> [snip]
> >>>
> >>>> @@ -1422,10 +1440,12 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
> >>>>  
> >>>>  	blk_mq_start_request(bd->rq);
> >>>>  
> >>>> -	if (!should_timeout_request(bd->rq))
> >>>> -		return null_handle_cmd(cmd);
> >>>> +	if (should_requeue_request(bd->rq))
> >>>> +		return BLK_STS_RESOURCE;
> >>>
> >>> Hm, this goes through the less interesting requeue path, add to the
> >>> dispatch list and __blk_mq_requeue_request(). blk_mq_requeue_request()
> >>> is the one that I wanted to test since that's the one that needs to call
> >>> the scheduler hook.
> >>
> >> Until recently, it would have :-)
> >>
> >> Both of them are interesting to test, though. Most of the core stall
> >> cases would have been triggered by going through the STS_RESOURCE case.
> >> How about we just make it exercise both? The below patch alternates
> >> between them when we have chosen to requeue.
> > 
> > Works for me. One idle thought, if we set this up to always requeue,
> > then it won't make any progress. Maybe we should limit the number of
> > times each request can be requeued so people (me) don't lock up their
> > test systems? Either way,
> 
> Dunno, that gets into the "doctor it hurts when I shoot myself in the
> foot" territory. Same can be said for the timeout setting. I think we
> should just ignore that.

Ack, fine with me.