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
[email protected]
503-578-3385
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
Jens Axboe [[email protected]] 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
[email protected]
503-578-3385
On Tue, Nov 13 2001, Jonathan Lahr wrote:
> Jens Axboe [[email protected]] 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
Jens Axboe [[email protected]] wrote:
> On Tue, Nov 13 2001, Jonathan Lahr wrote:
> > Jens Axboe [[email protected]] 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
[email protected]
503-578-3385
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
Jens Axboe [[email protected]] 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
[email protected]
503-578-3385