Skip to contentSkip to conversation

don't reuse a freed request

PATCH In review Arnaldo Carvalho de Melo 4 participants 10 messages Oct 31, 2005 Block layer

lore
Share & tools
lore

Arnaldo Carvalho de Melo

author
#0 lore raw

PATCH 1/1 don't reuse a freed request

Hi,

I'm getting the oops below when trying to use qemu with a kernel
built with just the noop iosched, I'm never had looked at this code before, so I did a quick hack that seems enough for my case.

Ah, this is with a fairly recent git tree (today), haven't checked
if it is present in 2.6.14.

Best Regards,

- Arnaldo

Unable to handle kernel paging request at virtual address c5f20f60
 printing eip:
c01b0ecd
*pde = 00017067
*pte = 05f20000
Oops: 0000 [#1]
DEBUG_PAGEALLOC
Modules linked in:
CPU:    0
EIP:    0060:[<c01b0ecd>]    Not tainted VLI
EFLAGS: 00000046   (2.6.14acme)

EIP is at elv_rq_merge_ok+0x15/0x7b
eax: 00000014 ebx: c5f20f58 ecx: 000003f8 edx: 00000046
esi: c12a5a90 edi: c5f20f58 ebp: c11658d0 esp: c11658c4
ds: 007b es: 007b ss: 0068
Process swapper (pid: 1, threadinfo=c1165000 task=c1164af0)

Stack: c0251883 c5ecfe4c c5d688c0 c1165904 c01b0f48 c5f20f58 c12a5a90 00000000
       c5874000 c018c5e1 c5f15f24 0000002b 00000000 c5ecfe4c c5d688c0 c12a5a90
       c1165920 c01b128d c5f20f58 c12a5a90 000a568a 00000000 00000002 c1165960
Call Trace:
 [<c0102a63>] show_stack+0x78/0x83
 [<c0102b88>] show_registers+0x100/0x167
 [<c0102d35>] die+0xcb/0x140
 [<c0234308>] do_page_fault+0x393/0x53a
 [<c0102777>] error_code+0x4f/0x54
 [<c01b0f48>] elv_try_merge+0x15/0x84
 [<c01b128d>] elv_merge+0x1d/0x4f
 [<c01b41d9>] __make_request+0xb2/0x425
 [<c01b46f9>] generic_make_request+0x125/0x137
 [<c01b47ae>] submit_bio+0xa3/0xa9
 [<c013d48a>] submit_bh+0xeb/0x10c
 [<c013b953>] __bread_slow+0x4a/0x86
 [<c013bba4>] __bread+0x25/0x2c
 [<c016b736>] ext3_get_branch+0x6c/0xe9
 [<c016bc20>] ext3_get_block_handle+0x99/0x28e
 [<c016be74>] ext3_get_block+0x5f/0x66
 [<c0157942>] do_mpage_readpage+0x110/0x3ab
 [<c0157c62>] mpage_readpages+0x85/0x114
 [<c016ca53>] ext3_readpages+0x16/0x18
 [<c012b27a>] read_pages+0x22/0xf5
 [<c012b442>] __do_page_cache_readahead+0xf5/0x113
 [<c012b553>] blockable_page_cache_readahead+0x43/0x9a
 [<c012b6ab>] page_cache_readahead+0x72/0x101
 [<c0126204>] do_generic_mapping_read+0xfc/0x451
 [<c012679b>] __generic_file_aio_read+0x15a/0x1a8
 [<c012682b>] generic_file_aio_read+0x42/0x49
 [<c0139712>] do_sync_read+0x9c/0xcd
 [<c01397cd>] vfs_read+0x8a/0x12a
 [<c014263e>] kernel_read+0x37/0x41
 [<c0142fa4>] prepare_binprm+0xbc/0xce
 [<c0143348>] do_execve+0xe9/0x1d9
 [<c01013e9>] sys_execve+0x2a/0x75
 [<c010254d>] syscall_call+0x7/0xb
Code: b9 21 08 00 59 eb 98 66 4a 75 e9 51 e8 55 c3 fd ff 59 eb e0 90 90 55 89 e5 56 8b 75 0c 53 8b 5d 08 68 83 18 25 c0 e8 bf db f5 ff <8b> 43 08 5a a8 d8 75 55 a8 20 74 51 68 b1 18 25 c0 e8 a9 db f5
 <0>Kernel panic - not syncing: Attempted to kill init!
drivers/block/ll_rw_blk.c +3 -0
@@ -1787,6 +1787,9 @@ static inline void blk_free_request(requ
 	if (rq->flags & REQ_ELVPRIV)
 		elv_put_request(q, rq);
 	mempool_free(rq, q->rq.rq_pool);
+
+	if (rq == q->last_merge)
+		q->last_merge = NULL;
 }
 
 static inline struct request *
author
#1 lore raw

Em Mon, Oct 31, 2005 at 12:30:24AM -0200, Arnaldo Carvalho de Melo escreveu:

Quoted 8 lines

Hi,

I'm getting the oops below when trying to use qemu with a kernel
built with just the noop iosched, I'm never had looked at this code before, so I did a quick hack that seems enough for my case.

Ah, this is with a fairly recent git tree (today), haven't checked
if it is present in 2.6.14.

Further info: building with all the io schedulers and using 'elevator=cfq' in the kernel cmd line produces another oops, with or without my patch:

hda:<1>Unable to handle kernel paging request at virtual address c554ef60 printing eip:
01b14f7
pde = 00015067
pte = 0554e000
ops: 0000 [#1]
EBUG_PAGEALLOC
odules linked in:

PU:    0
IP:    0060:[<c01b14f7>]    Not tainted VLI
FLAGS: 00000046   (2.6.14acme)

IP is at __elv_add_request+0xe7/0x13f
ax: c54f8e4c ebx: 00000003 ecx: c54f6ef8 edx: c554ef58
si: c554ef58 edi: c54f8e4c ebp: c1165ae0 esp: c1165ad4
s: 007b es: 007b ss: 0068
rocess swapper (pid: 1, threadinfo=c1165000 task=c1164af0)
tack: c554ef58 00000000 00000008 c1165b24 c01b45a5 c54f8e4c c554ef58 00000003

      00000000 00000000 00000000 00000008 00000000 00000000 c12a8710 c0128837
      c554ef58 00000008 c54f8e4c 00000100 c1165b60 c01b478d c54f8e4c c12a5a90
all Trace:
[<c0102a63>] show_stack+0x78/0x83
[<c0102b88>] show_registers+0x100/0x167
[<c0102d35>] die+0xcb/0x140
[<c0239780>] do_page_fault+0x393/0x53a
[<c0102777>] error_code+0x4f/0x54
[<c01b45a5>] __make_request+0x3ea/0x425
[<c01b478d>] generic_make_request+0x125/0x137
[<c01b4842>] submit_bio+0xa3/0xa9
[<c013d48a>] submit_bh+0xeb/0x10c
[<c013c702>] block_read_full_page+0x23e/0x253
[<c0140039>] blkdev_readpage+0x10/0x12
[<c012711b>] read_cache_page+0x74/0x1af
[<c0165822>] read_dev_sector+0x2d/0xb0
[<c0165c16>] msdos_partition+0x47/0x2cd
[<c01653a4>] check_partition+0x87/0xce
[<c016578e>] rescan_partitions+0x77/0xde
[<c014094b>] do_open+0x22c/0x2df
[<c0140a5d>] blkdev_get+0x5f/0x67
[<c0165702>] register_disk+0x96/0xab
[<c01b5d48>] add_disk+0x2f/0x3d
[<c01cb3f6>] ide_disk_probe+0x17a/0x199
[<c01ae283>] driver_probe_device+0x36/0x8a
[<c01ae382>] __driver_attach+0x33/0x48
[<c01adadd>] bus_for_each_dev+0x42/0x69
[<c01ae3ad>] driver_attach+0x16/0x1b
[<c01ade91>] bus_add_driver+0x5d/0xa6
[<c01ae752>] driver_register+0x54/0x5c
[<c01cb455>] idedisk_init+0xd/0xf
[<c02ac6b7>] do_initcalls+0x46/0x96
[<c02ac73c>] do_basic_setup+0x21/0x23
[<c01002a1>] init+0x25/0x10c
[<c0100c8d>] kernel_thread_helper+0x5/0xb

ode: 59 5b eb 52 8b 46 08 a8 20 75 08 0f 0b 79 01 6f ed 24 c0 83 c8 04 89 46 08 8b 47 0c 8b 00 56 57 ff 50 10 83 7f 08 00 58 5a 75 2b <8b> 46 08 a8 d8 75 24 a8 20 74 20 89 77 08 eb 1b 53 68 c0 e6 23 <0>Kernel panic - not syncing: Attempted to kill init!

author
#2 lore raw

Em Mon, Oct 31, 2005 at 12:55:26AM -0200, Arnaldo Carvalho de Melo escreveu:

Quoted 21 lines

Em Mon, Oct 31, 2005 at 12:30:24AM -0200, Arnaldo Carvalho de Melo escreveu:

Quoted 8 lines

Hi,

I'm getting the oops below when trying to use qemu with a kernel
built with just the noop iosched, I'm never had looked at this code before, so I did a quick hack that seems enough for my case.

Ah, this is with a fairly recent git tree (today), haven't checked
if it is present in 2.6.14.

Further info: building with all the io schedulers and using 'elevator=cfq' in the kernel cmd line produces another oops, with or without my patch:

hda:<1>Unable to handle kernel paging request at virtual address c554ef60 printing eip:
01b14f7
pde = 00015067
pte = 0554e000
ops: 0000 [#1]
EBUG_PAGEALLOC
odules linked in:

Quoted 4 lines
PU:    0
IP:    0060:[<c01b14f7>]    Not tainted VLI
FLAGS: 00000046   (2.6.14acme)

IP is at __elv_add_request+0xe7/0x13f

Ok, some more info, hope it'll be useful: I narrowed it down to this part of __elv_add_request:

	case ELEVATOR_INSERT_SORT:
                BUG_ON(!blk_fs_request(rq));
                rq->flags |= REQ_SORTED;
                q->elevator->ops->elevator_add_req_fn(q, rq);
                if (q->last_merge == NULL && rq_mergeable(rq))
                        q->last_merge = rq;
                break;

It seems it is not safe to touch the request (rq) after calling elevator_add_req_fn, as the panic happens when rq_mergeable tries to read rq->flags, or cfq_insert_request is doing something bad, well, enough for my block layer wanderings :-)

Best Regards,

- Arnaldo

P.S. the last patches to touch this code are post 2.6.14

#3 lore raw
In reply to Arnaldo Carvalho de Melo
Quoted 29 lines · Arnaldo Carvalho de Melo

Hi,

I'm getting the oops below when trying to use qemu with a kernel
built with just the noop iosched, I'm never had looked at this code before, so I did a quick hack that seems enough for my case.

Ah, this is with a fairly recent git tree (today), haven't checked
if it is present in 2.6.14.

Best Regards,

- Arnaldo

Unable to handle kernel paging request at virtual address c5f20f60
 printing eip:
c01b0ecd
*pde = 00017067
*pte = 05f20000
Oops: 0000 [#1]
DEBUG_PAGEALLOC
Modules linked in:
CPU:    0
EIP:    0060:[<c01b0ecd>]    Not tainted VLI
EFLAGS: 00000046   (2.6.14acme)

EIP is at elv_rq_merge_ok+0x15/0x7b
eax: 00000014 ebx: c5f20f58 ecx: 000003f8 edx: 00000046
esi: c12a5a90 edi: c5f20f58 ebp: c11658d0 esp: c11658c4
ds: 007b es: 007b ss: 0068
Process swapper (pid: 1, threadinfo=c1165000 task=c1164af0)

Quoted 13 lines
Stack: c0251883 c5ecfe4c c5d688c0 c1165904 c01b0f48 c5f20f58 c12a5a90 00000000
       c5874000 c018c5e1 c5f15f24 0000002b 00000000 c5ecfe4c c5d688c0 c12a5a90
       c1165920 c01b128d c5f20f58 c12a5a90 000a568a 00000000 00000002 c1165960
Call Trace:
 [<c0102a63>] show_stack+0x78/0x83
 [<c0102b88>] show_registers+0x100/0x167
 [<c0102d35>] die+0xcb/0x140
 [<c0234308>] do_page_fault+0x393/0x53a
 [<c0102777>] error_code+0x4f/0x54
 [<c01b0f48>] elv_try_merge+0x15/0x84
 [<c01b128d>] elv_merge+0x1d/0x4f
 [<c01b41d9>] __make_request+0xb2/0x425
 [<c01b46f9>] generic_make_request+0x125/0x137

Hrmpf, this looks really bad. Tejun, clearly there are still paths where ->last_rq isn't being cleared.

drivers/block/ll_rw_blk.c static inline void blk_free_request(requ · line 1795

Quoted 12 lines
drivers/block/ll_rw_blk.c +3 -0
@@ -1787,6 +1787,9 @@ static inline void blk_free_request(requ
 	if (rq->flags & REQ_ELVPRIV)
 		elv_put_request(q, rq);
 	mempool_free(rq, q->rq.rq_pool);
+
+	if (rq == q->last_merge)
+		q->last_merge = NULL;
 }
 
 static inline struct request *

It's most likely a bug getting this far in the first place, but does it fix things for you? I'll get on this asap.

Signature · 1 line
Jens Axboe
#4 lore raw

Hi, guys.

Quoted 46 lines · Jens Axboe
Quoted 29 lines · Arnaldo Carvalho de Melo

Hi,

I'm getting the oops below when trying to use qemu with a kernel
built with just the noop iosched, I'm never had looked at this code before, so I did a quick hack that seems enough for my case.

Ah, this is with a fairly recent git tree (today), haven't checked
if it is present in 2.6.14.

Best Regards,

- Arnaldo

Unable to handle kernel paging request at virtual address c5f20f60

printing eip:
c01b0ecd
*pde = 00017067
*pte = 05f20000

Oops: 0000 [#1]
DEBUG_PAGEALLOC
Modules linked in:
CPU:    0
EIP:    0060:[<c01b0ecd>]    Not tainted VLI
EFLAGS: 00000046   (2.6.14acme)

EIP is at elv_rq_merge_ok+0x15/0x7b
eax: 00000014 ebx: c5f20f58 ecx: 000003f8 edx: 00000046
esi: c12a5a90 edi: c5f20f58 ebp: c11658d0 esp: c11658c4
ds: 007b es: 007b ss: 0068
Process swapper (pid: 1, threadinfo=c1165000 task=c1164af0)

Quoted 13 lines
Stack: c0251883 c5ecfe4c c5d688c0 c1165904 c01b0f48 c5f20f58 c12a5a90 00000000
      c5874000 c018c5e1 c5f15f24 0000002b 00000000 c5ecfe4c c5d688c0 c12a5a90
      c1165920 c01b128d c5f20f58 c12a5a90 000a568a 00000000 00000002 c1165960
Call Trace:
[<c0102a63>] show_stack+0x78/0x83
[<c0102b88>] show_registers+0x100/0x167
[<c0102d35>] die+0xcb/0x140
[<c0234308>] do_page_fault+0x393/0x53a
[<c0102777>] error_code+0x4f/0x54
[<c01b0f48>] elv_try_merge+0x15/0x84
[<c01b128d>] elv_merge+0x1d/0x4f
[<c01b41d9>] __make_request+0xb2/0x425
[<c01b46f9>] generic_make_request+0x125/0x137
Quoted 3 lines

Hrmpf, this looks really bad. Tejun, clearly there are still paths where ->last_rq isn't being cleared.

I'm currently debugging this. The problem is that we are using generic dispatch queue directly in the noop and merging is NOT allowed on dispatch queues but generic handling of last_merge tries to merge requests. I'm still trying to verify this, so I'll be back with results soon.

Quoted 12 lines
Quoted 12 lines
drivers/block/ll_rw_blk.c +3 -0
@@ -1787,6 +1787,9 @@ static inline void blk_free_request(requ
 	if (rq->flags & REQ_ELVPRIV)
 		elv_put_request(q, rq);
 	mempool_free(rq, q->rq.rq_pool);
+
+	if (rq == q->last_merge)
+		q->last_merge = NULL;
 }
Quoted 6 lines
Quoted 1 line

static inline struct request *

It's most likely a bug getting this far in the first place, but does it fix things for you? I'll get on this asap.

If the bug is where I think it is, I think the proper thing to do is to use separate list_head in noop instead of using generic dispatch queue directly thus making noop consistent with other ioscheds.

I'm more worried about oops w/ cfq Arnaldo reported in this thread. I'll track that down as soon as I'm done with this one.

Many bugs. Sorry. :-)

Signature · 1 line
tejun
#5 lore raw
Quoted 82 lines · Tejun Heo

Hi, guys.

Quoted 50 lines · Jens Axboe
Quoted 30 lines · Arnaldo Carvalho de Melo

Hi,

I'm getting the oops below when trying to use qemu with a kernel
built with just the noop iosched, I'm never had looked at this code before,
so I did a quick hack that seems enough for my case.

Ah, this is with a fairly recent git tree (today), haven't checked
if it is present in 2.6.14.

Best Regards,

- Arnaldo

Unable to handle kernel paging request at virtual address c5f20f60

printing eip:
c01b0ecd
*pde = 00017067
*pte = 05f20000

Oops: 0000 [#1]
DEBUG_PAGEALLOC
Modules linked in:
CPU:    0
EIP:    0060:[<c01b0ecd>]    Not tainted VLI
EFLAGS: 00000046   (2.6.14acme)

EIP is at elv_rq_merge_ok+0x15/0x7b
eax: 00000014 ebx: c5f20f58 ecx: 000003f8 edx: 00000046
esi: c12a5a90 edi: c5f20f58 ebp: c11658d0 esp: c11658c4
ds: 007b es: 007b ss: 0068
Process swapper (pid: 1, threadinfo=c1165000 task=c1164af0)

Quoted 16 lines
Stack: c0251883 c5ecfe4c c5d688c0 c1165904 c01b0f48 c5f20f58 c12a5a90 
00000000
     c5874000 c018c5e1 c5f15f24 0000002b 00000000 c5ecfe4c c5d688c0 
     c12a5a90
     c1165920 c01b128d c5f20f58 c12a5a90 000a568a 00000000 00000002 
     c1165960
Call Trace:
[<c0102a63>] show_stack+0x78/0x83
[<c0102b88>] show_registers+0x100/0x167
[<c0102d35>] die+0xcb/0x140
[<c0234308>] do_page_fault+0x393/0x53a
[<c0102777>] error_code+0x4f/0x54
[<c01b0f48>] elv_try_merge+0x15/0x84
[<c01b128d>] elv_merge+0x1d/0x4f
[<c01b41d9>] __make_request+0xb2/0x425
[<c01b46f9>] generic_make_request+0x125/0x137
Quoted 3 lines

Hrmpf, this looks really bad. Tejun, clearly there are still paths where ->last_rq isn't being cleared.

I'm currently debugging this. The problem is that we are using generic dispatch queue directly in the noop and merging is NOT allowed on dispatch queues but generic handling of last_merge tries to merge requests. I'm still trying to verify this, so I'll be back with results soon.

Quoted 12 lines
Quoted 12 lines
drivers/block/ll_rw_blk.c +3 -0
@@ -1787,6 +1787,9 @@ static inline void blk_free_request(requ
 	if (rq->flags & REQ_ELVPRIV)
 		elv_put_request(q, rq);
 	mempool_free(rq, q->rq.rq_pool);
+
+	if (rq == q->last_merge)
+		q->last_merge = NULL;
 }
Quoted 6 lines
Quoted 1 line

static inline struct request *

It's most likely a bug getting this far in the first place, but does it fix things for you? I'll get on this asap.

Quoted 6 lines

If the bug is where I think it is, I think the proper thing to do is to use separate list_head in noop instead of using generic dispatch queue directly thus making noop consistent with other ioscheds.

I'm more worried about oops w/ cfq Arnaldo reported in this thread. I'll track that down as soon as I'm done with this one.

So either we disable merging for noop by setting REQ_NOMERGE in elevator_noop_add_request(), or we add a noop_list and do the dispatching like in the other io schedulers. I'd prefer the latter, merging is still beneficial for noop (and it has always done it).

For now, we should add the former.

Signed-off-by Jens Axboe <[email protected]>
drivers/block/noop-iosched.c +1 -0
@@ -9,6 +9,7 @@
 
 static void elevator_noop_add_request(request_queue_t *q, struct request *rq)
 {
+	rq->flags |= REQ_NOMERGE;
 	elv_dispatch_add_tail(q, rq);
 }
Signature · 1 line
Jens Axboe
#6 lore raw
Quoted 82 lines · Jens Axboe
Quoted 80 lines · Tejun Heo

Hi, guys.

Quoted 49 lines · Jens Axboe
Quoted 29 lines · Arnaldo Carvalho de Melo

Hi,

I'm getting the oops below when trying to use qemu with a kernel
built with just the noop iosched, I'm never had looked at this code before, so I did a quick hack that seems enough for my case.

Ah, this is with a fairly recent git tree (today), haven't checked
if it is present in 2.6.14.

Best Regards,

- Arnaldo

Unable to handle kernel paging request at virtual address c5f20f60

printing eip:
c01b0ecd
*pde = 00017067
*pte = 05f20000

Oops: 0000 [#1]
DEBUG_PAGEALLOC
Modules linked in:
CPU:    0
EIP:    0060:[<c01b0ecd>]    Not tainted VLI
EFLAGS: 00000046   (2.6.14acme)

EIP is at elv_rq_merge_ok+0x15/0x7b
eax: 00000014 ebx: c5f20f58 ecx: 000003f8 edx: 00000046
esi: c12a5a90 edi: c5f20f58 ebp: c11658d0 esp: c11658c4
ds: 007b es: 007b ss: 0068
Process swapper (pid: 1, threadinfo=c1165000 task=c1164af0)

Quoted 15 lines
Stack: c0251883 c5ecfe4c c5d688c0 c1165904 c01b0f48 c5f20f58 c12a5a90 00000000
    c5874000 c018c5e1 c5f15f24 0000002b 00000000 c5ecfe4c c5d688c0 
    c12a5a90
    c1165920 c01b128d c5f20f58 c12a5a90 000a568a 00000000 00000002 
    c1165960
Call Trace:
[<c0102a63>] show_stack+0x78/0x83
[<c0102b88>] show_registers+0x100/0x167
[<c0102d35>] die+0xcb/0x140
[<c0234308>] do_page_fault+0x393/0x53a
[<c0102777>] error_code+0x4f/0x54
[<c01b0f48>] elv_try_merge+0x15/0x84
[<c01b128d>] elv_merge+0x1d/0x4f
[<c01b41d9>] __make_request+0xb2/0x425
[<c01b46f9>] generic_make_request+0x125/0x137
Quoted 3 lines

Hrmpf, this looks really bad. Tejun, clearly there are still paths where ->last_rq isn't being cleared.

I'm currently debugging this. The problem is that we are using generic dispatch queue directly in the noop and merging is NOT allowed on dispatch queues but generic handling of last_merge tries to merge requests. I'm still trying to verify this, so I'll be back with results soon.

Quoted 11 lines
Quoted 11 lines
drivers/block/ll_rw_blk.c +3 -0
@@ -1787,6 +1787,9 @@ static inline void blk_free_request(requ
 	if (rq->flags & REQ_ELVPRIV)
 		elv_put_request(q, rq);
 	mempool_free(rq, q->rq.rq_pool);
+
+	if (rq == q->last_merge)
+		q->last_merge = NULL;
 }
Quoted 6 lines
Quoted 1 line

static inline struct request *

It's most likely a bug getting this far in the first place, but does it fix things for you? I'll get on this asap.

If the bug is where I think it is, I think the proper thing to do is to use separate list_head in noop instead of using generic dispatch queue directly thus making noop consistent with other ioscheds.

Quoted 7 lines
Quoted 1 line

I'm more worried about oops w/ cfq Arnaldo reported in this thread. I'll track that down as soon as I'm done with this one.

So either we disable merging for noop by setting REQ_NOMERGE in elevator_noop_add_request(), or we add a noop_list and do the dispatching like in the other io schedulers. I'd prefer the latter, merging is still beneficial for noop (and it has always done it).

Just verified. It happens when elv_merge_requests() happens. The merged request should be unlinked from list but noop does not have any merge handling callbacks ATM.

Sorry about the hassle. :-(

Quoted 2 lines

For now, we should add the former.

Yeap, also verified oops doesn't happen with the following patch.

I'll soon post a patch to convert noop such that it does proper dispatching. BTW, while I was looking at the code, I found something else, in elv_former/latter_request functions, if the iosched doesn't supply the callbacks, it uses rq->queue_list.prev/next implicitly (without this, this noop bug wouldn't have been triggered). I think this code is not necessary anymore. What do you think?

Thanks.

Signature · 1 line
tejun
#7 lore raw
Quoted 3 lines · Tejun Heo
Quoted 1 line

For now, we should add the former.

Yeap, also verified oops doesn't happen with the following patch.

Good

Quoted 2 lines

I'll soon post a patch to convert noop such that it does proper dispatching. BTW, while I was looking at the code, I found something

Sounds good.

Quoted 4 lines

else, in elv_former/latter_request functions, if the iosched doesn't supply the callbacks, it uses rq->queue_list.prev/next implicitly (without this, this noop bug wouldn't have been triggered). I think this code is not necessary anymore. What do you think?

Well that should still work for noop, if it has its own internal queueing list. But I would be quite fine with removing that implicit next/prev support and simply require io scheds to supply these functions always if they require rq-to-rq merging. Noop is the only one that relied on this in the past, now seems a good time to clean that up as well.

Besides, as you note, with merging disallowed on the ->queue_head, they don't work as expected anymore.

Signature · 1 line
Jens Axboe
#8 lore raw
In reply to Jens Axboe
Quoted 9 lines · Jens Axboe

So either we disable merging for noop by setting REQ_NOMERGE in elevator_noop_add_request(), or we add a noop_list and do the dispatching like in the other io schedulers. I'd prefer the latter, merging is still beneficial for noop (and it has always done it).

For now, we should add the former.

Signed-off-by Jens Axboe <[email protected]>

Btw, Jens, I appreciate seeing the discussion history when applying a patch, but at the same time I do _not_ want to use it as a commit message, it's just too confusing and worthless in that context.

And yet, your final comments don't much make sense without the background, so I can't just use them either.

So, I rewrote the explanation. Which is fine, but I wish people who sent patches would think more about what message they want to have in the commit logs, so that (a) Lazy-Linus doesn't have to write his own message and (b) so that the message is correct when Lazy-and-Stupid-Linus sometimes doesn't necessarily see/understand all the problems it fixes.

Btw, the email-patch-sending protocol still allows for putting all the ugly history in for my (and the mailing lists) pleasure: that's what the "---" marker after the explanation is for. So you can _both_ have a nice clean commit message _and_ give more of a historical background for the patch.

Anyway, in this case, the commit message ended up looking like this::

commit 581c1b14394aee60aff46ea67d05483261ed6527

Author: Jens Axboe <[email protected]>
Date:   Mon Oct 31 09:23:54 2005 +0100

[PATCH] noop-iosched: avoid corrupted request merging

Tejun Heo notes:

"I'm currently debugging this. The problem is that we are using the
generic dispatch queue directly in the noop sched and merging is NOT allowed on dispatch queues but generic handling of last_merge tries to merge requests. I'm still trying to verify this, so I'll be back with results soon."

In the meantime, disable merging for noop by setting REQ_NOMERGE in elevator_noop_add_request().

Eventually, we should add a noop_list and do the dispatching like in the other io schedulers. Merging is still beneficial for noop (and it has always done it).

Signed-off-by Jens Axboe <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>

which is basically your email cleaned up and compressed into a readable commit message.

Linus

#9 lore raw
Quoted 26 lines · Linus Torvalds
Quoted 9 lines · Jens Axboe

So either we disable merging for noop by setting REQ_NOMERGE in elevator_noop_add_request(), or we add a noop_list and do the dispatching like in the other io schedulers. I'd prefer the latter, merging is still beneficial for noop (and it has always done it).

For now, we should add the former.

Signed-off-by Jens Axboe <[email protected]>

Btw, Jens, I appreciate seeing the discussion history when applying a patch, but at the same time I do _not_ want to use it as a commit message, it's just too confusing and worthless in that context.

And yet, your final comments don't much make sense without the background, so I can't just use them either.

So, I rewrote the explanation. Which is fine, but I wish people who sent patches would think more about what message they want to have in the commit logs, so that (a) Lazy-Linus doesn't have to write his own message and (b) so that the message is correct when Lazy-and-Stupid-Linus sometimes doesn't necessarily see/understand all the problems it fixes.

Quoted 5 lines

Btw, the email-patch-sending protocol still allows for putting all the ugly history in for my (and the mailing lists) pleasure: that's what the "---" marker after the explanation is for. So you can _both_ have a nice clean commit message _and_ give more of a historical background for the patch.

My bad, I know I'm a little bad at describing patches sometimes (Andrew has been on my case in the past as well), I will make a conscious effort to describe them better.

Quoted 23 lines

Anyway, in this case, the commit message ended up looking like this::

commit 581c1b14394aee60aff46ea67d05483261ed6527

Author: Jens Axboe <[email protected]>
Date:   Mon Oct 31 09:23:54 2005 +0100

[PATCH] noop-iosched: avoid corrupted request merging

Tejun Heo notes:

"I'm currently debugging this. The problem is that we are using the
generic dispatch queue directly in the noop sched and merging is NOT allowed on dispatch queues but generic handling of last_merge tries to merge requests. I'm still trying to verify this, so I'll be back with results soon."

In the meantime, disable merging for noop by setting REQ_NOMERGE in elevator_noop_add_request().

Eventually, we should add a noop_list and do the dispatching like in the other io schedulers. Merging is still beneficial for noop (and it has always done it).

Quoted 5 lines
Signed-off-by Jens Axboe <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>

which is basically your email cleaned up and compressed into a readable commit message.

Indeed, looks good, thanks for writing it up!

Signature · 1 line
Jens Axboe