All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux
@ 2014-08-23 13:47 Thorsten Knabe
  2014-08-23 15:34 ` Richard Weinberger
  0 siblings, 1 reply; 9+ messages in thread
From: Thorsten Knabe @ 2014-08-23 13:47 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: linux-kernel, Thomas Meyer, Valdis.Kletnieks, linux

From: Thorsten Knabe <linux@thorsten-knabe.de>

UML: UBD: Fix for processes stuck in D state forever in UserModeLinux.

Starting with Linux 3.12 processes get stuck in D state forever in
UserModeLinux under sync heavy workloads. This bug was introduced by
commit 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport).
Fix bug by adding a check if FLUSH request was successfully submitted to
the I/O thread and keeping the FLUSH request on the request queue on
submission failures.

Fixes: 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
Signed-off-by: Thorsten Knabe <linux@thorsten-knabe.de>
---
Patch applies to 3.16.1.

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 3716e69..b7d2840 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)

 	while(1){
 		struct ubd *dev = q->queuedata;
-		if(dev->end_sg == 0){
+		if(dev->request == NULL){
 			struct request *req = blk_fetch_request(q);
 			if(req == NULL)
 				return;
@@ -1299,7 +1299,8 @@ static void do_ubd_request(struct request_queue *q)
 				return;
 			}
 			prepare_flush_request(req, io_req);
-			submit_request(io_req, dev);
+			if (submit_request(io_req, dev) == false)
+				return;
 		}

 		while(dev->start_sg < dev->end_sg){




-- 
___
 |        | /                 E-Mail: linux@thorsten-knabe.de
 |horsten |/\nabe                WWW: http://linux.thorsten-knabe.de

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux
  2014-08-23 13:47 [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux Thorsten Knabe
@ 2014-08-23 15:34 ` Richard Weinberger
  2014-08-23 17:43   ` Thorsten Knabe
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Weinberger @ 2014-08-23 15:34 UTC (permalink / raw)
  To: Thorsten Knabe; +Cc: linux-kernel, Thomas Meyer, Valdis.Kletnieks

Hi!

Am 23.08.2014 15:47, schrieb Thorsten Knabe:
> From: Thorsten Knabe <linux@thorsten-knabe.de>
> 
> UML: UBD: Fix for processes stuck in D state forever in UserModeLinux.
> 
> Starting with Linux 3.12 processes get stuck in D state forever in
> UserModeLinux under sync heavy workloads. This bug was introduced by
> commit 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport).
> Fix bug by adding a check if FLUSH request was successfully submitted to
> the I/O thread and keeping the FLUSH request on the request queue on
> submission failures.
> 
> Fixes: 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
> Signed-off-by: Thorsten Knabe <linux@thorsten-knabe.de>

Thanks a lot for hunting this issue down.

> ---
> Patch applies to 3.16.1.
> 
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index 3716e69..b7d2840 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
> 
>  	while(1){
>  		struct ubd *dev = q->queuedata;
> -		if(dev->end_sg == 0){
> +		if(dev->request == NULL){

Why do we need this specific change?

Thanks,
//richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux
  2014-08-23 15:34 ` Richard Weinberger
@ 2014-08-23 17:43   ` Thorsten Knabe
  2014-08-24 12:11     ` Richard Weinberger
  0 siblings, 1 reply; 9+ messages in thread
From: Thorsten Knabe @ 2014-08-23 17:43 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: linux-kernel, Thomas Meyer, Valdis.Kletnieks

Hi Richard.

On 08/23/2014 05:34 PM, Richard Weinberger wrote:
> Hi!
> 
> Am 23.08.2014 15:47, schrieb Thorsten Knabe:
>> From: Thorsten Knabe <linux@thorsten-knabe.de>
>>
>> UML: UBD: Fix for processes stuck in D state forever in UserModeLinux.
>>
>> Starting with Linux 3.12 processes get stuck in D state forever in
>> UserModeLinux under sync heavy workloads. This bug was introduced by
>> commit 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport).
>> Fix bug by adding a check if FLUSH request was successfully submitted to
>> the I/O thread and keeping the FLUSH request on the request queue on
>> submission failures.
>>
>> Fixes: 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
>> Signed-off-by: Thorsten Knabe <linux@thorsten-knabe.de>
> 
> Thanks a lot for hunting this issue down.
> 
>> ---
>> Patch applies to 3.16.1.
>>
>> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
>> index 3716e69..b7d2840 100644
>> --- a/arch/um/drivers/ubd_kern.c
>> +++ b/arch/um/drivers/ubd_kern.c
>> @@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
>>
>>  	while(1){
>>  		struct ubd *dev = q->queuedata;
>> -		if(dev->end_sg == 0){
>> +		if(dev->request == NULL){
> 
> Why do we need this specific change?

This change is required, because for FLUSH requests dev->end_sg is
initialized to 0 by blk_rq_map_sg() a few lines above, as FLUSH requests
have no data blocks attached to themselves.

Checking for dev->end_sg == 0 would then replace a not yet submitted
FLUSH request by the next request on the next iteration of the while
loop, also the FLUSH request was scheduled for resubmission to the I/O
thread.

Kind regards
Thorsten

> 
> Thanks,
> //richard
> 


-- 
___
 |        | /                 E-Mail: linux@thorsten-knabe.de
 |horsten |/\nabe                WWW: http://linux.thorsten-knabe.de

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux
  2014-08-23 17:43   ` Thorsten Knabe
@ 2014-08-24 12:11     ` Richard Weinberger
  2014-08-24 23:02       ` Thorsten Knabe
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Weinberger @ 2014-08-24 12:11 UTC (permalink / raw)
  To: Thorsten Knabe; +Cc: linux-kernel, Thomas Meyer, Valdis.Kletnieks

Am 23.08.2014 19:43, schrieb Thorsten Knabe:
> Hi Richard.
> 
> On 08/23/2014 05:34 PM, Richard Weinberger wrote:
>> Hi!
>>
>> Am 23.08.2014 15:47, schrieb Thorsten Knabe:
>>> From: Thorsten Knabe <linux@thorsten-knabe.de>
>>>
>>> UML: UBD: Fix for processes stuck in D state forever in UserModeLinux.
>>>
>>> Starting with Linux 3.12 processes get stuck in D state forever in
>>> UserModeLinux under sync heavy workloads. This bug was introduced by
>>> commit 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport).
>>> Fix bug by adding a check if FLUSH request was successfully submitted to
>>> the I/O thread and keeping the FLUSH request on the request queue on
>>> submission failures.
>>>
>>> Fixes: 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
>>> Signed-off-by: Thorsten Knabe <linux@thorsten-knabe.de>
>>
>> Thanks a lot for hunting this issue down.
>>
>>> ---
>>> Patch applies to 3.16.1.
>>>
>>> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
>>> index 3716e69..b7d2840 100644
>>> --- a/arch/um/drivers/ubd_kern.c
>>> +++ b/arch/um/drivers/ubd_kern.c
>>> @@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
>>>
>>>  	while(1){
>>>  		struct ubd *dev = q->queuedata;
>>> -		if(dev->end_sg == 0){
>>> +		if(dev->request == NULL){
>>
>> Why do we need this specific change?
> 
> This change is required, because for FLUSH requests dev->end_sg is
> initialized to 0 by blk_rq_map_sg() a few lines above, as FLUSH requests
> have no data blocks attached to themselves.

You meant "below"? Looks like I really miss something here.
At the bottom of the while(1) loop we have
                dev->end_sg = 0;
                dev->request = NULL;

Thanks,
//richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux
  2014-08-24 12:11     ` Richard Weinberger
@ 2014-08-24 23:02       ` Thorsten Knabe
  2014-08-25 13:25         ` Richard Weinberger
  0 siblings, 1 reply; 9+ messages in thread
From: Thorsten Knabe @ 2014-08-24 23:02 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: linux-kernel, Thomas Meyer, Valdis.Kletnieks

On 08/24/2014 02:11 PM, Richard Weinberger wrote:
> Am 23.08.2014 19:43, schrieb Thorsten Knabe:
>> Hi Richard.
>>
>> On 08/23/2014 05:34 PM, Richard Weinberger wrote:
>>> Hi!
>>>
>>> Am 23.08.2014 15:47, schrieb Thorsten Knabe:
>>>> From: Thorsten Knabe <linux@thorsten-knabe.de>
>>>>
>>>> UML: UBD: Fix for processes stuck in D state forever in UserModeLinux.
>>>>
>>>> Starting with Linux 3.12 processes get stuck in D state forever in
>>>> UserModeLinux under sync heavy workloads. This bug was introduced by
>>>> commit 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport).
>>>> Fix bug by adding a check if FLUSH request was successfully submitted to
>>>> the I/O thread and keeping the FLUSH request on the request queue on
>>>> submission failures.
>>>>
>>>> Fixes: 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
>>>> Signed-off-by: Thorsten Knabe <linux@thorsten-knabe.de>
>>>
>>> Thanks a lot for hunting this issue down.
>>>
>>>> ---
>>>> Patch applies to 3.16.1.
>>>>
>>>> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
>>>> index 3716e69..b7d2840 100644
>>>> --- a/arch/um/drivers/ubd_kern.c
>>>> +++ b/arch/um/drivers/ubd_kern.c
>>>> @@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
>>>>
>>>>  	while(1){
>>>>  		struct ubd *dev = q->queuedata;
>>>> -		if(dev->end_sg == 0){
>>>> +		if(dev->request == NULL){
>>>
>>> Why do we need this specific change?
>>
>> This change is required, because for FLUSH requests dev->end_sg is
>> initialized to 0 by blk_rq_map_sg() a few lines above, as FLUSH requests
>> have no data blocks attached to themselves.
> 
> You meant "below"? Looks like I really miss something here.
> At the bottom of the while(1) loop we have
>                 dev->end_sg = 0;
>                 dev->request = NULL;

No. The problematic line is:
                        dev->end_sg = blk_rq_map_sg(q, req, dev->sg);
and blk_rq_map_sg() returning 0 for REQ_FLUSH requests, because they
have no associated data blocks.

Hence on the next iteration of the while(1) loop:
                if(dev->end_sg == 0){
will be true, even if the request has not been successfully submitted to
the I/O thread in the previous iteration of the while(1) loop and a new
request will be fetched:
                        struct request *req = blk_fetch_request(q);
                        if(req == NULL)
                                return;

                        dev->request = req;
                        dev->rq_pos = blk_rq_pos(req);
                        dev->start_sg = 0;
                        dev->end_sg = blk_rq_map_sg(q, req, dev->sg);
                }

Thus the REQ_FLUSH request got lost and will never get submitted to the
I/O thread, there will be no matching answer from the I/O thread and the
lost REQ_FLUSH request will never complete...

Regards
Thorsten

> 
> Thanks,
> //richard
> 


-- 
___
 |        | /                 E-Mail: linux@thorsten-knabe.de
 |horsten |/\nabe                WWW: http://linux.thorsten-knabe.de

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux
  2014-08-24 23:02       ` Thorsten Knabe
@ 2014-08-25 13:25         ` Richard Weinberger
  2014-08-25 14:00           ` Thorsten Knabe
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Weinberger @ 2014-08-25 13:25 UTC (permalink / raw)
  To: Thorsten Knabe; +Cc: linux-kernel, Thomas Meyer, Valdis.Kletnieks

Am 25.08.2014 01:02, schrieb Thorsten Knabe:
> On 08/24/2014 02:11 PM, Richard Weinberger wrote:
>> Am 23.08.2014 19:43, schrieb Thorsten Knabe:
>>> Hi Richard.
>>>
>>> On 08/23/2014 05:34 PM, Richard Weinberger wrote:
>>>> Hi!
>>>>
>>>> Am 23.08.2014 15:47, schrieb Thorsten Knabe:
>>>>> From: Thorsten Knabe <linux@thorsten-knabe.de>
>>>>>
>>>>> UML: UBD: Fix for processes stuck in D state forever in UserModeLinux.
>>>>>
>>>>> Starting with Linux 3.12 processes get stuck in D state forever in
>>>>> UserModeLinux under sync heavy workloads. This bug was introduced by
>>>>> commit 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport).
>>>>> Fix bug by adding a check if FLUSH request was successfully submitted to
>>>>> the I/O thread and keeping the FLUSH request on the request queue on
>>>>> submission failures.
>>>>>
>>>>> Fixes: 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
>>>>> Signed-off-by: Thorsten Knabe <linux@thorsten-knabe.de>
>>>>
>>>> Thanks a lot for hunting this issue down.
>>>>
>>>>> ---
>>>>> Patch applies to 3.16.1.
>>>>>
>>>>> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
>>>>> index 3716e69..b7d2840 100644
>>>>> --- a/arch/um/drivers/ubd_kern.c
>>>>> +++ b/arch/um/drivers/ubd_kern.c
>>>>> @@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
>>>>>
>>>>>  	while(1){
>>>>>  		struct ubd *dev = q->queuedata;
>>>>> -		if(dev->end_sg == 0){
>>>>> +		if(dev->request == NULL){
>>>>
>>>> Why do we need this specific change?
>>>
>>> This change is required, because for FLUSH requests dev->end_sg is
>>> initialized to 0 by blk_rq_map_sg() a few lines above, as FLUSH requests
>>> have no data blocks attached to themselves.
>>
>> You meant "below"? Looks like I really miss something here.
>> At the bottom of the while(1) loop we have
>>                 dev->end_sg = 0;
>>                 dev->request = NULL;
> 
> No. The problematic line is:
>                         dev->end_sg = blk_rq_map_sg(q, req, dev->sg);
> and blk_rq_map_sg() returning 0 for REQ_FLUSH requests, because they
> have no associated data blocks.
> 
> Hence on the next iteration of the while(1) loop:
>                 if(dev->end_sg == 0){

At the bottom of the while loop dev->end_sg will be set to 0 anyway, this is what puzzles
me so hard.
(And dev->request too)

Thanks,
//richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux
  2014-08-25 13:25         ` Richard Weinberger
@ 2014-08-25 14:00           ` Thorsten Knabe
  2014-08-25 14:04             ` Richard Weinberger
  0 siblings, 1 reply; 9+ messages in thread
From: Thorsten Knabe @ 2014-08-25 14:00 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: linux-kernel, Thomas Meyer, Valdis.Kletnieks

On 08/25/2014 03:25 PM, Richard Weinberger wrote:
> Am 25.08.2014 01:02, schrieb Thorsten Knabe:
>> On 08/24/2014 02:11 PM, Richard Weinberger wrote:
>>> Am 23.08.2014 19:43, schrieb Thorsten Knabe:
>>>> Hi Richard.
>>>>
>>>> On 08/23/2014 05:34 PM, Richard Weinberger wrote:
>>>>> Hi!
>>>>>
>>>>> Am 23.08.2014 15:47, schrieb Thorsten Knabe:
>>>>>> From: Thorsten Knabe <linux@thorsten-knabe.de>
>>>>>>
>>>>>> UML: UBD: Fix for processes stuck in D state forever in UserModeLinux.
>>>>>>
>>>>>> Starting with Linux 3.12 processes get stuck in D state forever in
>>>>>> UserModeLinux under sync heavy workloads. This bug was introduced by
>>>>>> commit 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport).
>>>>>> Fix bug by adding a check if FLUSH request was successfully submitted to
>>>>>> the I/O thread and keeping the FLUSH request on the request queue on
>>>>>> submission failures.
>>>>>>
>>>>>> Fixes: 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
>>>>>> Signed-off-by: Thorsten Knabe <linux@thorsten-knabe.de>
>>>>>
>>>>> Thanks a lot for hunting this issue down.
>>>>>
>>>>>> ---
>>>>>> Patch applies to 3.16.1.
>>>>>>
>>>>>> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
>>>>>> index 3716e69..b7d2840 100644
>>>>>> --- a/arch/um/drivers/ubd_kern.c
>>>>>> +++ b/arch/um/drivers/ubd_kern.c
>>>>>> @@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
>>>>>>
>>>>>>  	while(1){
>>>>>>  		struct ubd *dev = q->queuedata;
>>>>>> -		if(dev->end_sg == 0){
>>>>>> +		if(dev->request == NULL){
>>>>>
>>>>> Why do we need this specific change?
>>>>
>>>> This change is required, because for FLUSH requests dev->end_sg is
>>>> initialized to 0 by blk_rq_map_sg() a few lines above, as FLUSH requests
>>>> have no data blocks attached to themselves.
>>>
>>> You meant "below"? Looks like I really miss something here.
>>> At the bottom of the while(1) loop we have
>>>                 dev->end_sg = 0;
>>>                 dev->request = NULL;
>>
>> No. The problematic line is:
>>                         dev->end_sg = blk_rq_map_sg(q, req, dev->sg);
>> and blk_rq_map_sg() returning 0 for REQ_FLUSH requests, because they
>> have no associated data blocks.
>>
>> Hence on the next iteration of the while(1) loop:
>>                 if(dev->end_sg == 0){
> 
> At the bottom of the while loop dev->end_sg will be set to 0 anyway, this is what puzzles
> me so hard.
> (And dev->request too)

Yes, but the statements:
                dev->end_sg = 0;
                dev->request = NULL;
at the end of the while loop will only be reached (for FLUSH requests
only with my patch applied) when submit_request() succeeds. Otherwise
the function do_ubd_request() returns early leaving dev->end_sg and
dev->request untouched.

> 
> Thanks,
> //richard
> 


-- 
___
 |        | /                 E-Mail: linux@thorsten-knabe.de
 |horsten |/\nabe                WWW: http://linux.thorsten-knabe.de

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux
  2014-08-25 14:00           ` Thorsten Knabe
@ 2014-08-25 14:04             ` Richard Weinberger
  2014-09-16 16:50               ` Richard Weinberger
  0 siblings, 1 reply; 9+ messages in thread
From: Richard Weinberger @ 2014-08-25 14:04 UTC (permalink / raw)
  To: Thorsten Knabe; +Cc: linux-kernel, Thomas Meyer, Valdis.Kletnieks

Am 25.08.2014 16:00, schrieb Thorsten Knabe:
> On 08/25/2014 03:25 PM, Richard Weinberger wrote:
>> Am 25.08.2014 01:02, schrieb Thorsten Knabe:
>>> On 08/24/2014 02:11 PM, Richard Weinberger wrote:
>>>> Am 23.08.2014 19:43, schrieb Thorsten Knabe:
>>>>> Hi Richard.
>>>>>
>>>>> On 08/23/2014 05:34 PM, Richard Weinberger wrote:
>>>>>> Hi!
>>>>>>
>>>>>> Am 23.08.2014 15:47, schrieb Thorsten Knabe:
>>>>>>> From: Thorsten Knabe <linux@thorsten-knabe.de>
>>>>>>>
>>>>>>> UML: UBD: Fix for processes stuck in D state forever in UserModeLinux.
>>>>>>>
>>>>>>> Starting with Linux 3.12 processes get stuck in D state forever in
>>>>>>> UserModeLinux under sync heavy workloads. This bug was introduced by
>>>>>>> commit 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport).
>>>>>>> Fix bug by adding a check if FLUSH request was successfully submitted to
>>>>>>> the I/O thread and keeping the FLUSH request on the request queue on
>>>>>>> submission failures.
>>>>>>>
>>>>>>> Fixes: 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
>>>>>>> Signed-off-by: Thorsten Knabe <linux@thorsten-knabe.de>
>>>>>>
>>>>>> Thanks a lot for hunting this issue down.
>>>>>>
>>>>>>> ---
>>>>>>> Patch applies to 3.16.1.
>>>>>>>
>>>>>>> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
>>>>>>> index 3716e69..b7d2840 100644
>>>>>>> --- a/arch/um/drivers/ubd_kern.c
>>>>>>> +++ b/arch/um/drivers/ubd_kern.c
>>>>>>> @@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
>>>>>>>
>>>>>>>  	while(1){
>>>>>>>  		struct ubd *dev = q->queuedata;
>>>>>>> -		if(dev->end_sg == 0){
>>>>>>> +		if(dev->request == NULL){
>>>>>>
>>>>>> Why do we need this specific change?
>>>>>
>>>>> This change is required, because for FLUSH requests dev->end_sg is
>>>>> initialized to 0 by blk_rq_map_sg() a few lines above, as FLUSH requests
>>>>> have no data blocks attached to themselves.
>>>>
>>>> You meant "below"? Looks like I really miss something here.
>>>> At the bottom of the while(1) loop we have
>>>>                 dev->end_sg = 0;
>>>>                 dev->request = NULL;
>>>
>>> No. The problematic line is:
>>>                         dev->end_sg = blk_rq_map_sg(q, req, dev->sg);
>>> and blk_rq_map_sg() returning 0 for REQ_FLUSH requests, because they
>>> have no associated data blocks.
>>>
>>> Hence on the next iteration of the while(1) loop:
>>>                 if(dev->end_sg == 0){
>>
>> At the bottom of the while loop dev->end_sg will be set to 0 anyway, this is what puzzles
>> me so hard.
>> (And dev->request too)
> 
> Yes, but the statements:
>                 dev->end_sg = 0;
>                 dev->request = NULL;
> at the end of the while loop will only be reached (for FLUSH requests
> only with my patch applied) when submit_request() succeeds. Otherwise
> the function do_ubd_request() returns early leaving dev->end_sg and
> dev->request untouched.

Can you please add this info into the commit message, I'm sure others get also
confused by that.^^
The logic in do_ubd_request() royal mess which needs fixing too. :-\

Thanks,
//richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux
  2014-08-25 14:04             ` Richard Weinberger
@ 2014-09-16 16:50               ` Richard Weinberger
  0 siblings, 0 replies; 9+ messages in thread
From: Richard Weinberger @ 2014-09-16 16:50 UTC (permalink / raw)
  To: Richard Weinberger; +Cc: Thorsten Knabe, LKML, Thomas Meyer, Valdis Kletnieks

On Mon, Aug 25, 2014 at 4:04 PM, Richard Weinberger <richard@nod.at> wrote:
> Am 25.08.2014 16:00, schrieb Thorsten Knabe:
>> On 08/25/2014 03:25 PM, Richard Weinberger wrote:
>>> Am 25.08.2014 01:02, schrieb Thorsten Knabe:
>>>> On 08/24/2014 02:11 PM, Richard Weinberger wrote:
>>>>> Am 23.08.2014 19:43, schrieb Thorsten Knabe:
>>>>>> Hi Richard.
>>>>>>
>>>>>> On 08/23/2014 05:34 PM, Richard Weinberger wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> Am 23.08.2014 15:47, schrieb Thorsten Knabe:
>>>>>>>> From: Thorsten Knabe <linux@thorsten-knabe.de>
>>>>>>>>
>>>>>>>> UML: UBD: Fix for processes stuck in D state forever in UserModeLinux.
>>>>>>>>
>>>>>>>> Starting with Linux 3.12 processes get stuck in D state forever in
>>>>>>>> UserModeLinux under sync heavy workloads. This bug was introduced by
>>>>>>>> commit 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport).
>>>>>>>> Fix bug by adding a check if FLUSH request was successfully submitted to
>>>>>>>> the I/O thread and keeping the FLUSH request on the request queue on
>>>>>>>> submission failures.
>>>>>>>>
>>>>>>>> Fixes: 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
>>>>>>>> Signed-off-by: Thorsten Knabe <linux@thorsten-knabe.de>
>>>>>>>
>>>>>>> Thanks a lot for hunting this issue down.
>>>>>>>
>>>>>>>> ---
>>>>>>>> Patch applies to 3.16.1.
>>>>>>>>
>>>>>>>> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
>>>>>>>> index 3716e69..b7d2840 100644
>>>>>>>> --- a/arch/um/drivers/ubd_kern.c
>>>>>>>> +++ b/arch/um/drivers/ubd_kern.c
>>>>>>>> @@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
>>>>>>>>
>>>>>>>>         while(1){
>>>>>>>>                 struct ubd *dev = q->queuedata;
>>>>>>>> -               if(dev->end_sg == 0){
>>>>>>>> +               if(dev->request == NULL){
>>>>>>>
>>>>>>> Why do we need this specific change?
>>>>>>
>>>>>> This change is required, because for FLUSH requests dev->end_sg is
>>>>>> initialized to 0 by blk_rq_map_sg() a few lines above, as FLUSH requests
>>>>>> have no data blocks attached to themselves.
>>>>>
>>>>> You meant "below"? Looks like I really miss something here.
>>>>> At the bottom of the while(1) loop we have
>>>>>                 dev->end_sg = 0;
>>>>>                 dev->request = NULL;
>>>>
>>>> No. The problematic line is:
>>>>                         dev->end_sg = blk_rq_map_sg(q, req, dev->sg);
>>>> and blk_rq_map_sg() returning 0 for REQ_FLUSH requests, because they
>>>> have no associated data blocks.
>>>>
>>>> Hence on the next iteration of the while(1) loop:
>>>>                 if(dev->end_sg == 0){
>>>
>>> At the bottom of the while loop dev->end_sg will be set to 0 anyway, this is what puzzles
>>> me so hard.
>>> (And dev->request too)
>>
>> Yes, but the statements:
>>                 dev->end_sg = 0;
>>                 dev->request = NULL;
>> at the end of the while loop will only be reached (for FLUSH requests
>> only with my patch applied) when submit_request() succeeds. Otherwise
>> the function do_ubd_request() returns early leaving dev->end_sg and
>> dev->request untouched.
>
> Can you please add this info into the commit message, I'm sure others get also
> confused by that.^^
> The logic in do_ubd_request() royal mess which needs fixing too. :-\

Hmm, you didn't resend the patch.
I'll amend the commit message myself and push it to Linus this week.

-- 
Thanks,
//richard

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-09-16 16:50 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-23 13:47 [PATCH] UML: UBD: Fix for processes stuck in D state forever in UserModeLinux Thorsten Knabe
2014-08-23 15:34 ` Richard Weinberger
2014-08-23 17:43   ` Thorsten Knabe
2014-08-24 12:11     ` Richard Weinberger
2014-08-24 23:02       ` Thorsten Knabe
2014-08-25 13:25         ` Richard Weinberger
2014-08-25 14:00           ` Thorsten Knabe
2014-08-25 14:04             ` Richard Weinberger
2014-09-16 16:50               ` Richard Weinberger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.