Possible explanation for SCSI benchmark problems in 2.5

Possible explanation for SCSI benchmark problems in 2.5

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

Hi All,

While looking at other code (OK, how to get the MCA drivers not to use bounce 
buffers...)  I discovered that the way scsi_scan.c calls 
scsi_initialize_merge_fn() guarantees (on an x86) that the 
blk_queue_bounce_limit is always called with BLK_BOUNCE_HIGH.  This will 
probably hurt benchmarks on machines with 1Gb or more of memory.

The problem is that scsi_initialize_merge_fn() checks sdev->type, but this is 
always -1 in scsi_alloc_sdev, and thus it never uses the pci dma mask for disk 
devices.

The attached patch moves the initialisation to after sdev->type has the 
correct value.

James


[-- Attachment #2: bounce_limit.diff --]
[-- Type: text/plain , Size: 750 bytes --]

===== drivers/scsi/scsi_scan.c 1.21 vs edited =====
--- 1.21/drivers/scsi/scsi_scan.c	Wed Aug 14 10:06:05 2002
+++ edited/drivers/scsi/scsi_scan.c	Sun Sep 29 15:41:47 2002
@@ -528,7 +528,6 @@
 		scsi_initialize_queue(sdev, shost);
 		sdev->request_queue.queuedata = (void *) sdev;
 
-		scsi_initialize_merge_fn(sdev);
 		init_waitqueue_head(&sdev->scpnt_wait);
 
 		/*
@@ -1408,6 +1407,11 @@
 	sdev->removable = (0x80 & inq_result[1]) >> 7;
 	sdev->lockable = sdev->removable;
 	sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
+
+	/* initialise merge function needs some of the above data to 
+	 * work correctly */
+	scsi_initialize_merge_fn(sdev);
+
 
 	/*
 	 * XXX maybe move the identifier and driverfs/devfs setup to a new

Re: Possible explanation for SCSI benchmark problems in 2.5

[Andrew Morton]

James Bottomley wrote:
> 
> Hi All,
> 
> While looking at other code (OK, how to get the MCA drivers not to use bounce
> buffers...)  I discovered that the way scsi_scan.c calls
> scsi_initialize_merge_fn() guarantees (on an x86) that the
> blk_queue_bounce_limit is always called with BLK_BOUNCE_HIGH.  This will
> probably hurt benchmarks on machines with 1Gb or more of memory.
> 
> The problem is that scsi_initialize_merge_fn() checks sdev->type, but this is
> always -1 in scsi_alloc_sdev, and thus it never uses the pci dma mask for disk
> devices.
> 
> The attached patch moves the initialisation to after sdev->type has the
> correct value.
> 

argh, sorry.  I've had such a patch in my tree for ages, and
I do all testing with it in place.

But high-IO is bust for IDE as well at present, so at least
we're being fair ;)

Re: Possible explanation for SCSI benchmark problems in 2.5

[Jens Axboe]

On Sun, Sep 29 2002, Andrew Morton wrote:
> But high-IO is bust for IDE as well at present, so at least
> we're being fair ;)

Nah that's fixed, it worries me that you say otherwise. Doesn't it work
for you in stock 2.5.39?

-- 
Jens Axboe

Re: Possible explanation for SCSI benchmark problems in 2.5

[Andrew Morton]

Jens Axboe wrote:
> 
> On Sun, Sep 29 2002, Andrew Morton wrote:
> > But high-IO is bust for IDE as well at present, so at least
> > we're being fair ;)
> 
> Nah that's fixed, it worries me that you say otherwise. Doesn't it work
> for you in stock 2.5.39?
> 

Ah you're right, it went away.  I was assuming that it was still
a problem because my fixit patch wasn't throwing rejects...

Thanks.

Re: Possible explanation for SCSI benchmark problems in 2.5

[Jens Axboe]

On Mon, Sep 30 2002, Andrew Morton wrote:
> Jens Axboe wrote:
> > 
> > On Sun, Sep 29 2002, Andrew Morton wrote:
> > > But high-IO is bust for IDE as well at present, so at least
> > > we're being fair ;)
> > 
> > Nah that's fixed, it worries me that you say otherwise. Doesn't it work
> > for you in stock 2.5.39?
> > 
> 
> Ah you're right, it went away.  I was assuming that it was still
> a problem because my fixit patch wasn't throwing rejects...

I _think_ we should have that patch in addition (it seems noone calls
pci_set_dma_mask on the ide pci dev). At least it cannot hurt, so feel
free to pass it along.

-- 
Jens Axboe

Re: Possible explanation for SCSI benchmark problems in 2.5

[Mike Anderson]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> Hi All,
> 
> While looking at other code (OK, how to get the MCA drivers not to use bounce 
> buffers...)  I discovered that the way scsi_scan.c calls 
> scsi_initialize_merge_fn() guarantees (on an x86) that the 
> blk_queue_bounce_limit is always called with BLK_BOUNCE_HIGH.  This will 
> probably hurt benchmarks on machines with 1Gb or more of memory.
> 
> The problem is that scsi_initialize_merge_fn() checks sdev->type, but this is 
> always -1 in scsi_alloc_sdev, and thus it never uses the pci dma mask for disk 
> devices.
> 
> The attached patch moves the initialisation to after sdev->type has the 
> correct value.

I thought this was previously discussed on this thread.
http://marc.theaimsgroup.com/?l=linux-kernel&m=103064539622958&w=2

and I believe the comment from Jens was that this could be killed
off in 2.5 if someone did a check for safety on all the devices.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: Possible explanation for SCSI benchmark problems in 2.5

[James Bottomley]

andmike@us.ibm.com said:
> I thought this was previously discussed on this thread. http://
> marc.theaimsgroup.com/?l=linux-kernel&m=103064539622958&w=2

I remember the thread (well now that my memory's been refreshed).

> and I believe the comment from Jens was that this could be killed off
> in 2.5 if someone did a check for safety on all the devices. 

I'm not really an expert in st, which is where it looks like the problems are. 
 On cursory reading of the code, I think the st driver does dio only if all 
pages are below the HIGHMEM zone, otherwise it does intermediate buffering.  
Thus, the st driver will never put anything in the highmem zone in the queue 
and Andrew Morton's version should be fine.

Does everyone agree with this analysis?

James

Re: Possible explanation for SCSI benchmark problems in 2.5

[Kai Makisara]

On Sun, 29 Sep 2002, James Bottomley wrote:

> andmike@us.ibm.com said:
> > I thought this was previously discussed on this thread. http://
> > marc.theaimsgroup.com/?l=linux-kernel&m=103064539622958&w=2
>
> I remember the thread (well now that my memory's been refreshed).
>
> > and I believe the comment from Jens was that this could be killed off
> > in 2.5 if someone did a check for safety on all the devices.
>
> I'm not really an expert in st, which is where it looks like the problems are.
>  On cursory reading of the code, I think the st driver does dio only if all
> pages are below the HIGHMEM zone, otherwise it does intermediate buffering.
> Thus, the st driver will never put anything in the highmem zone in the queue
> and Andrew Morton's version should be fine.
>
St does direct i/o if all pages are directly addressable by the SCSI
adapter (based on the mask). The pages may be in the HIGHMEM zone but
Andrew Morton's change is safe. In fact, st already uses the logic enabled
by the change. (As far as I was able to follow the code, the SCSI requests
sent by st never actually use any code affected by Andrew's change.)

	Kai

Re: Possible explanation for SCSI benchmark problems in 2.5

[Jens Axboe]

On Sun, Sep 29 2002, Mike Anderson wrote:
> James Bottomley [James.Bottomley@steeleye.com] wrote:
> > Hi All,
> > 
> > While looking at other code (OK, how to get the MCA drivers not to use bounce 
> > buffers...)  I discovered that the way scsi_scan.c calls 
> > scsi_initialize_merge_fn() guarantees (on an x86) that the 
> > blk_queue_bounce_limit is always called with BLK_BOUNCE_HIGH.  This will 
> > probably hurt benchmarks on machines with 1Gb or more of memory.
> > 
> > The problem is that scsi_initialize_merge_fn() checks sdev->type, but this is 
> > always -1 in scsi_alloc_sdev, and thus it never uses the pci dma mask for disk 
> > devices.
> > 
> > The attached patch moves the initialisation to after sdev->type has the 
> > correct value.
> 
> I thought this was previously discussed on this thread.
> http://marc.theaimsgroup.com/?l=linux-kernel&m=103064539622958&w=2
> 
> and I believe the comment from Jens was that this could be killed
> off in 2.5 if someone did a check for safety on all the devices.

(BTW James, patch looks good)

Well that's one way to interpret it. But it really needs to be a good
audit, I refuse to remove the check if someone just assumes that
sr/st/whatnot are safe. It's simply not worth the risk.

-- 
Jens Axboe