* [PATCH][noop-iosched] don't reuse a freed request
@ 2005-10-31 2:30 Arnaldo Carvalho de Melo
2005-10-31 2:55 ` Arnaldo Carvalho de Melo
2005-10-31 7:40 ` Jens Axboe
0 siblings, 2 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2005-10-31 2:30 UTC (permalink / raw)
To: Jens Axboe, Tejun Heo; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]
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!
[-- Attachment #2: noop-iosched.patch --]
[-- Type: text/plain, Size: 311 bytes --]
--- a/drivers/block/ll_rw_blk.c
+++ b/drivers/block/ll_rw_blk.c
@@ -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 *
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][noop-iosched] don't reuse a freed request
2005-10-31 2:30 [PATCH][noop-iosched] don't reuse a freed request Arnaldo Carvalho de Melo
@ 2005-10-31 2:55 ` Arnaldo Carvalho de Melo
2005-10-31 3:52 ` Arnaldo Carvalho de Melo
2005-10-31 7:40 ` Jens Axboe
1 sibling, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2005-10-31 2:55 UTC (permalink / raw)
To: Jens Axboe, Tejun Heo, linux-kernel
Em Mon, Oct 31, 2005 at 12:30:24AM -0200, Arnaldo Carvalho de Melo escreveu:
> 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!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][noop-iosched] don't reuse a freed request
2005-10-31 2:55 ` Arnaldo Carvalho de Melo
@ 2005-10-31 3:52 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2005-10-31 3:52 UTC (permalink / raw)
To: Jens Axboe, Tejun Heo, linux-kernel
Em Mon, Oct 31, 2005 at 12:55:26AM -0200, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Oct 31, 2005 at 12:30:24AM -0200, Arnaldo Carvalho de Melo escreveu:
> > 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
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][noop-iosched] don't reuse a freed request
2005-10-31 2:30 [PATCH][noop-iosched] don't reuse a freed request Arnaldo Carvalho de Melo
2005-10-31 2:55 ` Arnaldo Carvalho de Melo
@ 2005-10-31 7:40 ` Jens Axboe
2005-10-31 8:04 ` Tejun Heo
1 sibling, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2005-10-31 7:40 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Tejun Heo, linux-kernel
On Mon, Oct 31 2005, Arnaldo Carvalho de Melo wrote:
> 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
Hrmpf, this looks really bad. Tejun, clearly there are still paths where
->last_rq isn't being cleared.
> --- a/drivers/block/ll_rw_blk.c
> +++ b/drivers/block/ll_rw_blk.c
> @@ -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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][noop-iosched] don't reuse a freed request
2005-10-31 7:40 ` Jens Axboe
@ 2005-10-31 8:04 ` Tejun Heo
2005-10-31 8:23 ` Jens Axboe
0 siblings, 1 reply; 10+ messages in thread
From: Tejun Heo @ 2005-10-31 8:04 UTC (permalink / raw)
To: Jens Axboe; +Cc: Arnaldo Carvalho de Melo, linux-kernel
Hi, guys.
Jens Axboe wrote:
> On Mon, Oct 31 2005, Arnaldo Carvalho de Melo wrote:
>
>>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
>
>
> 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.
>
>>--- a/drivers/block/ll_rw_blk.c
>>+++ b/drivers/block/ll_rw_blk.c
>>@@ -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.
>
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. :-)
--
tejun
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][noop-iosched] don't reuse a freed request
2005-10-31 8:04 ` Tejun Heo
@ 2005-10-31 8:23 ` Jens Axboe
2005-10-31 8:59 ` Tejun Heo
2005-10-31 15:53 ` Linus Torvalds
0 siblings, 2 replies; 10+ messages in thread
From: Jens Axboe @ 2005-10-31 8:23 UTC (permalink / raw)
To: Tejun Heo; +Cc: Arnaldo Carvalho de Melo, linux-kernel, Linus Torvalds
On Mon, Oct 31 2005, Tejun Heo wrote:
> Hi, guys.
>
> Jens Axboe wrote:
> >On Mon, Oct 31 2005, Arnaldo Carvalho de Melo wrote:
> >
> >>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
> >
> >
> >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.
>
> >
> >>--- a/drivers/block/ll_rw_blk.c
> >>+++ b/drivers/block/ll_rw_blk.c
> >>@@ -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.
> >
>
> 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 <axboe@suse.de>
diff --git a/drivers/block/noop-iosched.c b/drivers/block/noop-iosched.c
--- a/drivers/block/noop-iosched.c
+++ b/drivers/block/noop-iosched.c
@@ -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);
}
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][noop-iosched] don't reuse a freed request
2005-10-31 8:23 ` Jens Axboe
@ 2005-10-31 8:59 ` Tejun Heo
2005-10-31 9:11 ` Jens Axboe
2005-10-31 15:53 ` Linus Torvalds
1 sibling, 1 reply; 10+ messages in thread
From: Tejun Heo @ 2005-10-31 8:59 UTC (permalink / raw)
To: Jens Axboe; +Cc: Arnaldo Carvalho de Melo, linux-kernel, Linus Torvalds
Jens Axboe wrote:
> On Mon, Oct 31 2005, Tejun Heo wrote:
>
>>Hi, guys.
>>
>>Jens Axboe wrote:
>>
>>>On Mon, Oct 31 2005, Arnaldo Carvalho de Melo wrote:
>>>
>>>
>>>>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
>>>
>>>
>>>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.
>>
>>
>>>>--- a/drivers/block/ll_rw_blk.c
>>>>+++ b/drivers/block/ll_rw_blk.c
>>>>@@ -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.
>>>
>>
>>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).
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. :-(
>
> 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.
--
tejun
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][noop-iosched] don't reuse a freed request
2005-10-31 8:59 ` Tejun Heo
@ 2005-10-31 9:11 ` Jens Axboe
0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2005-10-31 9:11 UTC (permalink / raw)
To: Tejun Heo; +Cc: Arnaldo Carvalho de Melo, linux-kernel, Linus Torvalds
On Mon, Oct 31 2005, Tejun Heo wrote:
> >For now, we should add the former.
>
> Yeap, also verified oops doesn't happen with the following patch.
Good
> 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.
> 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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][noop-iosched] don't reuse a freed request
2005-10-31 8:23 ` Jens Axboe
2005-10-31 8:59 ` Tejun Heo
@ 2005-10-31 15:53 ` Linus Torvalds
2005-10-31 18:03 ` Jens Axboe
1 sibling, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2005-10-31 15:53 UTC (permalink / raw)
To: Jens Axboe; +Cc: Tejun Heo, Arnaldo Carvalho de Melo, linux-kernel
On Mon, 31 Oct 2005, Jens Axboe wrote:
>
> 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 <axboe@suse.de>
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 <axboe@suse.de>
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 <axboe@suse.de>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
which is basically your email cleaned up and compressed into a readable
commit message.
Linus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][noop-iosched] don't reuse a freed request
2005-10-31 15:53 ` Linus Torvalds
@ 2005-10-31 18:03 ` Jens Axboe
0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2005-10-31 18:03 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Tejun Heo, Arnaldo Carvalho de Melo, linux-kernel
On Mon, Oct 31 2005, Linus Torvalds wrote:
>
>
> On Mon, 31 Oct 2005, Jens Axboe wrote:
> >
> > 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 <axboe@suse.de>
>
> 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.
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.
> Anyway, in this case, the commit message ended up looking like this::
>
> commit 581c1b14394aee60aff46ea67d05483261ed6527
> Author: Jens Axboe <axboe@suse.de>
> 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 <axboe@suse.de>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
>
> which is basically your email cleaned up and compressed into a readable
> commit message.
Indeed, looks good, thanks for writing it up!
--
Jens Axboe
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2005-10-31 18:02 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-10-31 2:30 [PATCH][noop-iosched] don't reuse a freed request Arnaldo Carvalho de Melo
2005-10-31 2:55 ` Arnaldo Carvalho de Melo
2005-10-31 3:52 ` Arnaldo Carvalho de Melo
2005-10-31 7:40 ` Jens Axboe
2005-10-31 8:04 ` Tejun Heo
2005-10-31 8:23 ` Jens Axboe
2005-10-31 8:59 ` Tejun Heo
2005-10-31 9:11 ` Jens Axboe
2005-10-31 15:53 ` Linus Torvalds
2005-10-31 18:03 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox