fixes for linked list bugs in block I/O code
PATCH 1/1 fixes for linked list bugs in block I/O code
I found a couple of small linked list bugs in __make_request() in drivers/block/ll_rw_blk.c. The bugs exist in both kernels 2.4.20 and 2.5.69. Therefore I have made patches for both kernels. The problem is that when inserting a new buffer_head into the linked list of buffer_head structures maintained by "struct request", the b_reqnext field is not initialized.
-Dave Peterson
[email protected]
========== START OF 2.4.20 PATCH FOR drivers/block/ll_rw_blk.c ===========
ll_rw_blk.c.new Wed May 7 15:58:07 2003 renamed from ll_rw_blk.c.old Wed May 7 15:54:58 2003 +2 -0
@@ -973,6 +973,7 @@ insert_here = &req->queue; break; } + bh->b_reqnext = req->bhtail->b_reqnext; req->bhtail->b_reqnext = bh; req->bhtail = bh; req->nr_sectors = req->hard_nr_sectors += count;
@@ -1061,6 +1062,7 @@ req->waiting = NULL; req->bh = bh; req->bhtail = bh; + bh->b_reqnext = NULL; req->rq_dev = bh->b_rdev; req->start_time = jiffies; req_new_io(req, 0, count);
========== END OF 2.4.20 PATCH FOR drivers/block/ll_rw_blk.c =============
========== START OF 2.5.69 PATCH FOR drivers/block/ll_rw_blk.c ===========
ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +2 -0
@@ -1721,6 +1721,7 @@ break; } + bio->bi_next = req->biotail->bi_next; req->biotail->bi_next = bio; req->biotail = bio; req->nr_sectors = req->hard_nr_sectors += nr_sectors;
@@ -1811,6 +1812,7 @@ req->buffer = bio_data(bio); /* see ->buffer comment above */ req->waiting = NULL; req->bio = req->biotail = bio; + bio->bi_next = NULL; req->rq_disk = bio->bi_bdev->bd_disk; req->start_time = jiffies; add_request(q, req, insert_here);
========== END OF 2.5.69 PATCH FOR drivers/block/ll_rw_blk.c =============
Quoted 37 lines · Dave Peterson
I found a couple of small linked list bugs in
__make_request()indrivers/block/ll_rw_blk.c. The bugs exist in both kernels 2.4.20 and 2.5.69. Therefore I have made patches for both kernels. The problem is that when inserting a newbuffer_headinto the linked list ofbuffer_headstructures maintained by "struct request", theb_reqnextfield is not initialized.-Dave Peterson
[email protected]========== START OF 2.4.20 PATCH FOR
drivers/block/ll_rw_blk.c===========ll_rw_blk.c.new Wed May 7 15:58:07 2003 renamed from ll_rw_blk.c.old Wed May 7 15:54:58 2003 +2
-0@@ -973,6 +973,7 @@ insert_here = &req->queue; break; } + bh->b_reqnext = req->bhtail->b_reqnext; req->bhtail->b_reqnext = bh; req->bhtail = bh; req->nr_sectors = req->hard_nr_sectors += count;@@ -1061,6 +1062,7 @@ req->waiting = NULL; req->bh = bh; req->bhtail = bh; + bh->b_reqnext = NULL; req->rq_dev = bh->b_rdev; req->start_time = jiffies; req_new_io(req, 0, count);========== END OF 2.4.20 PATCH FOR
drivers/block/ll_rw_blk.c======================= START OF 2.5.69 PATCH FOR
drivers/block/ll_rw_blk.c===========
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1724
Quoted 4 lines
ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +1
-0@@ -1721,6 +1721,7 @@ break; } + bio->bi_next = req->biotail->bi_next;
This is simply wrong, look at the line below.
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1725
Quoted 1 line
ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +0
-0req->biotail->bi_next = bio;
req->bio - first bioreq->bio->bi_next - next bio
...req->biotail - last bio
so req->biotail->bi_next should be NULL
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1815
Quoted 7 lines
ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +1
-0req->biotail = bio; req->nr_sectors = req->hard_nr_sectors += nr_sectors;@@ -1811,6 +1812,7 @@ req->buffer = bio_data(bio); /* see ->buffer comment above */ req->waiting = NULL; req->bio = req->biotail = bio; + bio->bi_next = NULL;
No need for that, look at bio_init() in fs/bio.c.
Quoted 4 lines
ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +0
-0req->rq_disk = bio->bi_bdev->bd_disk; req->start_time = jiffies; add_request(q, req, insert_here);========== END OF 2.5.69 PATCH FOR
drivers/block/ll_rw_blk.c=============
--
Bartlomiej
Quoted 11 lines · Bartlomiej Zolnierkiewicz
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1724
Quoted 8 lines
========== START OF 2.5.69 PATCH FOR
drivers/block/ll_rw_blk.c
=========== --- ll_rw_blk.c.old Wed May 7 15:55:18 2003
+++ ll_rw_blk.c.new Wed May 7 16:01:56 2003ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +1
-0@@ -1721,6 +1721,7 @@ break; } + bio->bi_next = req->biotail->bi_next;This is simply wrong, look at the line below.
Quoted 8 lines
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1725
Quoted 1 line
ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +0
-0req->biotail->bi_next = bio;
req->bio- first bioreq->bio->bi_next- next bio
...req->biotail- last bioso
req->biotail->bi_nextshould be NULL
I believe it is correct. Assuming that the list is initially in a sane state, req->biotail->bi_next will be NULL immediately before executing the statement that I added. Therefore, my fix will set bio->bi_next to NULL, which is what we want because bio becomes the new end of the list.
Quoted 9 lines
Quoted 7 lines
(quoted diff) +0
-0req->biotail = bio; req->nr_sectors = req->hard_nr_sectors +=
nr_sectors; @@ -1811,6 +1812,7 @@req->buffer = bio_data(bio); /* see ->buffer comment above */ req->waiting = NULL; req->bio = req->biotail = bio;+
bio->bi_next= NULL;No need for that, look at
bio_init()infs/bio.c.
Yes, it looks like bio_init has been added in the 2.5 kernels, solving the problem. However, this is still a bug in 2.4.20.
-Dave
Quoted 19 lines · Dave Peterson
Quoted 18 lines · Bartlomiej Zolnierkiewicz
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1724
Quoted 8 lines
========== START OF 2.5.69 PATCH FOR
drivers/block/ll_rw_blk.c
=========== --- ll_rw_blk.c.old Wed May 7 15:55:18 2003
+++ ll_rw_blk.c.new Wed May 7 16:01:56 2003ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +1
-0@@ -1721,6 +1721,7 @@ break; } + bio->bi_next = req->biotail->bi_next;This is simply wrong, look at the line below.
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1725
Quoted 1 line
ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +0
-0req->biotail->bi_next = bio;
req->bio- first bioreq->bio->bi_next- next bio
...req->biotail- last bio
Quoted 7 lines
Quoted 1 line
so
req->biotail->bi_nextshould be NULLI believe it is correct. Assuming that the list is initially in a sane state,
req->biotail->bi_nextwill be NULL immediately before executing the statement that I added. Therefore, my fix will setbio->bi_nextto NULL, which is what we want because bio becomes the new end of the list.
Yes, but bio->bi_next is a NULL already.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
(quoted diff) +0
-0req->biotail = bio; req->nr_sectors = req->hard_nr_sectors +=
nr_sectors; @@ -1811,6 +1812,7 @@req->buffer = bio_data(bio); /* see ->buffer comment above */ req->waiting = NULL; req->bio = req->biotail = bio;+
bio->bi_next= NULL;
Quoted 4 lines
Quoted 1 line
No need for that, look at
bio_init()infs/bio.c.Yes, it looks like
bio_inithas been added in the 2.5 kernels, solving the problem. However, this is still a bug in 2.4.20.
Maybe. If so only second part of the patch is important.
Regards,
--
Bartlomiej
Quoted 22 lines · Bartlomiej Zolnierkiewicz
Quoted 21 lines · Dave Peterson
Quoted 19 lines · Bartlomiej Zolnierkiewicz
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1724
Quoted 8 lines
========== START OF 2.5.69 PATCH FOR
drivers/block/ll_rw_blk.c
=========== --- ll_rw_blk.c.old Wed May 7 15:55:18 2003
+++ ll_rw_blk.c.new Wed May 7 16:01:56 2003ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +1
-0@@ -1721,6 +1721,7 @@ break; } + bio->bi_next = req->biotail->bi_next;This is simply wrong, look at the line below.
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1725
Quoted 1 line
ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +0
-0req->biotail->bi_next = bio;
req->bio- first bioreq->bio->bi_next- next bio
...req->biotail- last bioso
req->biotail->bi_nextshould be NULL
Quoted 7 lines
Quoted 5 lines
I believe it is correct. Assuming that the list is initially in a sane state,
req->biotail->bi_nextwill be NULL immediately before executing the statement that I added. Therefore, my fix will setbio->bi_nextto NULL, which is what we want because bio becomes the new end of the list.Yes, but
bio->bi_nextis a NULL already.
I think assuming this is bad programming form. You are assuming that the memory allocator zeros out newly allocated memory. Though your assumption may be correct, it's always possible that this behavior will change some day (perhaps for efficiency reasons), causing your code to break. In my opinion, the savings of a few cpu clock cycles that you gain by omitting the initialization isn't worth compromising the robustness of your code.
-Dave
Quoted 29 lines · Dave Peterson
Quoted 28 lines · Bartlomiej Zolnierkiewicz
Quoted 26 lines · Dave Peterson
Quoted 11 lines · Bartlomiej Zolnierkiewicz
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1724
Quoted 8 lines
========== START OF 2.5.69 PATCH FOR
drivers/block/ll_rw_blk.c
=========== --- ll_rw_blk.c.old Wed May 7 15:55:18 2003
+++ ll_rw_blk.c.new Wed May 7 16:01:56 2003ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +1
-0@@ -1721,6 +1721,7 @@ break; } + bio->bi_next = req->biotail->bi_next;This is simply wrong, look at the line below.
Quoted 8 lines
ll_rw_blk.c.new Wed May 7 16:01:56 2003 line 1725
Quoted 1 line
ll_rw_blk.c.new Wed May 7 16:01:56 2003 renamed from ll_rw_blk.c.old Wed May 7 15:55:18 2003 +0
-0req->biotail->bi_next = bio;
req->bio- first bioreq->bio->bi_next- next bio
...req->biotail- last bioso
req->biotail->bi_nextshould be NULLI believe it is correct. Assuming that the list is initially in a sane state,
req->biotail->bi_nextwill be NULL immediately before executing the statement that I added. Therefore, my fix will setbio->bi_nextto NULL, which is what we want because bio becomes the new end of the list.
Quoted 4 lines
Quoted 1 line
Yes, but
bio->bi_nextis a NULL already.I think assuming this is bad programming form. You are assuming that the memory allocator zeros out newly allocated memory. Though your
No, it is not memory allocator but block layer. Look at bio_init(), there is bio->bi_next = NULL explicitly.
Quoted 5 lines
assumption may be correct, it's always possible that this behavior will change some day (perhaps for efficiency reasons), causing your code to break. In my opinion, the savings of a few cpu clock cycles that you gain by omitting the initialization isn't worth compromising the robustness of your code.
Agreed, but not the case here (speaking 2.5).
Better add check for bio->bi_next != NULL to catch improper usage.
--
Bartlomiej
Quoted 1 line
-Dave
Quoted 7 lines · Bartlomiej Zolnierkiewicz
Quoted 4 lines
Quoted 1 line
Yes, but
bio->bi_nextis a NULL already.I think assuming this is bad programming form. You are assuming that the memory allocator zeros out newly allocated memory. Though your
No, it is not memory allocator but block layer. Look at
bio_init(), there isbio->bi_next= NULL explicitly.
Ok, I agree: the patch is not needed for 2.5 (although I think it still makes sense for 2.4.20). Thanks for the correction.
-Dave
Quoted 15 lines · Dave Peterson
I found a couple of small linked list bugs in
__make_request()indrivers/block/ll_rw_blk.c. The bugs exist in both kernels 2.4.20 and 2.5.69. Therefore I have made patches for both kernels. The problem is that when inserting a newbuffer_headinto the linked list ofbuffer_headstructures maintained by "struct request", theb_reqnextfield is not initialized.-Dave Peterson
[email protected]========== START OF 2.4.20 PATCH FOR
drivers/block/ll_rw_blk.c===========
ll_rw_blk.c.new Wed May 7 15:58:07 2003 line 976
Quoted 4 lines
ll_rw_blk.c.new Wed May 7 15:58:07 2003 renamed from ll_rw_blk.c.old Wed May 7 15:54:58 2003 +1
-0@@ -973,6 +973,7 @@ insert_here = &req->queue; break; } + bh->b_reqnext = req->bhtail->b_reqnext;
This is convoluted nonsense, bhtail->b_reqnext is NULL by definition. So a simple
bh->b_reqnext = NULL;
is much clearer.
ll_rw_blk.c.new Wed May 7 15:58:07 2003 line 1068
Quoted 11 lines
ll_rw_blk.c.new Wed May 7 15:58:07 2003 renamed from ll_rw_blk.c.old Wed May 7 15:54:58 2003 +1
-0req->bhtail->b_reqnext = bh; req->bhtail = bh; req->nr_sectors = req->hard_nr_sectors += count;@@ -1061,6 +1062,7 @@ req->waiting = NULL; req->bh = bh; req->bhtail = bh; + bh->b_reqnext = NULL; req->rq_dev = bh->b_rdev; req->start_time = jiffies; req_new_io(req, 0, count);
Bart already covered why 2.5 definitely does not need it. I dunno what to say for 2.4, to me it looks like a BUG if you pass in a buffer_head with uninitialized b_reqnext. Why should that member be any different?
In fact, from where did you see this buffer_head coming from? Who is submitting IO on a not properly inited bh? To me, that sounds like not a block layer bug but an fs bug.
Signature · 1 line
Jens Axboe
Quoted 6 lines · Jens Axboe
This is convoluted nonsense,
bhtail->b_reqnextis NULL by definition. So a simplebh->b_reqnext = NULL;is much clearer.
Ok, that's fine with me. Setting it explicitly to NULL is also correct and perhaps a bit more clear.
Quoted 7 lines
Quoted 7 lines
req->bhtail->b_reqnext = bh; req->bhtail = bh; req->nr_sectors = req->hard_nr_sectors += count;ll_rw_blk.c.new Wed May 7 15:58:07 2003 renamed from ll_rw_blk.c.old Wed May 7 15:54:58 2003 +0
-0@@ -1061,6 +1062,7 @@ req->waiting = NULL; req->bh = bh; req->bhtail = bh;
Quoted 8 lines
ll_rw_blk.c.new Wed May 7 15:58:07 2003 line 1068
Quoted 4 lines
ll_rw_blk.c.new Wed May 7 15:58:07 2003 renamed from ll_rw_blk.c.old Wed May 7 15:54:58 2003 +1
-0+ bh->b_reqnext = NULL; req->rq_dev = bh->b_rdev; req->start_time = jiffies; req_new_io(req, 0, count);Bart already covered why 2.5 definitely does not need it. I dunno what to say for 2.4, to me it looks like a BUG if you pass in a
buffer_headwith uninitializedb_reqnext. Why should that member be any different?
Ok, agreed that 2.5 does not need the patch.
Quoted 3 lines
In fact, from where did you see this
buffer_headcoming from? Who is submitting IO on a not properly inited bh? To me, that sounds like not a block layer bug but an fs bug.
I would argue that __make_request() is where the insertion of the buffer_head into the request structure is performed, and setting the b_reqnext field of the buffer_head to its proper value is an integral part of performing the insertion. Why not keep all of the insertion logic in one place rather than distribuing it throughout the code and requiring all callers of __make_request() to remember to initialize b_reqnext? Localizing the insertion logic makes the code clearer, more compact, and easier to maintain.
-Dave