[bug] block subsystem related crash with latest -git

On Thu, Oct 18, 2007 at 06:42:46AM -0400, Jeff Garzik wrote:[color=blue]
> Jens Axboe wrote:[color=green]
>> On Thu, Oct 18 2007, Jeff Garzik wrote:[color=darkred]
>>> 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.[/color]
>> Great![color=darkred]
>>> 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 :)[/color]
>> Tomo and I agreed to kill sg_last() a few days ago anyways, so this is
>> perfectly fine with me.[/color]
>
> 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.[/color]

Tested-by: Olof Johansson <olof@lixom.net>

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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Thu, Oct 18, 2007 at 04:38:36PM +0200, Jens Axboe wrote:[color=blue]
> On Thu, Oct 18 2007, Benny Halevy wrote:[color=green]
> > On Oct. 18, 2007, 16:05 +0200, Jens Axboe <jens.axboe@oracle.com> wrote:[color=darkred]
> > > 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.
> > >[/color]
> > right. of course.[/color]
>
> 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.[/color]

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 <asm/dma.h>

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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Thu, Oct 18 2007, Olof Johansson wrote:[color=blue]
> On Thu, Oct 18, 2007 at 04:38:36PM +0200, Jens Axboe wrote:[color=green]
> > On Thu, Oct 18 2007, Benny Halevy wrote:[color=darkred]
> > > On Oct. 18, 2007, 16:05 +0200, Jens Axboe <jens.axboe@oracle.com> 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.[/color]
> >
> > 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.[/color]
>
> PPC needs this. I'm guessing other arches might too. Otherwise seems to boot
> and work well (with Jeff's sata patch).[/color]

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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

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

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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Thu, Oct 18 2007, Linus Torvalds wrote:[color=blue]
>
>
> On Thu, 18 Oct 2007, Jens Axboe wrote:[color=green]
> > - unsigned long addr = page_to_phys(s->page) + s->offset;
> > + unsigned long addr = page_to_phys(sg_page(s)) + s->offset;[/color]
>
> 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?[/color]

Sure thing, it's used quite a lot.
[color=blue]
> 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?[/color]

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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Thu, Oct 18 2007, Jens Axboe wrote:[color=blue]
> On Thu, Oct 18 2007, Linus Torvalds wrote:[color=green]
> >
> >
> > On Thu, 18 Oct 2007, Jens Axboe wrote:[color=darkred]
> > > - unsigned long addr = page_to_phys(s->page) + s->offset;
> > > + unsigned long addr = page_to_phys(sg_page(s)) + s->offset;[/color]
> >
> > 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?[/color]
>
> Sure thing, it's used quite a lot.[/color]

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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Thu, 18 Oct 2007 09:55:04 -0700 (PDT)
Linus Torvalds <torvalds@linux-foundation.org> wrote:
[color=blue]
>
>
> On Thu, 18 Oct 2007, Jens Axboe wrote:[color=green]
> > - unsigned long addr = page_to_phys(s->page) +
> > s->offset;
> > + unsigned long addr = page_to_phys(sg_page(s)) +
> > s->offset;[/color]
>
> 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[/color]

..... 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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Thu, Oct 18 2007, Arjan van de Ven wrote:[color=blue]
> On Thu, 18 Oct 2007 09:55:04 -0700 (PDT)
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>[color=green]
> >
> >
> > On Thu, 18 Oct 2007, Jens Axboe wrote:[color=darkred]
> > > - unsigned long addr = page_to_phys(s->page) +
> > > s->offset;
> > > + unsigned long addr = page_to_phys(sg_page(s)) +
> > > s->offset;[/color]
> >
> > 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[/color]
>
> .... but will that work for systems with IOMMU ? or is it fundamentally
> the wrong interface[/color]

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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

Linus Torvalds wrote:[color=blue]
> 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?[/color]


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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Thu, 18 Oct 2007 19:14:29 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:
[color=blue]
> On Thu, Oct 18 2007, Arjan van de Ven wrote:[color=green]
> > On Thu, 18 Oct 2007 09:55:04 -0700 (PDT)
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >[color=darkred]
> > >
> > >
> > > 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[/color]
> >
> > .... but will that work for systems with IOMMU ? or is it fundamentally
> > the wrong interface[/color]
>
> 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.[/color]

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

[url]http://marc.info/?l=linux-scsi&m=119079718126157&w=2[/url]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

[Just catching with reading lkml to this post]

On 10/18/07, Jens Axboe <jens.axboe@oracle.com> wrote:[color=blue]
>
> 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.[/color]

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

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):
[url]http://lkml.org/lkml/2007/10/7/43[/url]

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 [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]