SCSI io_request_lock patch

SCSI io_request_lock patch

[Jonathan Lahr]

This is a request for comments on the patch described below which 
implements a revised approach to reducing io_request_lock contention 
in 2.4.

This new version of the io_request_lock patch (siorl-v0) is available
at http://sourceforge.net/projects/lse/.  It employs the same
concurrent request queueing scheme as the iorlv0 patch but isolates 
code changes to the SCSI subsystem and engages the new locking scheme 
only for SCSI drivers which explicitly request it.  I took this more 
restricted approach after additional development based on comments from 
Jens and others indicated that iorlv0 impacted the IDE subsystem and
was unnecessarily broad in general.

The siorl-v0 patch allows drivers to enable concurrent queueing through 
the concurrent_queue field in the Scsi_Host_Template which is copied to 
the request queue.  It creates SCSI-specific versions of generic block 
i/o functions used by the SCSI subsystem and modifies them to conditionally 
engage the new locking scheme based on this field.  It allows control over 
which drivers use concurrent queueing and preserves original block i/o 
behavior by default.

I tested this patch with aic7xxx and lpfc drivers, and regression tested 
it with IDE disk and CDROM drivers.  Any feedback would be appreciated.

Thanks,
Jonathan

-- 
Jonathan Lahr
IBM Linux Technology Center
Beaverton, Oregon
lahr@us.ibm.com
503-578-3385

Re: [Lse-tech] SCSI io_request_lock patch

[Jens Axboe]

On Mon, Nov 12 2001, Jonathan Lahr wrote:
> 
> This is a request for comments on the patch described below which 
> implements a revised approach to reducing io_request_lock contention 
> in 2.4.
> 
> This new version of the io_request_lock patch (siorl-v0) is available
> at http://sourceforge.net/projects/lse/.  It employs the same
> concurrent request queueing scheme as the iorlv0 patch but isolates 
> code changes to the SCSI subsystem and engages the new locking scheme 
> only for SCSI drivers which explicitly request it.  I took this more 
> restricted approach after additional development based on comments from 
> Jens and others indicated that iorlv0 impacted the IDE subsystem and
> was unnecessarily broad in general.
> 
> The siorl-v0 patch allows drivers to enable concurrent queueing through 
> the concurrent_queue field in the Scsi_Host_Template which is copied to 
> the request queue.  It creates SCSI-specific versions of generic block 
> i/o functions used by the SCSI subsystem and modifies them to conditionally 
> engage the new locking scheme based on this field.  It allows control over 
> which drivers use concurrent queueing and preserves original block i/o 
> behavior by default.

Sorry Jonathan, but this is even more broken than the last patch. In
different ways. In no particular order:

o You are duplicating way too much code and exporting block internals
o You are breaking SCSI merge completely, why on earth are you suddenly
  using ll_*_merge functions for SCSI?!
o scsi_make_request need not worry about head active
o scsi_make_request can safe the q->*_merge indirect
o scsi_dispatch_cmd() io_request_lock removal looks racy

At least you are not breaking anything other than SCSI this time...

-- 
Jens Axboe

Re: [Lse-tech] SCSI io_request_lock patch

[Jonathan Lahr]

Jens Axboe [axboe@suse.de] wrote:
> On Mon, Nov 12 2001, Jonathan Lahr wrote:
> > 
> > This is a request for comments on the patch described below which 
> > implements a revised approach to reducing io_request_lock contention 
> > in 2.4.
> > 
> > This new version of the io_request_lock patch (siorl-v0) is available
> > at http://sourceforge.net/projects/lse/.  It employs the same
> > concurrent request queueing scheme as the iorlv0 patch but isolates 
> > code changes to the SCSI subsystem and engages the new locking scheme 
> > only for SCSI drivers which explicitly request it.  I took this more 
> > restricted approach after additional development based on comments from 
> > Jens and others indicated that iorlv0 impacted the IDE subsystem and
> > was unnecessarily broad in general.
> > 
> > The siorl-v0 patch allows drivers to enable concurrent queueing through 
> > the concurrent_queue field in the Scsi_Host_Template which is copied to 
> > the request queue.  It creates SCSI-specific versions of generic block 
> > i/o functions used by the SCSI subsystem and modifies them to conditionally 
> > engage the new locking scheme based on this field.  It allows control over 
> > which drivers use concurrent queueing and preserves original block i/o 
> > behavior by default.
> 
> Sorry Jonathan, but this is even more broken than the last patch. In
> different ways. In no particular order:
> 
> o You are duplicating way too much code and exporting block internals

The duplication is a reasonable starting point for SCSI-specific functions.
The block i/o design provides for exactly this type of tailoring through
function pointers installed in request_queue.

What problem you do see with exporting block internals?

> o You are breaking SCSI merge completely, why on earth are you suddenly
>   using ll_*_merge functions for SCSI?!
> o scsi_make_request need not worry about head active
> o scsi_make_request can safe the q->*_merge indirect
> o scsi_dispatch_cmd() io_request_lock removal looks racy

I will investigate the above comments further.

> At least you are not breaking anything other than SCSI this time...

Do you think the separation of SCSI from generic block i/o code and the
driver-activated control of concurrent queueing provides a path for future 
work to reduce io_request_lock contention in SCSI/FC?

-- 
Jonathan Lahr
IBM Linux Technology Center
Beaverton, Oregon
lahr@us.ibm.com
503-578-3385

Re: [Lse-tech] SCSI io_request_lock patch

[Jens Axboe]

On Tue, Nov 13 2001, Jonathan Lahr wrote:
> Jens Axboe [axboe@suse.de] wrote:
> > On Mon, Nov 12 2001, Jonathan Lahr wrote:
> > > 
> > > This is a request for comments on the patch described below which
> > > implements a revised approach to reducing io_request_lock
> > > contention in 2.4.
> > > 
> > > This new version of the io_request_lock patch (siorl-v0) is
> > > available at http://sourceforge.net/projects/lse/.  It employs the
> > > same concurrent request queueing scheme as the iorlv0 patch but
> > > isolates code changes to the SCSI subsystem and engages the new
> > > locking scheme only for SCSI drivers which explicitly request it.
> > > I took this more restricted approach after additional development
> > > based on comments from Jens and others indicated that iorlv0
> > > impacted the IDE subsystem and was unnecessarily broad in general.
> > > 
> > > The siorl-v0 patch allows drivers to enable concurrent queueing
> > > through the concurrent_queue field in the Scsi_Host_Template which
> > > is copied to the request queue.  It creates SCSI-specific versions
> > > of generic block i/o functions used by the SCSI subsystem and
> > > modifies them to conditionally engage the new locking scheme based
> > > on this field.  It allows control over which drivers use
> > > concurrent queueing and preserves original block i/o behavior by
> > > default.
> > 
> > Sorry Jonathan, but this is even more broken than the last patch. In
> > different ways. In no particular order:
> > 
> > o You are duplicating way too much code and exporting block
> > internals
> 
> The duplication is a reasonable starting point for SCSI-specific
> functions.  The block i/o design provides for exactly this type of
> tailoring through function pointers installed in request_queue.

Yes I know, I wrote most of said code :-)

> What problem you do see with exporting block internals?

It's absolutely worthless. Look, it ties in with the points I made
below. You are exporting the merge functions for instance, and setting
them in the queue. This will cause scsi_merge not to use it's own
functions, broken.

The make_request_fn addition could be ok, just needs to be cleaned a
bit.

> > o You are breaking SCSI merge completely, why on earth are you
> > suddenly using ll_*_merge functions for SCSI?!  o scsi_make_request
> > need not worry about head active o scsi_make_request can safe the
> > q->*_merge indirect o scsi_dispatch_cmd() io_request_lock removal
> > looks racy
> 
> I will investigate the above comments further.
> 
> > At least you are not breaking anything other than SCSI this time...
> 
> Do you think the separation of SCSI from generic block i/o code and
> the driver-activated control of concurrent queueing provides a path
> for future work to reduce io_request_lock contention in SCSI/FC?

Not really, but I do think it could be a viable 2.4 alternative. For 2.5
we still want to do this the right way.

-- 
Jens Axboe

Re: [Lse-tech] SCSI io_request_lock patch

[Jonathan Lahr]

Jens Axboe [axboe@suse.de] wrote:
> On Tue, Nov 13 2001, Jonathan Lahr wrote:
> > Jens Axboe [axboe@suse.de] wrote:
> > > On Mon, Nov 12 2001, Jonathan Lahr wrote:
> > > > 
> > > > This is a request for comments on the patch described below which
> > > > implements a revised approach to reducing io_request_lock
> > > > contention in 2.4.
> > > > 
> > > > This new version of the io_request_lock patch (siorl-v0) is
> > > > available at http://sourceforge.net/projects/lse/.  It employs the
> > > > same concurrent request queueing scheme as the iorlv0 patch but
> > > > isolates code changes to the SCSI subsystem and engages the new
> > > > locking scheme only for SCSI drivers which explicitly request it.
> > > > I took this more restricted approach after additional development
> > > > based on comments from Jens and others indicated that iorlv0
> > > > impacted the IDE subsystem and was unnecessarily broad in general.
> > > > 
> > > > The siorl-v0 patch allows drivers to enable concurrent queueing
> > > > through the concurrent_queue field in the Scsi_Host_Template which
> > > > is copied to the request queue.  It creates SCSI-specific versions
> > > > of generic block i/o functions used by the SCSI subsystem and
> > > > modifies them to conditionally engage the new locking scheme based
> > > > on this field.  It allows control over which drivers use
> > > > concurrent queueing and preserves original block i/o behavior by
> > > > default.
> > > 
> > > Sorry Jonathan, but this is even more broken than the last patch. In
> > > different ways. In no particular order:
> > > 
> > > o You are duplicating way too much code and exporting block
> > > internals
> > 
> > The duplication is a reasonable starting point for SCSI-specific
> > functions.  The block i/o design provides for exactly this type of
> > tailoring through function pointers installed in request_queue.
> 
> Yes I know, I wrote most of said code :-)

And this approach makes good use of it.

> > What problem you do see with exporting block internals?
> 
> It's absolutely worthless. Look, it ties in with the points I made
> below. You are exporting the merge functions for instance, and setting
> them in the queue. This will cause scsi_merge not to use it's own
> functions, broken.

As in the baseline, initialize_merge_fn overwrites these pointers:
     q->back_merge_fn = scsi_back_merge_fn_;
     q->front_merge_fn = scsi_front_merge_fn_;
     q->merge_requests_fn = scsi_merge_requests_fn_;

> > Do you think the separation of SCSI from generic block i/o code and
> > the driver-activated control of concurrent queueing provides a path
> > for future work to reduce io_request_lock contention in SCSI/FC?
> 
> Not really, but I do think it could be a viable 2.4 alternative. For 2.5
> we still want to do this the right way.

I'll try to stay apprised of the 2.5 work as it progresses.

-- 
Jonathan Lahr
IBM Linux Technology Center
Beaverton, Oregon
lahr@us.ibm.com
503-578-3385

Re: [Lse-tech] SCSI io_request_lock patch

[Jens Axboe]

On Wed, Nov 14 2001, Jonathan Lahr wrote:
> > It's absolutely worthless. Look, it ties in with the points I made
> > below. You are exporting the merge functions for instance, and setting
> > them in the queue. This will cause scsi_merge not to use it's own
> > functions, broken.
> 
> As in the baseline, initialize_merge_fn overwrites these pointers:
>      q->back_merge_fn = scsi_back_merge_fn_;
>      q->front_merge_fn = scsi_front_merge_fn_;
>      q->merge_requests_fn = scsi_merge_requests_fn_;

I had forgotten I had #if 0 out the check for already set back_merge etc
in scsi_merge -- however that's still beside the point. _Why_ are you
exporting the ll_rw_blk functions and setting them just to have them
overridden? Makes no sense.

Don't export the merge functions ever, define your own if you really
need them. You don't, though.

As I've mentioned before, go ahead with the make_request_fn replacement.
That is indeed what it is there for.

-- 
Jens Axboe

Re: [Lse-tech] SCSI io_request_lock patch

[Jonathan Lahr]

Jens Axboe [axboe@suse.de] wrote:
> On Wed, Nov 14 2001, Jonathan Lahr wrote:
> > > It's absolutely worthless. Look, it ties in with the points I made
> > > below. You are exporting the merge functions for instance, and setting
> > > them in the queue. This will cause scsi_merge not to use it's own
> > > functions, broken.
> > 
> > As in the baseline, initialize_merge_fn overwrites these pointers:
> >      q->back_merge_fn = scsi_back_merge_fn_;
> >      q->front_merge_fn = scsi_front_merge_fn_;
> >      q->merge_requests_fn = scsi_merge_requests_fn_;
> 
> I had forgotten I had #if 0 out the check for already set back_merge etc
> in scsi_merge -- however that's still beside the point. _Why_ are you
> exporting the ll_rw_blk functions and setting them just to have them
> overridden? Makes no sense.
...
> Don't export the merge functions ever, define your own if you really
> need them. You don't, though.

That resulted merely from basing scsi_init_queue on blk_init_queue.
I'll simplify the code by removing these unnecessary assignments.

-- 
Jonathan Lahr
IBM Linux Technology Center
Beaverton, Oregon
lahr@us.ibm.com
503-578-3385