[bug] block subsystem related crash with latest -git - Kernel

This is a discussion on [bug] block subsystem related crash with latest -git - Kernel ; On Wed, Oct 17 2007, Linus Torvalds wrote: > > > On Wed, 17 Oct 2007, Jens Axboe wrote: > > > > OK, it is fine, as long as the sglist is cleared initially. And I don't > > ...

+ Reply to Thread
Page 2 of 8 FirstFirst 1 2 3 4 ... LastLast
Results 21 to 40 of 151

Thread: [bug] block subsystem related crash with latest -git

  1. Re: [bug] block subsystem related crash with latest -git

    On Wed, Oct 17 2007, Linus Torvalds wrote:
    >
    >
    > On Wed, 17 Oct 2007, Jens Axboe wrote:
    > >
    > > OK, it is fine, as long as the sglist is cleared initially. And I don't
    > > think there's anyway around that, clearly I didn't think long enough
    > > before including the memset() removal from Tomo.

    >
    > Ok, I think that one-liner fixes the real bug.
    >
    > But I think the rest of your changes are simply bad.
    >
    > The fix to block/ll_rw_block.c should likely be something like the
    > appended instead:
    >
    > - remove the "memset()" you had added earlier. It's bogus. It cannot be
    > the right thing. If the sg list wasn't initialized correctly much
    > earlier, trying to initialize it late is pointless - it contains crap.


    It's required to clear output members (like dma_len and so on), since
    some of the IOMU code really wants that initialized.

    > - the old code was fine, but let's initialize "sg" to NULL to make it
    > clear that the initial value of sg is pointless, and only "next_sg"
    > matters (since sg had better be assigned from that).


    If you prefer the old next_sg approach to my alternative, that is fine
    with me. But we do need the memset().

    --
    Jens Axboe

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [bug] block subsystem related crash with latest -git


    * Linus Torvalds wrote:

    > But I think the rest of your changes are simply bad.
    >
    > The fix to block/ll_rw_block.c should likely be something like the
    > appended instead:
    >
    > - remove the "memset()" you had added earlier. It's bogus. It cannot be
    > the right thing. If the sg list wasn't initialized correctly much
    > earlier, trying to initialize it late is pointless - it contains crap.
    >
    > - the old code was fine, but let's initialize "sg" to NULL to make it
    > clear that the initial value of sg is pointless, and only "next_sg"
    > matters (since sg had better be assigned from that).
    >
    > Hmm?


    built and booted your patch (removed Jens's) but it still crashes in
    blk_rq_map_sg() - see below. Should i have left something from Jens's
    patch?

    Ingo

    ------------------->
    [ 34.698199] VFS: Mounted root (ext3 filesystem) readonly.
    [ 34.703917] Freeing unused kernel memory: 372k freed
    [ 34.746609] BUG: unable to handle kernel paging request at virtual address 6e616872
    [ 34.754106] printing eip: 7840503b *pde = 00000000
    [ 34.758960] Oops: 0000 [#1] DEBUG_PAGEALLOC
    [ 34.763120]
    [ 34.764594] Pid: 1, comm: swapper Not tainted (2.6.23 #7)
    [ 34.769965] EIP: 0060:[<7840503b>] EFLAGS: 00010006 CPU: 0
    [ 34.775429] EIP is at blk_rq_map_sg+0xbb/0x170
    [ 34.779845] EAX: 3ffcd000 EBX: 7c88a68c ECX: 3ffce000 EDX: 007ff9a0
    [ 34.786085] ESI: 6e616862 EDI: 798ea9a0 EBP: 00001000 ESP: 7b421bf4
    [ 34.792324] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
    [ 34.797697] Process swapper (pid: 1, ti=7b420000 task=7b416000 task.ti=7b420000)
    [ 34.804890] Stack: 00000046 7b520000 00002000 3ffcf000 7c88ac80 00000001 00000001 00000001
    [ 34.813209] 7c88a600 01000000 7c88ab00 7b521d1c 7c88a708 7b520000 784c7ac5 7b520000
    [ 34.821529] 784c75ca 7c88a708 7b524ce4 7b521d1c 7c88a708 784e4570 7b416000 7813661b
    [ 34.829847] Call Trace:
    [ 34.832448] [<784c7ac5>] scsi_init_io+0x55/0xe0
    [ 34.837042] [<784c75ca>] scsi_get_cmd_from_req+0x2a/0x40
    [ 34.842414] [<784e4570>] sd_prep_fn+0x80/0x940
    [ 34.846920] [<7813661b>] __lock_acquire+0x4ab/0xe20
    [ 34.851860] [<781450a6>] add_to_page_cache+0x66/0xb0
    [ 34.856887] [<78404633>] elv_dispatch_sort+0x23/0xe0
    [ 34.861912] [<784041a0>] elv_next_request+0xa0/0x130
    [ 34.866938] [<784c8c04>] scsi_request_fn+0x1e4/0x370
    [ 34.871965] [<78120f02>] del_timer+0x62/0x70
    [ 34.876298] [<78407445>] __generic_unplug_device+0x25/0x30
    [ 34.881844] [<78407715>] generic_unplug_device+0x15/0x30
    [ 34.887217] [<78404e00>] blk_backing_dev_unplug+0x0/0x10
    [ 34.892590] [<78404e0c>] blk_backing_dev_unplug+0xc/0x10
    [ 34.897963] [<78180a9d>] block_sync_page+0x2d/0x40
    [ 34.902817] [<78144dc9>] sync_page+0x29/0x40
    [ 34.907150] [<7876f5bc>] __wait_on_bit_lock+0x3c/0x70
    [ 34.912263] [<78144da0>] sync_page+0x0/0x40
    [ 34.916509] [<78144d82>] __lock_page+0x52/0x60
    [ 34.921016] [<7812adb0>] wake_bit_function+0x0/0x60
    [ 34.925955] [<7814554c>] do_generic_mapping_read+0x21c/0x450
    [ 34.931675] [<78144b10>] file_read_actor+0x0/0x130
    [ 34.936528] [<78147107>] generic_file_aio_read+0x137/0x180
    [ 34.942074] [<78144b10>] file_read_actor+0x0/0x130
    [ 34.946927] [<78161245>] do_sync_read+0xd5/0x120
    [ 34.951607] [<78135646>] mark_lock+0x76/0x550
    [ 34.956027] [<7812ad70>] autoremove_wake_function+0x0/0x40
    [ 34.961573] [<78155026>] __vma_link+0x36/0x70
    [ 34.965993] [<78161170>] do_sync_read+0x0/0x120
    [ 34.970586] [<78161ae3>] vfs_read+0xb3/0x110
    [ 34.974919] [<781652cd>] kernel_read+0x3d/0x60
    [ 34.979425] [<7816539f>] prepare_binprm+0xaf/0xe0
    [ 34.984192] [<78166973>] do_execve+0x123/0x1c0
    [ 34.988698] [<78100f1f>] sys_execve+0x2f/0x90
    [ 34.993118] [<781028a2>] syscall_call+0x7/0xb
    [ 34.997538] [<78106f1c>] kernel_execve+0x1c/0x30
    [ 35.002217] [<7810016e>] init_post+0x9e/0xe0
    [ 35.006550] [<78a147b3>] kernel_init+0x163/0x280
    [ 35.011230] [<78a14650>] kernel_init+0x0/0x280
    [ 35.015736] [<78103a97>] kernel_thread_helper+0x7/0x10
    [ 35.020936] =======================
    [ 35.024489] Code: 29 c1 c1 f9 05 c1 e1 0c 03 4a 08 8b 52 04 01 ca 89 54 24 0c 8b 3b 89 fa 29 c2 89 d0 c1 f8 05 c1 e0 0c 03 43 08 39 44 24 0c 74 7e <8b> 46 10 8d 56 10 a8 01 75 4c 89 3e 89 6e 0c 8b 43 08 89 46 04
    [ 35.043294] EIP: [<7840503b>] blk_rq_map_sg+0xbb/0x170 SS:ESP 0068:7b421bf4
    [ 35.050228] Kernel panic - not syncing: Attempted to kill init!
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [bug] ata subsystem related crash with latest -git


    * Jens Axboe wrote:

    > Also, can you send a dmesg from that system so I can see which libata
    > drivers are involved?


    same system for which i sent the earlier bootlog - but here's the second
    crashlog too. Crash is reproducible - crashed twice in a row already.
    (NOTE: the attached log is from this second crash.)

    Ingo


  4. Re: [bug] block subsystem related crash with latest -git



    On Wed, 17 Oct 2007, Jens Axboe wrote:
    > >
    > > - remove the "memset()" you had added earlier. It's bogus. It cannot be
    > > the right thing. If the sg list wasn't initialized correctly much
    > > earlier, trying to initialize it late is pointless - it contains crap.

    >
    > It's required to clear output members (like dma_len and so on), since
    > some of the IOMU code really wants that initialized.


    No it's NOT!

    The whole point here is that the sg had *already* better be cleared.

    If it wasn't cleared before, that's a bug regardless of anything else. So
    a memset() is guaranteed to be either a no-op or hiding another bug!

    So yes, those members had better be zero. It's just that they had better
    be zero long before that memset! The memset should have been done when
    allocating the SG list.

    > If you prefer the old next_sg approach to my alternative, that is fine
    > with me. But we do need the memset().


    Really? Explain why. Entering that code with a non-initialized SG list is
    a bug to begin with.

    Linus
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [bug] block subsystem related crash with latest -git



    On Wed, 17 Oct 2007, Jens Axboe wrote:
    > >
    > > So avoiding the "sg_next()" on the last entry is pointless.

    >
    > Yeah, I didn't quite understand why if sg was valid, why dereferencing
    > *(sg + 1)->page would crap out :/


    Actually, I take that back. If 'sg' is the last entry in a *non*linked
    scatter-gather list (ie we don't use the last entry as a link, we actually
    use it as a real SG entry), then "sg_next(sg)" will indeed access past the
    end of the whole allocated array, and will access one past the end.

    And with page-alloc debugging, that *will* blow up.

    So I think your change to use "sg_next()" only when you actually need a
    next pointer is the correct one after all.

    Linus
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [bug] ata subsystem related crash with latest -git



    On Wed, 17 Oct 2007, Ingo Molnar wrote:
    >
    > ok, here's a different but similar crash that triggers on the testbox:


    Just to confirm - is this with the added memset() in
    drivers/scsi/scsi_lib.c?

    Or did you get this oops before that patch?

    Linus
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [bug] block subsystem related crash with latest -git



    On Wed, 17 Oct 2007, Ingo Molnar wrote:
    >
    > built and booted your patch (removed Jens's) but it still crashes in
    > blk_rq_map_sg() - see below. Should i have left something from Jens's
    > patch?


    The *real* fix in Jens' patch (and the only thing that really matters) is
    the one to drivers/scsi/scsi_lib.c.

    The rest is all fluff.

    Linus
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [bug] ata subsystem related crash with latest -git


    * Linus Torvalds wrote:

    >
    >
    > On Wed, 17 Oct 2007, Ingo Molnar wrote:
    > >
    > > ok, here's a different but similar crash that triggers on the testbox:

    >
    > Just to confirm - is this with the added memset() in
    > drivers/scsi/scsi_lib.c?


    yes, Jens's last patch was applied - apparently this ATA crash was
    masked by the other one. I got one successful bootup after applying
    Jens's patch and it has crashed on bootup twice since then.

    Ingo
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [bug] block subsystem related crash with latest -git

    On Wed, Oct 17 2007, Linus Torvalds wrote:
    >
    >
    > On Wed, 17 Oct 2007, Jens Axboe wrote:
    > > >
    > > > - remove the "memset()" you had added earlier. It's bogus. It cannot be
    > > > the right thing. If the sg list wasn't initialized correctly much
    > > > earlier, trying to initialize it late is pointless - it contains crap.

    > >
    > > It's required to clear output members (like dma_len and so on), since
    > > some of the IOMU code really wants that initialized.

    >
    > No it's NOT!
    >
    > The whole point here is that the sg had *already* better be cleared.
    >
    > If it wasn't cleared before, that's a bug regardless of anything else. So
    > a memset() is guaranteed to be either a no-op or hiding another bug!
    >
    > So yes, those members had better be zero. It's just that they had better
    > be zero long before that memset! The memset should have been done when
    > allocating the SG list.


    For the chain elements - yes, definitely! But we also want to clear dma
    mapping output values, at least sparc64 wants that. You could argue that
    the IOMMU code should be fixed up, but I don't think we should mix the
    two.

    So we need the memset() in blk_rq_map_sg() AS WELL AS the initial sg
    table clear. I'm not arguing about the latter, we clearly do need that.

    > > If you prefer the old next_sg approach to my alternative, that is fine
    > > with me. But we do need the memset().

    >
    > Really? Explain why. Entering that code with a non-initialized SG list is
    > a bug to begin with.


    Not for the output members, they will be filled AFTER blk_rq_map_sg()
    returns and someone use iommu_map_sg() to re-iterate the sglist and map
    the pages appropriately.

    --
    Jens Axboe

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: [bug] block subsystem related crash with latest -git

    On Wed, Oct 17 2007, Linus Torvalds wrote:
    >
    >
    > On Wed, 17 Oct 2007, Jens Axboe wrote:
    > > >
    > > > So avoiding the "sg_next()" on the last entry is pointless.

    > >
    > > Yeah, I didn't quite understand why if sg was valid, why dereferencing
    > > *(sg + 1)->page would crap out :/

    >
    > Actually, I take that back. If 'sg' is the last entry in a *non*linked
    > scatter-gather list (ie we don't use the last entry as a link, we actually
    > use it as a real SG entry), then "sg_next(sg)" will indeed access past the
    > end of the whole allocated array, and will access one past the end.
    >
    > And with page-alloc debugging, that *will* blow up.
    >
    > So I think your change to use "sg_next()" only when you actually need a
    > next pointer is the correct one after all.


    Thanks, so I'm not totally crazy :-)

    Can you just pull:

    git://git.kernel.dk/data/git/linux-2.6-block.git for-linus

    then so we get those two pieces correct? Then the remaining issue seems
    to be a new one that is biting Ingo elsewhere, at least we'll all be on
    the same page then.

    --
    Jens Axboe

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. Re: [bug] block subsystem related crash with latest -git



    On Wed, 17 Oct 2007, Linus Torvalds wrote:
    >
    > So I think your change to use "sg_next()" only when you actually need a
    > next pointer is the correct one after all.


    That still leaves the initialization issue. The link pointers need to all
    be initialized at SG allocation time (and not just the last one for the
    case where it's a linked entry).

    And if you initialize them at allocation time, does an allocation ever get
    re-used without being free'd in between (if it does, you do indeed need to
    initialize the non-link pointers each time - but I don't see it being the
    case, so ..)

    Linus
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  12. Re: [bug] ata subsystem related crash with latest -git

    On Wed, Oct 17 2007, Jens Axboe wrote:
    > On Wed, Oct 17 2007, Ingo Molnar wrote:
    > >
    > > ok, here's a different but similar crash that triggers on the testbox:
    > >
    > > [ 233.438890] BUG: unable to handle kernel paging request at virtual address 7d93e000
    > > [ 233.446390] printing eip: 784e9480 *pde = 01000067 *pte = 0593e000
    > > [ 233.452630] Oops: 0000 [#1] DEBUG_PAGEALLOC
    > > [ 233.456790]
    > > [ 233.458264] Pid: 0, comm: swapper Not tainted (2.6.23 #5)
    > > [ 233.463637] EIP: 0060:[<784e9480>] EFLAGS: 00010087 CPU: 0
    > > [ 233.469101] EIP is at ata_qc_issue+0x90/0x380
    > > [ 233.473429] EAX: 7d93dff0 EBX: 0000001f ECX: 7d93dff0 EDX: 798daf80
    > > [ 233.479668] ESI: 00000020 EDI: 7d93de00 EBP: 7b54007c ESP: 78a13e14
    > > [ 233.485908] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
    > > [ 233.491282] Process swapper (pid: 0, ti=78a12000 task=789753e0 task.ti=78a12000)
    > > [ 233.498473] Stack: 7d93de00 7b540000 7b540000 00000000 7d93dfe0 7b54007c 7d93db00 7b5417a4
    > > [ 233.506793] 784c2490 784ef69e 784f21f3 7b52de98 7d93db00 7b540000 7b5417a4 7d93db00
    > > [ 233.515112] 7b540000 7b524004 784f22e0 784ef380 784c2490 7d93db00 00000202 7b524004
    > > [ 233.523432] Call Trace:
    > > [ 233.526033] [<784c2490>] scsi_done+0x0/0x20
    > > [ 233.530279] [<784ef69e>] ata_scsi_translate+0xbe/0x140
    > > [ 233.535478] [<784f21f3>] ata_scsi_queuecmd+0x33/0x200
    > > [ 233.540591] [<784f22e0>] ata_scsi_queuecmd+0x120/0x200
    > > [ 233.545791] [<784ef380>] ata_scsi_rw_xlat+0x0/0x220
    > > [ 233.550730] [<784c2490>] scsi_done+0x0/0x20
    > > [ 233.554976] [<784c2d12>] scsi_dispatch_cmd+0x152/0x290
    > > [ 233.560177] [<78135c67>] trace_hardirqs_on+0x67/0xb0
    > > [ 233.565202] [<784c8c7e>] scsi_request_fn+0x1be/0x370
    > > [ 233.570229] [<78408086>] blk_run_queue+0x36/0x80
    > > [ 233.574909] [<784c7520>] scsi_next_command+0x30/0x50
    > > [ 233.579935] [<784c76ab>] scsi_end_request+0xab/0xe0
    > > [ 233.584875] [<784c83f9>] scsi_io_completion+0xa9/0x3d0
    > > [ 233.590075] [<78135c67>] trace_hardirqs_on+0x67/0xb0
    > > [ 233.595100] [<78405125>] blk_done_softirq+0x45/0x80
    > > [ 233.600040] [<78405153>] blk_done_softirq+0x73/0x80
    > > [ 233.604981] [<7811d4c3>] __do_softirq+0x53/0xb0
    > > [ 233.609573] [<7811d588>] do_softirq+0x68/0x70
    > > [ 233.613993] [<78105351>] do_IRQ+0x51/0x90
    > > [ 233.618066] [<78135c9c>] trace_hardirqs_on+0x9c/0xb0
    > > [ 233.623092] [<7810f2d0>] pgd_dtor+0x0/0x50
    > > [ 233.627252] [<7810388e>] common_interrupt+0x2e/0x40
    > > [ 233.632192] [<7810f2d0>] pgd_dtor+0x0/0x50
    > > [ 233.636352] [<7815f3be>] quicklist_trim+0x5e/0x90
    > > [ 233.641118] [<7810f2cb>] check_pgt_cache+0x1b/0x20
    > > [ 233.645971] [<78100c52>] cpu_idle+0x32/0x60
    > > [ 233.650217] [<78a14b35>] start_kernel+0x265/0x300
    > > [ 233.654983] [<78a14380>] unknown_bootoption+0x0/0x1e0
    > > [ 233.660097] =======================
    > > [ 233.663649] Code: 00 00 00 8b 45 34 a8 02 0f 84 ed 00 00 00 8b bd 88 00 00 00 31 db 89 3c 24 8b 75 3c 89 f8 c7 44 24 10 00 00 00 00 eb 1b 8d 76 00 <8b> 50 10 8d 48 10 f6 c2 01 0f 85 be 02 00 00 89 44 24 10 83 c3
    > > [ 233.682455] EIP: [<784e9480>] ata_qc_issue+0x90/0x380 SS:ESP 0068:78a13e14
    > > [ 233.689302] Kernel panic - not syncing: Fatal exception in interrupt
    > >
    > > (gdb) list *0x784e9480
    > > 0x784e9480 is in ata_qc_issue (include/linux/scatterlist.h:48).
    > > 43 */
    > > 44 static inline struct scatterlist *sg_next(struct scatterlist *sg)
    > > 45 {
    > > 46 sg++;
    > > 47
    > > 48 if (unlikely(sg_is_chain(sg)))
    > > 49 sg = sg_chain_ptr(sg);
    > > 50
    > > 51 return sg;
    > > 52 }
    > > (gdb)
    > >
    > > so there's sg_next() involvement too. Below is the disassembly.

    >
    > You must have a magic test box :-)
    >
    > Will investigate... libata doesn't actually enable chaining, but since
    > i386 supports it, it ends up using the chain helpers anyway.
    >
    > There seems to be some automatic inlining involved here, it must be
    > dying inside ata_sg_setup().


    OK, something to try out - libata doesn't enable sg chaining, since the
    support isn't complete yet. Here's a dumb check just to verify that
    scsi_alloc_sgtable() will NEVER return a chain entry for a host that
    doesn't have it enabled. If this triggers for you, then that could
    explain your problem.

    diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
    index aac8a02..cc89c86 100644
    --- a/drivers/scsi/scsi_lib.c
    +++ b/drivers/scsi/scsi_lib.c
    @@ -777,8 +777,12 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
    * sglist must be the biggest one, or we would not have
    * ended up doing another loop.
    */
    - if (prev)
    + if (prev) {
    + struct Scsi_Host *shost = cmd->device->host;
    +
    + BUG_ON(!shost->use_sg_chaining);
    sg_chain(prev, SCSI_MAX_SG_SEGMENTS, sgl);
    + }

    /*
    * don't allow subsequent mempool allocs to sleep, it would

    --
    Jens Axboe

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  13. Re: [bug] block subsystem related crash with latest -git

    On Wed, Oct 17 2007, Linus Torvalds wrote:
    >
    >
    > On Wed, 17 Oct 2007, Linus Torvalds wrote:
    > >
    > > So I think your change to use "sg_next()" only when you actually need a
    > > next pointer is the correct one after all.

    >
    > That still leaves the initialization issue. The link pointers need to all
    > be initialized at SG allocation time (and not just the last one for the
    > case where it's a linked entry).
    >
    > And if you initialize them at allocation time, does an allocation ever get
    > re-used without being free'd in between (if it does, you do indeed need to
    > initialize the non-link pointers each time - but I don't see it being the
    > case, so ..)


    The links will not change, so reuse should actually be OK. The reuse
    cannot be larger than the original would hold, but that would be a bug
    without chaining as well of course.

    WRT reuse, if we end up requeuing a request, than we will go through the
    whole thing again. I don't think Ingo is hitting that either, though.

    --
    Jens Axboe

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  14. Re: [bug] block subsystem related crash with latest -git



    On Wed, 17 Oct 2007, Jens Axboe wrote:
    >
    > For the chain elements - yes, definitely! But we also want to clear dma
    > mapping output values, at least sparc64 wants that. You could argue that
    > the IOMMU code should be fixed up, but I don't think we should mix the
    > two.
    >
    > So we need the memset() in blk_rq_map_sg() AS WELL AS the initial sg
    > table clear. I'm not arguing about the latter, we clearly do need that.


    I still don't see your argument.

    The SG table is *already*zeroed*.

    The fact that output values will be changed later is irrelevant. They
    haven't been changed *before* - or at least you haven't given a reasonable
    explanation for why they should have.

    So explain to me why the memset() in blk_rq_map_sg() helps, considering
    that the memory must have been zeroed at allocation time anyway? Tell me
    what it is that fills any of the fields we don't already initialize, and
    that can happen *before* that memset?

    Linus
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  15. Re: [bug] block subsystem related crash with latest -git

    On Wed, Oct 17 2007, Linus Torvalds wrote:
    >
    >
    > On Wed, 17 Oct 2007, Jens Axboe wrote:
    > >
    > > For the chain elements - yes, definitely! But we also want to clear dma
    > > mapping output values, at least sparc64 wants that. You could argue that
    > > the IOMMU code should be fixed up, but I don't think we should mix the
    > > two.
    > >
    > > So we need the memset() in blk_rq_map_sg() AS WELL AS the initial sg
    > > table clear. I'm not arguing about the latter, we clearly do need that.

    >
    > I still don't see your argument.
    >
    > The SG table is *already*zeroed*.
    >
    > The fact that output values will be changed later is irrelevant. They
    > haven't been changed *before* - or at least you haven't given a reasonable
    > explanation for why they should have.
    >
    > So explain to me why the memset() in blk_rq_map_sg() helps, considering
    > that the memory must have been zeroed at allocation time anyway? Tell me
    > what it is that fills any of the fields we don't already initialize, and
    > that can happen *before* that memset?


    Ah ok, I see why you are confused. The SCSI case is one, it allocs and
    frees the sg table each time. The entries are thus always initialized
    when they end up in blk_rq_map_sg(). However, other drivers allocate one
    at driver init time and use that one all the time. It needs to be
    initially cleared, but it wont be cleared before each request mapping.
    And that is fine, as long as we do clear entries in blk_rq_map_sg().

    --
    Jens Axboe

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  16. Re: [bug] ata subsystem related crash with latest -git


    btw., i get the following build warning:

    drivers/scsi/scsi_lib.c: In function 'scsi_free_sgtable':
    drivers/scsi/scsi_lib.c:708: warning: 'index' may be used uninitialized in this function
    drivers/scsi/scsi_lib.c:708: note: 'index' was declared here
    drivers/scsi/scsi_lib.c:708: warning: 'index' may be used uninitialized in this function
    drivers/scsi/scsi_lib.c:708: note: 'index' was declared here
    drivers/scsi/scsi_lib.c: In function 'scsi_alloc_sgtable':
    drivers/scsi/scsi_lib.c:708: warning: 'index' may be used uninitialized in this function
    drivers/scsi/scsi_lib.c:708: note: 'index' was declared here

    not sure it matters.

    Ingo
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  17. Re: [bug] ata subsystem related crash with latest -git


    * Jens Axboe wrote:

    > @@ -777,8 +777,12 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
    > * sglist must be the biggest one, or we would not have
    > * ended up doing another loop.
    > */
    > - if (prev)
    > + if (prev) {
    > + struct Scsi_Host *shost = cmd->device->host;
    > +
    > + BUG_ON(!shost->use_sg_chaining);
    > sg_chain(prev, SCSI_MAX_SG_SEGMENTS, sgl);
    > + }
    >


    nope, this did not help. First bootup went fine, second bootup crashed
    again (see below), without hitting the BUG_ON().

    Ingo

    [ 39.351198] BUG: unable to handle kernel paging request at virtual address 7ca58000
    [ 39.358698] printing eip: 784e9480 *pde = 00dda027 *pte = 04a58000
    [ 39.364939] Oops: 0000 [#1] DEBUG_PAGEALLOC
    [ 39.369099]
    [ 39.370573] Pid: 0, comm: swapper Not tainted (2.6.23 #9)
    [ 39.375944] EIP: 0060:[<784e9480>] EFLAGS: 00010087 CPU: 0
    [ 39.381408] EIP is at ata_qc_issue+0x90/0x380
    [ 39.385737] EAX: 7ca57ff0 EBX: 0000001f ECX: 7ca57ff0 EDX: 79181c20
    [ 39.391977] ESI: 00000020 EDI: 7ca57e00 EBP: 7b54007c ESP: 78a13e10
    [ 39.398217] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
    [ 39.403590] Process swapper (pid: 0, ti=78a12000 task=789753e0 task.ti=78a12000)
    [ 39.410781] Stack: 7ca57e00 7b540000 7b540000 00000000 7ca57fe0 7b54007c 7c991980 7b5417a4
    [ 39.419101] 784c2490 784ef69e 784f21f3 7b52de98 7c991980 7b540000 7b5417a4 7c991980
    [ 39.427421] 7b540000 7b524004 784f22e0 784ef380 784c2490 7c991980 00000206 7b524004
    [ 39.435740] Call Trace:
    [ 39.438340] [<784c2490>] scsi_done+0x0/0x20
    [ 39.442586] [<784ef69e>] ata_scsi_translate+0xbe/0x140
    [ 39.447785] [<784f21f3>] ata_scsi_queuecmd+0x33/0x200
    [ 39.452899] [<784f22e0>] ata_scsi_queuecmd+0x120/0x200
    [ 39.458099] [<784ef380>] ata_scsi_rw_xlat+0x0/0x220
    [ 39.463038] [<784c2490>] scsi_done+0x0/0x20
    [ 39.467285] [<784c2d12>] scsi_dispatch_cmd+0x152/0x290
    [ 39.472484] [<78135c67>] trace_hardirqs_on+0x67/0xb0
    [ 39.477511] [<784c8c7e>] scsi_request_fn+0x1be/0x370
    [ 39.482538] [<78408086>] blk_run_queue+0x36/0x80
    [ 39.487217] [<784c7520>] scsi_next_command+0x30/0x50
    [ 39.492243] [<784c76ab>] scsi_end_request+0xab/0xe0
    [ 39.497183] [<784c83f9>] scsi_io_completion+0xa9/0x3d0
    [ 39.502383] [<78135c67>] trace_hardirqs_on+0x67/0xb0
    [ 39.507409] [<78405125>] blk_done_softirq+0x45/0x80
    [ 39.512348] [<78405153>] blk_done_softirq+0x73/0x80
    [ 39.517288] [<7811d4c3>] __do_softirq+0x53/0xb0
    [ 39.521882] [<7811d588>] do_softirq+0x68/0x70
    [ 39.526302] [<78105351>] do_IRQ+0x51/0x90
    [ 39.530374] [<78770a54>] _spin_unlock+0x14/0x20
    [ 39.534968] [<7810290f>] restore_nocheck+0x12/0x15
    [ 39.539820] [<7810f2d0>] pgd_dtor+0x0/0x50
    [ 39.543980] [<7810388e>] common_interrupt+0x2e/0x40
    [ 39.548920] [<7810f2d0>] pgd_dtor+0x0/0x50
    [ 39.553079] [<7815f3bd>] quicklist_trim+0x5d/0x90
    [ 39.557846] [<7810f2cb>] check_pgt_cache+0x1b/0x20
    [ 39.562698] [<78100c52>] cpu_idle+0x32/0x60
    [ 39.566945] [<78a14b35>] start_kernel+0x265/0x300
    [ 39.571712] [<78a14380>] unknown_bootoption+0x0/0x1e0
    [ 39.576825] =======================
    [ 39.580377] Code: 00 00 00 8b 45 34 a8 02 0f 84 ed 00 00 00 8b bd 88 00 00 00 31 db 89 3c 24 8b 75 3c 89 f8 c7 44 24 10 00 00 00 00 eb 1b 8d 76 00 <8b> 50 10 8d 48 10 f6 c2 01 0f 85 be 02 00 00 89 44 24 10 83 c3
    [ 39.599183] EIP: [<784e9480>] ata_qc_issue+0x90/0x380 SS:ESP 0068:78a13e10
    [ 39.606030] Kernel panic - not syncing: Fatal exception in interrupt
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  18. Re: [bug] ata subsystem related crash with latest -git

    On Wed, Oct 17 2007, Ingo Molnar wrote:
    >
    > btw., i get the following build warning:
    >
    > drivers/scsi/scsi_lib.c: In function 'scsi_free_sgtable':
    > drivers/scsi/scsi_lib.c:708: warning: 'index' may be used uninitialized in this function
    > drivers/scsi/scsi_lib.c:708: note: 'index' was declared here
    > drivers/scsi/scsi_lib.c:708: warning: 'index' may be used uninitialized in this function
    > drivers/scsi/scsi_lib.c:708: note: 'index' was declared here
    > drivers/scsi/scsi_lib.c: In function 'scsi_alloc_sgtable':
    > drivers/scsi/scsi_lib.c:708: warning: 'index' may be used uninitialized in this function
    > drivers/scsi/scsi_lib.c:708: note: 'index' was declared here
    >
    > not sure it matters.


    Hmm, I don't see them here (gcc 4.2.1). Must be the BUG(), does it go
    away if you add a index = -1 in the default: case? Just to be absolutely
    sure...

    --
    Jens Axboe

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  19. Re: [bug] block subsystem related crash with latest -git



    On Wed, 17 Oct 2007, Jens Axboe wrote:
    >
    > Ah ok, I see why you are confused. The SCSI case is one, it allocs and
    > frees the sg table each time. The entries are thus always initialized
    > when they end up in blk_rq_map_sg(). However, other drivers allocate one
    > at driver init time and use that one all the time. It needs to be
    > initially cleared, but it wont be cleared before each request mapping.
    > And that is fine, as long as we do clear entries in blk_rq_map_sg().


    Ahh, ok, thanks for clearing that up.

    Linus
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  20. Re: [bug] ata subsystem related crash with latest -git


    * Ingo Molnar wrote:

    > > > drivers/scsi/scsi_lib.c:708: warning: 'index' may be used uninitialized in this function
    > > > drivers/scsi/scsi_lib.c:708: note: 'index' was declared here
    > > >
    > > > not sure it matters.

    > >
    > > Hmm, I don't see them here (gcc 4.2.1). Must be the BUG(), does it go
    > > away if you add a index = -1 in the default: case? Just to be
    > > absolutely sure...

    >
    > well if i initialize the variable then of course the warning goes away
    >
    >
    > NOTE: i get the same warning even without the BUG_ON() patch, and
    > without your other fix patch as well. I'm using gcc 4.2.2. (Do you get
    > the warning if you pick up the config i sent you earlier today?)


    btw., i changed the initialization to -1 and the crash still occurs
    (as expected).

    Ingo
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread
Page 2 of 8 FirstFirst 1 2 3 4 ... LastLast