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

PATCH In review Thorsten Knabe 3 participants 9 messages Aug 23, 2014 – Sep 16, 2014 User-mode Linux (UML)

lore

Share & tools

lore

Thorsten Knabe

author

2014-08-23 14:04 UTC #0 lore raw

PATCH 1/1 UML: UBD: Fix for processes stuck in D state forever in UserModeLinux

From: Thorsten Knabe <[email protected]>

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 <[email protected]>

---
Patch applies to 3.16.1.

arch/um/drivers/ubd_kern.c +3 -2

index 3716e69..b7d2840 100644
@@ -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){

Signature · 3 lines

___
 |        | /                 E-Mail: [email protected]
 |horsten |/\nabe                WWW: http://linux.thorsten-knabe.de

Richard Weinberger

maintainer

2014-08-23 15:34 UTC #1 lore raw

Hi!

Am 23.08.2014 15:47, schrieb Thorsten Knabe:

Quoted 11 lines

From: Thorsten Knabe <[email protected]>
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.

Quoted 2 lines

Fixes 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
Signed-off-by Thorsten Knabe <[email protected]>

Thanks a lot for hunting this issue down.

arch/um/drivers/ubd_kern.c static void do_ubd_request(struct request_queue *q) · line 1280

Quoted 13 lines

---
Patch applies to 3.16.1.

arch/um/drivers/ubd_kern.c +1 -1

index 3716e69..b7d2840 100644
@@ -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

Thorsten Knabe

author

2014-08-23 17:43 UTC #2 lore raw

Hi Richard.

Quoted 28 lines · Richard Weinberger

Hi!
Am 23.08.2014 15:47, schrieb Thorsten Knabe:
Quoted 11 lines
From: Thorsten Knabe <[email protected]>
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.
Quoted 2 lines
Fixes 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
Signed-off-by Thorsten Knabe <[email protected]>
Thanks a lot for hunting this issue down.
Quoted 9 lines
---
Patch applies to 3.16.1.
arch/um/drivers/ubd_kern.c +0 -0
index 3716e69..b7d2840 100644
@@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
 

Quoted 6 lines

arch/um/drivers/ubd_kern.c static void do_ubd_request(struct request_queue *q) · line 1280
Quoted 4 lines
arch/um/drivers/ubd_kern.c +1 -1
 	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

Quoted 4 lines

Thanks,
//richard

Signature · 3 lines

___
 |        | /                 E-Mail: [email protected]
 |horsten |/\nabe                WWW: http://linux.thorsten-knabe.de

Richard Weinberger

maintainer

2014-08-24 12:11 UTC #3 lore raw

Am 23.08.2014 19:43, schrieb Thorsten Knabe:

Quoted 38 lines

Hi Richard.
Quoted 28 lines · Richard Weinberger
Hi!
Am 23.08.2014 15:47, schrieb Thorsten Knabe:
Quoted 11 lines
From: Thorsten Knabe <[email protected]>
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.
Quoted 2 lines
Fixes 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
Signed-off-by Thorsten Knabe <[email protected]>
Thanks a lot for hunting this issue down.
Quoted 9 lines
---
Patch applies to 3.16.1.
arch/um/drivers/ubd_kern.c +0 -0
index 3716e69..b7d2840 100644
@@ -1277,7 +1277,7 @@ static void do_ubd_request(struct request_queue *q)
 
Quoted 6 lines
arch/um/drivers/ubd_kern.c static void do_ubd_request(struct request_queue *q) · line 1280
Quoted 4 lines
arch/um/drivers/ubd_kern.c +1 -1
 	while(1){
 		struct ubd *dev = q->queuedata;
-		if(dev->end_sg == 0){
+		if(dev->request == NULL){
Why do we need this specific change?

Quoted 3 lines

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

Thorsten Knabe

author

2014-08-24 23:02 UTC #4 lore raw

Quoted 39 lines · Richard Weinberger

Am 23.08.2014 19:43, schrieb Thorsten Knabe:
Quoted 38 lines
Hi Richard.
Quoted 34 lines · Richard Weinberger
Hi!
Am 23.08.2014 15:47, schrieb Thorsten Knabe:
Quoted 11 lines
From: Thorsten Knabe <[email protected]>
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.
Quoted 2 lines
Fixes 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
Signed-off-by Thorsten Knabe <[email protected]>
Thanks a lot for hunting this issue down.
arch/um/drivers/ubd_kern.c static void do_ubd_request(struct request_queue *q) · line 1280
Quoted 13 lines
---
Patch applies to 3.16.1.
arch/um/drivers/ubd_kern.c +1 -1
index 3716e69..b7d2840 100644
@@ -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?

Quoted 8 lines

Quoted 3 lines
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

Quoted 4 lines

Thanks,
//richard

Signature · 3 lines

___
 |        | /                 E-Mail: [email protected]
 |horsten |/\nabe                WWW: http://linux.thorsten-knabe.de

Richard Weinberger

maintainer

2014-08-25 13:25 UTC #5 lore raw

Am 25.08.2014 01:02, schrieb Thorsten Knabe:

Quoted 49 lines · Richard Weinberger

Quoted 39 lines
Am 23.08.2014 19:43, schrieb Thorsten Knabe:
Quoted 38 lines
Hi Richard.
Quoted 34 lines · Richard Weinberger
Hi!
Am 23.08.2014 15:47, schrieb Thorsten Knabe:
Quoted 11 lines
From: Thorsten Knabe <[email protected]>
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.
Quoted 2 lines
Fixes 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
Signed-off-by Thorsten Knabe <[email protected]>
Thanks a lot for hunting this issue down.
arch/um/drivers/ubd_kern.c static void do_ubd_request(struct request_queue *q) · line 1280
Quoted 13 lines
---
Patch applies to 3.16.1.
arch/um/drivers/ubd_kern.c +1 -1
index 3716e69..b7d2840 100644
@@ -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?
Quoted 8 lines
Quoted 3 lines
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;

Quoted 7 lines

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

Thorsten Knabe

author

2014-08-25 14:00 UTC #6 lore raw

Quoted 55 lines · Richard Weinberger

Am 25.08.2014 01:02, schrieb Thorsten Knabe:
Quoted 54 lines · Richard Weinberger
Quoted 39 lines
Am 23.08.2014 19:43, schrieb Thorsten Knabe:
Quoted 38 lines
Hi Richard.
Quoted 34 lines · Richard Weinberger
Hi!
Am 23.08.2014 15:47, schrieb Thorsten Knabe:
Quoted 11 lines
From: Thorsten Knabe <[email protected]>
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.
Quoted 2 lines
Fixes 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
Signed-off-by Thorsten Knabe <[email protected]>
Thanks a lot for hunting this issue down.
arch/um/drivers/ubd_kern.c static void do_ubd_request(struct request_queue *q) · line 1280
Quoted 13 lines
---
Patch applies to 3.16.1.
arch/um/drivers/ubd_kern.c +1 -1
index 3716e69..b7d2840 100644
@@ -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?
Quoted 8 lines
Quoted 3 lines
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 4 lines

Thanks,
//richard

Signature · 3 lines

___
 |        | /                 E-Mail: [email protected]
 |horsten |/\nabe                WWW: http://linux.thorsten-knabe.de

Richard Weinberger

maintainer

2014-08-25 14:04 UTC #7 lore raw

Am 25.08.2014 16:00, schrieb Thorsten Knabe:

Quoted 63 lines · Richard Weinberger

Quoted 55 lines
Am 25.08.2014 01:02, schrieb Thorsten Knabe:
Quoted 54 lines · Richard Weinberger
Quoted 39 lines
Am 23.08.2014 19:43, schrieb Thorsten Knabe:
Quoted 38 lines
Hi Richard.
Quoted 34 lines · Richard Weinberger
Hi!
Am 23.08.2014 15:47, schrieb Thorsten Knabe:
Quoted 11 lines
From: Thorsten Knabe <[email protected]>
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.
Quoted 2 lines
Fixes 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
Signed-off-by Thorsten Knabe <[email protected]>
Thanks a lot for hunting this issue down.
arch/um/drivers/ubd_kern.c static void do_ubd_request(struct request_queue *q) · line 1280
Quoted 13 lines
---
Patch applies to 3.16.1.
arch/um/drivers/ubd_kern.c +1 -1
index 3716e69..b7d2840 100644
@@ -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?
Quoted 8 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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)

Quoted 7 lines

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

Richard Weinberger

2014-09-16 16:50 UTC #8 lore raw

Quoted 72 lines · Richard Weinberger

Am 25.08.2014 16:00, schrieb Thorsten Knabe:
Quoted 63 lines · Richard Weinberger
Quoted 55 lines
Am 25.08.2014 01:02, schrieb Thorsten Knabe:
Quoted 54 lines · Richard Weinberger
Quoted 39 lines
Am 23.08.2014 19:43, schrieb Thorsten Knabe:
Quoted 38 lines
Hi Richard.
Quoted 34 lines · Richard Weinberger
Hi!
Am 23.08.2014 15:47, schrieb Thorsten Knabe:
Quoted 11 lines
From: Thorsten Knabe <[email protected]>
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.
Quoted 2 lines
Fixes 805f11a0d5 (um: ubd: Add REQ_FLUSH suppport)
Signed-off-by Thorsten Knabe <[email protected]>
Thanks a lot for hunting this issue down.
arch/um/drivers/ubd_kern.c static void do_ubd_request(struct request_queue *q) · line 1280
Quoted 13 lines
---
Patch applies to 3.16.1.
arch/um/drivers/ubd_kern.c +1 -1
index 3716e69..b7d2840 100644
@@ -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?
Quoted 8 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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)
Quoted 7 lines
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.

Quoted 3 lines

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.

Signature · 2 lines

Thanks,
//richard