Jens,
As you know, for the last week or so I have been battling some
performance issues in XFS and 2.4.20-rc1. Well, we finally found
the culprit back in 2.4.20-pre2.
When the block highmem patch was included, it added highmem_io to the
scsi controller structure. This can only ever be set to one if
CONFIG_HIGHIO is set. Yet there are several spots in the scsi
code which test based on its value regardless.
/*
* we really want to use sg even for a single segment request,
* however some people just cannot be bothered to write decent
* driver code so we can't risk to break somebody making the
* assumption that sg requests will always contain at least 2
* segments. if the driver is 32-bit dma safe, then use sg for
* 1 entry anyways. if not, don't rely on the driver handling this
* case.
*/
if (count == 1 && !SCpnt->host->highmem_io) {
this_count = req->current_nr_sectors;
goto single_segment;
}
Running with 128M of memory I usually do not turn highmem on. Well
finally I did and my performance went from this:
Version 1.02a ------Sequential Create------ --------Random Create--------
burst.americas.sgi. -Create-- --Read--- -Delete-- -Create-- --Read--- -Delete--
files /sec %CP /sec %CP /sec %CP /sec %CP /sec %CP /sec %CP
2 156 4 +++++ +++ 118 3 149 3 +++++ +++ 108 2
which is the worst I ever saw, to this:
Version 1.02a ------Sequential Create------ --------Random Create--------
burst.americas.sgi. -Create-- --Read--- -Delete-- -Create-- --Read--- -Delete--
files /sec %CP /sec %CP /sec %CP /sec %CP /sec %CP /sec %CP
2 2732 81 +++++ +++ 1771 38 2555 86 +++++ +++ 1418 30
Jens, can you do something about this please?
Steve
p.s. You now owe me a week's consulting some time ;-)
--
Steve Lord voice: +1-651-683-3511
Principal Engineer, Filesystem Software email: [email protected]
On Fri, Nov 15 2002, Steve Lord wrote:
> Jens,
>
> As you know, for the last week or so I have been battling some
> performance issues in XFS and 2.4.20-rc1. Well, we finally found
> the culprit back in 2.4.20-pre2.
>
> When the block highmem patch was included, it added highmem_io to the
> scsi controller structure. This can only ever be set to one if
> CONFIG_HIGHIO is set. Yet there are several spots in the scsi
> code which test based on its value regardless.
Doh! You are right, this is a very stupid bug, thanks for catching it.
I'm on the road this weekend, I'' submit a patch to fix it as soon
as I get back.
> Jens, can you do something about this please?
Of course
> p.s. You now owe me a week's consulting some time ;-)
Indeed, sorry about that...
Jens
On Fri, Nov 15 2002, Steve Lord wrote:
> Jens,
>
> As you know, for the last week or so I have been battling some
> performance issues in XFS and 2.4.20-rc1. Well, we finally found
> the culprit back in 2.4.20-pre2.
>
> When the block highmem patch was included, it added highmem_io to the
> scsi controller structure. This can only ever be set to one if
> CONFIG_HIGHIO is set. Yet there are several spots in the scsi
> code which test based on its value regardless.
>
> /*
> * we really want to use sg even for a single segment request,
> * however some people just cannot be bothered to write decent
> * driver code so we can't risk to break somebody making the
> * assumption that sg requests will always contain at least 2
> * segments. if the driver is 32-bit dma safe, then use sg for
> * 1 entry anyways. if not, don't rely on the driver handling this
> * case.
> */
> if (count == 1 && !SCpnt->host->highmem_io) {
> this_count = req->current_nr_sectors;
> goto single_segment;
> }
Steve,
Something isn't quite making sense. If we go over every single instance
of checking ->highmem_io, they all look sane (ie checking on non-highmem
setup must yield 0). So that part looks good.
However, I think a typo snuck in there, in exactly the spot you pasted
above. Could you try 2.4.20-rc2 with this patch applied?
===== drivers/scsi/scsi_merge.c 1.9 vs edited =====
--- 1.9/drivers/scsi/scsi_merge.c Mon Sep 16 09:25:10 2002
+++ edited/drivers/scsi/scsi_merge.c Mon Nov 18 13:04:41 2002
@@ -835,7 +835,7 @@
* case.
*/
if (count == 1 && !SCpnt->host->highmem_io) {
- this_count = req->current_nr_sectors;
+ this_count = req->nr_sectors;
goto single_segment;
}
--
Jens Axboe
On Mon, Nov 18 2002, Stephen Lord wrote:
> On Mon, 2002-11-18 at 06:05, Jens Axboe wrote:
>
> >
> > Steve,
> >
> > Something isn't quite making sense. If we go over every single instance
> > of checking ->highmem_io, they all look sane (ie checking on non-highmem
> > setup must yield 0). So that part looks good.
> >
> > However, I think a typo snuck in there, in exactly the spot you pasted
> > above. Could you try 2.4.20-rc2 with this patch applied?
> >
> > ===== drivers/scsi/scsi_merge.c 1.9 vs edited =====
> > --- 1.9/drivers/scsi/scsi_merge.c Mon Sep 16 09:25:10 2002
> > +++ edited/drivers/scsi/scsi_merge.c Mon Nov 18 13:04:41 2002
> > @@ -835,7 +835,7 @@
> > * case.
> > */
> > if (count == 1 && !SCpnt->host->highmem_io) {
> > - this_count = req->current_nr_sectors;
> > + this_count = req->nr_sectors;
> > goto single_segment;
> > }
>
> That looks like it does it, performance is pretty much what I was
> getting with HIGHIO on.
Great, thanks for verifying that. I'll push it to Marcelo ASAP.
--
Jens Axboe
On Mon, 2002-11-18 at 06:05, Jens Axboe wrote:
>
> Steve,
>
> Something isn't quite making sense. If we go over every single instance
> of checking ->highmem_io, they all look sane (ie checking on non-highmem
> setup must yield 0). So that part looks good.
>
> However, I think a typo snuck in there, in exactly the spot you pasted
> above. Could you try 2.4.20-rc2 with this patch applied?
>
> ===== drivers/scsi/scsi_merge.c 1.9 vs edited =====
> --- 1.9/drivers/scsi/scsi_merge.c Mon Sep 16 09:25:10 2002
> +++ edited/drivers/scsi/scsi_merge.c Mon Nov 18 13:04:41 2002
> @@ -835,7 +835,7 @@
> * case.
> */
> if (count == 1 && !SCpnt->host->highmem_io) {
> - this_count = req->current_nr_sectors;
> + this_count = req->nr_sectors;
> goto single_segment;
> }
That looks like it does it, performance is pretty much what I was
getting with HIGHIO on.
Thanks
Steve
--
Stephen Lord <[email protected]>