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

This is a discussion on [bug] block subsystem related crash with latest -git - Kernel ; On Thu, Oct 18, 2007 at 06:42:46AM -0400, Jeff Garzik wrote: > Jens Axboe wrote: >> On Thu, Oct 18 2007, Jeff Garzik wrote: >>> Jens Axboe wrote: >>>> That should work as well. WRT ata_sg_is_last(), if we go ahead ...

+ Reply to Thread
Page 8 of 8 FirstFirst ... 6 7 8
Results 141 to 151 of 151

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

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

    On Thu, Oct 18, 2007 at 06:42:46AM -0400, Jeff Garzik wrote:
    > Jens Axboe wrote:
    >> On Thu, Oct 18 2007, Jeff Garzik wrote:
    >>> Jens Axboe wrote:
    >>>> That should work as well. WRT ata_sg_is_last(), if we go ahead with my
    >>>> recent sg chaining updates, we can keep the test as it would be a single
    >>>> conditional and not require any looping.
    >>>> Let me know when you have tested this!
    >>> The patch I attached to the last email got both sata_mv test boxes
    >>> working reliably (so far).
    >>>
    >>> I worked up a patch that kills ata_sg_is_last() (plus the
    >>> max_phys_segments sata_mv fix), see attached. I'm thinking this is what
    >>> I like to see in upstream.

    >> Great!
    >>> Of course, this doesn't explain why ata_sg_is_last() was broken, but
    >>> since it's working _and_ slightly more efficient, I don't really care

    >> Tomo and I agreed to kill sg_last() a few days ago anyways, so this is
    >> perfectly fine with me.

    >
    > Yep, the [attached] patch that kills ata_sg_is_last() is working here on
    > both machines that were previously croaking.
    >
    > It would be nice to get pdc_adma, sata_sil24 and ipr it-works test done,
    > but IMO the patch is pretty straightforward and should be OK.


    Tested-by: Olof Johansson

    Looks ok on SATA_SIL24 and SATA_MV here on PPC (together with Jens' latest
    patch). Both barfed before.


    Thanks!

    -Olof
    -
    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] ata subsystem related crash with latest -git

    On Thu, Oct 18, 2007 at 04:38:36PM +0200, Jens Axboe wrote:
    > On Thu, Oct 18 2007, Benny Halevy wrote:
    > > On Oct. 18, 2007, 16:05 +0200, Jens Axboe wrote:
    > > > On Thu, Oct 18 2007, Jens Axboe wrote:
    > > >> On Thu, Oct 18 2007, Benny Halevy wrote:
    > > >>>> return sg;
    > > >>>> }
    > > >>>> @@ -83,6 +96,9 @@ static inline struct scatterlist *sg_last(struct scatterlist *sgl,
    > > >>>> ret = sg;
    > > >>>>
    > > >>>> #endif
    > > >>>> +#ifdef CONFIG_DEBUG_SG
    > > >>>> + BUG_ON(sgl[0].sg_magic != SG_MAGIC);
    > > >>> can it also do BUG_ON(!sg_is_last(sg))?
    > > >> That would make sense, definitely. I'll add that.
    > > >
    > > > BUG_ON(!sg_is_last(ret));
    > > >
    > > > it should be, not sg. That's what I merged.
    > > >

    > > right. of course.

    >
    > OK, that found something interesting - mapping a request may shrink it,
    > so we need to update the end marker to move it earlier in the list.
    > Basically just a
    >
    > if (sg)
    > __sg_mark_end(sg);
    >
    > at the bottom of blk_rq_map_sg(). Updated patch below, booted on other
    > machines now as well without incident.


    PPC needs this. I'm guessing other arches might too. Otherwise seems to boot
    and work well (with Jeff's sata patch).


    Thanks,

    -Olof

    diff --git a/include/asm-powerpc/scatterlist.h b/include/asm-powerpc/scatterlist.h
    index b9f1dbc..fcf7d55 100644
    --- a/include/asm-powerpc/scatterlist.h
    +++ b/include/asm-powerpc/scatterlist.h
    @@ -14,6 +14,9 @@
    #include

    struct scatterlist {
    +#ifdef CONFIG_DEBUG_SG
    + unsigned long sg_magic;
    +#endif
    unsigned long page_link;
    unsigned int offset;
    unsigned int length;
    -
    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

    On Thu, Oct 18 2007, Olof Johansson wrote:
    > On Thu, Oct 18, 2007 at 04:38:36PM +0200, Jens Axboe wrote:
    > > On Thu, Oct 18 2007, Benny Halevy wrote:
    > > > On Oct. 18, 2007, 16:05 +0200, Jens Axboe wrote:
    > > > > On Thu, Oct 18 2007, Jens Axboe wrote:
    > > > >> On Thu, Oct 18 2007, Benny Halevy wrote:
    > > > >>>> return sg;
    > > > >>>> }
    > > > >>>> @@ -83,6 +96,9 @@ static inline struct scatterlist *sg_last(struct scatterlist *sgl,
    > > > >>>> ret = sg;
    > > > >>>>
    > > > >>>> #endif
    > > > >>>> +#ifdef CONFIG_DEBUG_SG
    > > > >>>> + BUG_ON(sgl[0].sg_magic != SG_MAGIC);
    > > > >>> can it also do BUG_ON(!sg_is_last(sg))?
    > > > >> That would make sense, definitely. I'll add that.
    > > > >
    > > > > BUG_ON(!sg_is_last(ret));
    > > > >
    > > > > it should be, not sg. That's what I merged.
    > > > >
    > > > right. of course.

    > >
    > > OK, that found something interesting - mapping a request may shrink it,
    > > so we need to update the end marker to move it earlier in the list.
    > > Basically just a
    > >
    > > if (sg)
    > > __sg_mark_end(sg);
    > >
    > > at the bottom of blk_rq_map_sg(). Updated patch below, booted on other
    > > machines now as well without incident.

    >
    > PPC needs this. I'm guessing other arches might too. Otherwise seems to boot
    > and work well (with Jeff's sata patch).


    Oh duh, indeed I didn't add that to the archs when converting. Thanks
    for the patch, I'll update the rest as well.

    And thanks a lot for boot testing it!

    --
    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/

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



    On Thu, 18 Oct 2007, Jens Axboe wrote:
    > - unsigned long addr = page_to_phys(s->page) + s->offset;
    > + unsigned long addr = page_to_phys(sg_page(s)) + s->offset;


    Umm. May I suggest (I haven't read the whole thread yet, maybe somebody
    else already did) that

    static inline unsigned long sg_phys(struct scatterlist *sg)
    {
    return page_to_phys(sg_page(sg)) + sg->offset;
    }

    would be a good thing to have?

    Very few drivers should care so much about the *page* itself (or the
    offset). That's something that the generic allocation code etc cares
    about, but the driver is almost bound to care mostly about the actual DMA
    address, so adding that helper function that abstracts the sg access would
    be helpful in hiding some of the cruft?

    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] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Linus Torvalds wrote:
    >
    >
    > On Thu, 18 Oct 2007, Jens Axboe wrote:
    > > - unsigned long addr = page_to_phys(s->page) + s->offset;
    > > + unsigned long addr = page_to_phys(sg_page(s)) + s->offset;

    >
    > Umm. May I suggest (I haven't read the whole thread yet, maybe somebody
    > else already did) that
    >
    > static inline unsigned long sg_phys(struct scatterlist *sg)
    > {
    > return page_to_phys(sg_page(sg)) + sg->offset;
    > }
    >
    > would be a good thing to have?


    Sure thing, it's used quite a lot.

    > Very few drivers should care so much about the *page* itself (or the
    > offset). That's something that the generic allocation code etc cares
    > about, but the driver is almost bound to care mostly about the actual DMA
    > address, so adding that helper function that abstracts the sg access would
    > be helpful in hiding some of the cruft?


    I'll add it to the mix.

    --
    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/

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

    On Thu, Oct 18 2007, Jens Axboe wrote:
    > On Thu, Oct 18 2007, Linus Torvalds wrote:
    > >
    > >
    > > On Thu, 18 Oct 2007, Jens Axboe wrote:
    > > > - unsigned long addr = page_to_phys(s->page) + s->offset;
    > > > + unsigned long addr = page_to_phys(sg_page(s)) + s->offset;

    > >
    > > Umm. May I suggest (I haven't read the whole thread yet, maybe somebody
    > > else already did) that
    > >
    > > static inline unsigned long sg_phys(struct scatterlist *sg)
    > > {
    > > return page_to_phys(sg_page(sg)) + sg->offset;
    > > }
    > >
    > > would be a good thing to have?

    >
    > Sure thing, it's used quite a lot.


    Actually, only 11 occurrences in the patch. But still a nice little
    cleanup.

    --
    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/

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

    On Thu, 18 Oct 2007 09:55:04 -0700 (PDT)
    Linus Torvalds wrote:

    >
    >
    > On Thu, 18 Oct 2007, Jens Axboe wrote:
    > > - unsigned long addr = page_to_phys(s->page) +
    > > s->offset;
    > > + unsigned long addr = page_to_phys(sg_page(s)) +
    > > s->offset;

    >
    > Umm. May I suggest (I haven't read the whole thread yet, maybe
    > somebody else already did) that
    >
    > static inline unsigned long sg_phys(struct scatterlist *sg)
    > {
    > return page_to_phys(sg_page(sg)) + sg->offset;
    > }
    >
    > would be a good thing to have?
    >
    > Very few drivers should care so much about the *page* itself (or the
    > offset). That's something that the generic allocation code etc cares
    > about, but the driver is almost bound to care mostly about the actual
    > DMA address


    ..... but will that work for systems with IOMMU ? or is it fundamentally
    the wrong interface
    -
    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

    On Thu, Oct 18 2007, Arjan van de Ven wrote:
    > On Thu, 18 Oct 2007 09:55:04 -0700 (PDT)
    > Linus Torvalds wrote:
    >
    > >
    > >
    > > On Thu, 18 Oct 2007, Jens Axboe wrote:
    > > > - unsigned long addr = page_to_phys(s->page) +
    > > > s->offset;
    > > > + unsigned long addr = page_to_phys(sg_page(s)) +
    > > > s->offset;

    > >
    > > Umm. May I suggest (I haven't read the whole thread yet, maybe
    > > somebody else already did) that
    > >
    > > static inline unsigned long sg_phys(struct scatterlist *sg)
    > > {
    > > return page_to_phys(sg_page(sg)) + sg->offset;
    > > }
    > >
    > > would be a good thing to have?
    > >
    > > Very few drivers should care so much about the *page* itself (or the
    > > offset). That's something that the generic allocation code etc cares
    > > about, but the driver is almost bound to care mostly about the actual
    > > DMA address

    >
    > .... but will that work for systems with IOMMU ? or is it fundamentally
    > the wrong interface


    They use foo_to_bus() on the address. sg_phys() should of course only be
    used where the user previously did page_to_phys() on the sg page.

    --
    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/

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

    Linus Torvalds wrote:
    > Very few drivers should care so much about the *page* itself (or the
    > offset). That's something that the generic allocation code etc cares
    > about, but the driver is almost bound to care mostly about the actual DMA
    > address, so adding that helper function that abstracts the sg access would
    > be helpful in hiding some of the cruft?



    FWIW libata cares about both. When doing DMA, it cares about the DMA
    address. When doing PIO, it cares about the actual page, because it
    does a kmap before sending the data through a 16-bit/32-bit data FIFO.

    Jeff


    -
    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] ata subsystem related crash with latest -git

    On Thu, 18 Oct 2007 19:14:29 +0200
    Jens Axboe wrote:

    > On Thu, Oct 18 2007, Arjan van de Ven wrote:
    > > On Thu, 18 Oct 2007 09:55:04 -0700 (PDT)
    > > Linus Torvalds wrote:
    > >
    > > >
    > > >
    > > > On Thu, 18 Oct 2007, Jens Axboe wrote:
    > > > > - unsigned long addr = page_to_phys(s->page) +
    > > > > s->offset;
    > > > > + unsigned long addr = page_to_phys(sg_page(s)) +
    > > > > s->offset;
    > > >
    > > > Umm. May I suggest (I haven't read the whole thread yet, maybe
    > > > somebody else already did) that
    > > >
    > > > static inline unsigned long sg_phys(struct scatterlist *sg)
    > > > {
    > > > return page_to_phys(sg_page(sg)) + sg->offset;
    > > > }
    > > >
    > > > would be a good thing to have?
    > > >
    > > > Very few drivers should care so much about the *page* itself (or the
    > > > offset). That's something that the generic allocation code etc cares
    > > > about, but the driver is almost bound to care mostly about the actual
    > > > DMA address

    > >
    > > .... but will that work for systems with IOMMU ? or is it fundamentally
    > > the wrong interface

    >
    > They use foo_to_bus() on the address. sg_phys() should of course only be
    > used where the user previously did page_to_phys() on the sg page.


    I can take care of IOMMU stuff when I'll send IOMMU merging fix
    patchset:

    http://marc.info/?l=linux-scsi&m=119079718126157&w=2
    -
    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] ata subsystem related crash with latest -git

    [Just catching with reading lkml to this post]

    On 10/18/07, Jens Axboe wrote:
    >
    > Theory - ata_sg_is_last() isn't returning true for the last entry. Can
    > you double check that it correcly marks the last entry in mv_fill_sg()?
    > Alternatively, just try this patch.


    I "hate" to point this out, but I already reported that sata_sil24
    fails on 1. Sep.:
    http://lkml.org/lkml/2007/9/1/95

    In the thread "sata_sil24 broken since 2.6.23-rc4-mm1" I spent over a
    week to trace this back to ata_sg_is_last (finally found on 7. Oct):
    http://lkml.org/lkml/2007/10/7/43

    Thanks for finally patching this now...

    Torsten
    -
    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 8 of 8 FirstFirst ... 6 7 8