RE: [patch] scsi: use lock per host instead of per device for shared queue tag host

RE: [patch] scsi: use lock per host instead of per device for shared queue tag host

[Ed Lin]

> -----Original Message-----
> From: Jens Axboe [mailto:jens.axboe@oracle.com] 
> Sent: Thursday, January 25, 2007 7:48 AM
> To: Ed Lin
> Cc: David Somayajulu; Michael Reed; linux-scsi; linux-kernel; 
> james.Bottomley; jeff; Promise_Linux
> Subject: Re: [patch] scsi: use lock per host instead of per 
> device for shared queue tag host
> 
> 
> On Thu, Jan 25 2007, Jens Axboe wrote:
> > On Wed, Jan 24 2007, Ed Lin wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: David Somayajulu [mailto:david.somayajulu@qlogic.com] 
> > > > Sent: Wednesday, January 24, 2007 5:03 PM
> > > > To: Ed Lin; Michael Reed
> > > > Cc: linux-scsi; linux-kernel; james.Bottomley; jeff; 
> > > > Promise_Linux; Jens Axboe
> > > > Subject: RE: [patch] scsi: use lock per host instead of per 
> > > > device for shared queue tag host
> > > > 
> > > > 
> > > > > It seems another driver(qla4xxx) is also using shared 
> queue tag.
> > > > > It is natural to imagine there might be same symptom in that
> > > > > driver. But I don't know the driver and have no hardware so I
> > > > > can not say anything certain about it.
> > > > 
> > > > qla4xxx implements slightly differently, in the sense we 
> > > > don't have the
> > > > equivalent of         
> > > > struct st_ccb ccb[MU_MAX_REQUEST]; 
> > > > which is in struct st_hba. In other words we don't have 
> a local array
> > > > which like stex to keep track of the outstanding 
> commands to the hba.
> > > > 
> > > > We had a discussion on this one while implementing 
> block-layer tagging
> > > > in qla4xxx and Jens Axboe added the test_and_set_bit() in the 
> > > > following
> > > > code in blk_queue_start_tag() to take care of it.
> > > > 	do {
> > > > 		tag = find_first_zero_bit(bqt->tag_map, 
> bqt->max_depth);
> > > > 		if (tag >= bqt->max_depth)
> > > > 			return 1;
> > > > 	} while (test_and_set_bit(tag, bqt->tag_map));
> > > > Please see the following link for the discussion
> > > > http://marc.theaimsgroup.com/?l=linux-scsi&m=115886351206726&w=2
> > > > 
> > > > Cheers
> > > > David Somayajulu
> > > > QLogic Corporation
> > > >
> > > 
> > > Yes, this piece of code of allocating tag, in itself, is safe.
> > > But the following
> > > 
> > > 	if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> > > 		printk(KERN_ERR "%s: attempt to clear non-busy tag
> > > (%d)\n",
> > > 		       __FUNCTION__, tag);
> > > 		return;
> > > 	}
> > > 
> > > code of freeing tag (in blk_queue_end_tag())seems to be using
> > > unsafe __test_and_clear_bit instead of test_and_clear_bit.
> > > I once changed it to test_and_clear_bit and thought it was fixed.
> > > But the panic happened thereafter nonetheless(using gcc 3.4.6.
> > > gcc 4.1.0 is better but still with kernel errors). bqt also needs
> > > to be protected in this case. Replacing queue lock per device with
> > > a host lock is a simple but logical fix for it. To introduce a
> > > more refined lock is possible, but seems too tedious and elaborate
> > > for this issue, since a queue lock is already out there, and a
> > > hostwide lock is needed anyway.
> > 
> > Does this fix it? There really should be no need to add 
> extra locking
> > for this, it would be a shame.
> > 
> > diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
> > index fb67897..e752e5d 100644
> > --- a/block/ll_rw_blk.c
> > +++ b/block/ll_rw_blk.c
> > @@ -1072,12 +1072,16 @@ void 
> blk_queue_end_tag(request_queue_t *q, struct request *rq)
> >  		 */
> >  		return;
> >  
> > -	if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> > +	smp_mb__before_clear_bit();
> > +
> > +	if (unlikely(!test_and_clear_bit(tag, bqt->tag_map))) {
> >  		printk(KERN_ERR "%s: attempt to clear non-busy 
> tag (%d)\n",
> >  		       __FUNCTION__, tag);
> >  		return;
> >  	}
> >  
> > +	smp_mb__after_clear_bit();
> > +
> >  	list_del_init(&rq->queuelist);
> >  	rq->cmd_flags &= ~REQ_QUEUED;
> >  	rq->tag = -1;
> > 
> 
> Double checking the actual implementation, the smp_mb__* should not be
> needed with the test_and_*_bit operations. The __test_and_clear_bit()
> change is needed, though. What kind of crash did you see when you did
> that? It should not crash, but you could see the "attempt to clear
> non-busy tag" error though.

Besides the test_and_clear_bit, I think the bqt code(refer to last mail)
also needs protection, like:

	list_del_init(&rq->queuelist);
	...
	if (unlikely(bqt->tag_index[tag] == NULL))
		printk(KERN_ERR "%s: tag %d is missing\n",
		       __FUNCTION__, tag);

	bqt->tag_index[tag] = NULL;
	bqt->busy--;

and

	bqt->tag_index[tag] = rq;
	...
	list_add(&rq->queuelist, &bqt->busy_list);
	bqt->busy++;

because bqt is also globally shared within all devices in the host in
this case. (q->queue_tags was assigned as host->bqt in scsi_activate_tcq
)

With a gcc 4.1.0 compiled kernel, I did not get kernel panic, but
still got kernel errors: "tag  is missing". I guess a possible race
scenario could be:

cpu a:__test_and_clear_bit
cpu b:test_and_set_bit, allocate a tag just freed by cpu a
cpu b:bqt->tag_index[tag] = rq; 
cpu a:bqt->tag_index[tag] = NULL;

Next time, when the request returns,  
	if (unlikely(bqt->tag_index[tag] == NULL))
		printk(KERN_ERR "%s: tag %d is missing\n",
		       __FUNCTION__, tag);
prints out "tag  is missing".

There may possibly be some other errors. So we need a lock here.
I think the simple but reliable way to do it is just to replace
queue lock with a host lock. James pointed out that there may be
performance slow down when many devices are accessed at the
same time. But I think the major part is still on the hardware,
and a host lock is the price these kind of controllers must pay.

To your question:
but you could see the "attempt to clear non-busy tag" error though

Yes I did see it. And then I see the BUG() at blk_queue_start_tag()

	if (unlikely((rq->cmd_flags & REQ_QUEUED))) {
		printk(KERN_ERR 
		       "%s: request %p for device [%s] already tagged
%d",
		       __FUNCTION__, rq,
		       rq->rq_disk ? rq->rq_disk->disk_name : "?",
rq->tag);
		BUG();
	}

Because after "attempt to clear non-busy tag" error REQ_QUEUED flag
is not cleared.

Thanks,

Ed Lin

Re: [patch] scsi: use lock per host instead of per device for shared queue tag host

[Jeff Garzik]

Ed Lin wrote:
> There may possibly be some other errors. So we need a lock here.
> I think the simple but reliable way to do it is just to replace
> queue lock with a host lock. James pointed out that there may be
> performance slow down when many devices are accessed at the
> same time. But I think the major part is still on the hardware,
> and a host lock is the price these kind of controllers must pay.


I agree.

Further, a host lock is (a) common across many controllers, to protect 
host-wide resources and (b) only limits us when the controller is 
CPU-limited, a very rare scenario.

	Jeff

Re: [patch] scsi: use lock per host instead of per device for shared queue tag host

[Jens Axboe]

On Thu, Jan 25 2007, Jens Axboe wrote:
> On Wed, Jan 24 2007, Ed Lin wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: David Somayajulu [mailto:david.somayajulu@qlogic.com] 
> > > Sent: Wednesday, January 24, 2007 5:03 PM
> > > To: Ed Lin; Michael Reed
> > > Cc: linux-scsi; linux-kernel; james.Bottomley; jeff; 
> > > Promise_Linux; Jens Axboe
> > > Subject: RE: [patch] scsi: use lock per host instead of per 
> > > device for shared queue tag host
> > > 
> > > 
> > > > It seems another driver(qla4xxx) is also using shared queue tag.
> > > > It is natural to imagine there might be same symptom in that
> > > > driver. But I don't know the driver and have no hardware so I
> > > > can not say anything certain about it.
> > > 
> > > qla4xxx implements slightly differently, in the sense we 
> > > don't have the
> > > equivalent of         
> > > struct st_ccb ccb[MU_MAX_REQUEST]; 
> > > which is in struct st_hba. In other words we don't have a local array
> > > which like stex to keep track of the outstanding commands to the hba.
> > > 
> > > We had a discussion on this one while implementing block-layer tagging
> > > in qla4xxx and Jens Axboe added the test_and_set_bit() in the 
> > > following
> > > code in blk_queue_start_tag() to take care of it.
> > > 	do {
> > > 		tag = find_first_zero_bit(bqt->tag_map, bqt->max_depth);
> > > 		if (tag >= bqt->max_depth)
> > > 			return 1;
> > > 	} while (test_and_set_bit(tag, bqt->tag_map));
> > > Please see the following link for the discussion
> > > http://marc.theaimsgroup.com/?l=linux-scsi&m=115886351206726&w=2
> > > 
> > > Cheers
> > > David Somayajulu
> > > QLogic Corporation
> > >
> > 
> > Yes, this piece of code of allocating tag, in itself, is safe.
> > But the following
> > 
> > 	if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> > 		printk(KERN_ERR "%s: attempt to clear non-busy tag
> > (%d)\n",
> > 		       __FUNCTION__, tag);
> > 		return;
> > 	}
> > 
> > code of freeing tag (in blk_queue_end_tag())seems to be using
> > unsafe __test_and_clear_bit instead of test_and_clear_bit.
> > I once changed it to test_and_clear_bit and thought it was fixed.
> > But the panic happened thereafter nonetheless(using gcc 3.4.6.
> > gcc 4.1.0 is better but still with kernel errors). bqt also needs
> > to be protected in this case. Replacing queue lock per device with
> > a host lock is a simple but logical fix for it. To introduce a
> > more refined lock is possible, but seems too tedious and elaborate
> > for this issue, since a queue lock is already out there, and a
> > hostwide lock is needed anyway.
> 
> Does this fix it? There really should be no need to add extra locking
> for this, it would be a shame.
> 
> diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
> index fb67897..e752e5d 100644
> --- a/block/ll_rw_blk.c
> +++ b/block/ll_rw_blk.c
> @@ -1072,12 +1072,16 @@ void blk_queue_end_tag(request_queue_t *q, struct request *rq)
>  		 */
>  		return;
>  
> -	if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> +	smp_mb__before_clear_bit();
> +
> +	if (unlikely(!test_and_clear_bit(tag, bqt->tag_map))) {
>  		printk(KERN_ERR "%s: attempt to clear non-busy tag (%d)\n",
>  		       __FUNCTION__, tag);
>  		return;
>  	}
>  
> +	smp_mb__after_clear_bit();
> +
>  	list_del_init(&rq->queuelist);
>  	rq->cmd_flags &= ~REQ_QUEUED;
>  	rq->tag = -1;
> 

Double checking the actual implementation, the smp_mb__* should not be
needed with the test_and_*_bit operations. The __test_and_clear_bit()
change is needed, though. What kind of crash did you see when you did
that? It should not crash, but you could see the "attempt to clear
non-busy tag" error though.

-- 
Jens Axboe

Re: [patch] scsi: use lock per host instead of per device for shared queue tag host

[Jens Axboe]

On Wed, Jan 24 2007, Ed Lin wrote:
> 
> 
> > -----Original Message-----
> > From: David Somayajulu [mailto:david.somayajulu@qlogic.com] 
> > Sent: Wednesday, January 24, 2007 5:03 PM
> > To: Ed Lin; Michael Reed
> > Cc: linux-scsi; linux-kernel; james.Bottomley; jeff; 
> > Promise_Linux; Jens Axboe
> > Subject: RE: [patch] scsi: use lock per host instead of per 
> > device for shared queue tag host
> > 
> > 
> > > It seems another driver(qla4xxx) is also using shared queue tag.
> > > It is natural to imagine there might be same symptom in that
> > > driver. But I don't know the driver and have no hardware so I
> > > can not say anything certain about it.
> > 
> > qla4xxx implements slightly differently, in the sense we 
> > don't have the
> > equivalent of         
> > struct st_ccb ccb[MU_MAX_REQUEST]; 
> > which is in struct st_hba. In other words we don't have a local array
> > which like stex to keep track of the outstanding commands to the hba.
> > 
> > We had a discussion on this one while implementing block-layer tagging
> > in qla4xxx and Jens Axboe added the test_and_set_bit() in the 
> > following
> > code in blk_queue_start_tag() to take care of it.
> > 	do {
> > 		tag = find_first_zero_bit(bqt->tag_map, bqt->max_depth);
> > 		if (tag >= bqt->max_depth)
> > 			return 1;
> > 	} while (test_and_set_bit(tag, bqt->tag_map));
> > Please see the following link for the discussion
> > http://marc.theaimsgroup.com/?l=linux-scsi&m=115886351206726&w=2
> > 
> > Cheers
> > David Somayajulu
> > QLogic Corporation
> >
> 
> Yes, this piece of code of allocating tag, in itself, is safe.
> But the following
> 
> 	if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
> 		printk(KERN_ERR "%s: attempt to clear non-busy tag
> (%d)\n",
> 		       __FUNCTION__, tag);
> 		return;
> 	}
> 
> code of freeing tag (in blk_queue_end_tag())seems to be using
> unsafe __test_and_clear_bit instead of test_and_clear_bit.
> I once changed it to test_and_clear_bit and thought it was fixed.
> But the panic happened thereafter nonetheless(using gcc 3.4.6.
> gcc 4.1.0 is better but still with kernel errors). bqt also needs
> to be protected in this case. Replacing queue lock per device with
> a host lock is a simple but logical fix for it. To introduce a
> more refined lock is possible, but seems too tedious and elaborate
> for this issue, since a queue lock is already out there, and a
> hostwide lock is needed anyway.

Does this fix it? There really should be no need to add extra locking
for this, it would be a shame.

diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index fb67897..e752e5d 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -1072,12 +1072,16 @@ void blk_queue_end_tag(request_queue_t *q, struct request *rq)
 		 */
 		return;
 
-	if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
+	smp_mb__before_clear_bit();
+
+	if (unlikely(!test_and_clear_bit(tag, bqt->tag_map))) {
 		printk(KERN_ERR "%s: attempt to clear non-busy tag (%d)\n",
 		       __FUNCTION__, tag);
 		return;
 	}
 
+	smp_mb__after_clear_bit();
+
 	list_del_init(&rq->queuelist);
 	rq->cmd_flags &= ~REQ_QUEUED;
 	rq->tag = -1;

-- 
Jens Axboe

RE: [patch] scsi: use lock per host instead of per device for shared queue tag host

[Ed Lin]

> -----Original Message-----
> From: David Somayajulu [mailto:david.somayajulu@qlogic.com] 
> Sent: Wednesday, January 24, 2007 5:03 PM
> To: Ed Lin; Michael Reed
> Cc: linux-scsi; linux-kernel; james.Bottomley; jeff; 
> Promise_Linux; Jens Axboe
> Subject: RE: [patch] scsi: use lock per host instead of per 
> device for shared queue tag host
> 
> 
> > It seems another driver(qla4xxx) is also using shared queue tag.
> > It is natural to imagine there might be same symptom in that
> > driver. But I don't know the driver and have no hardware so I
> > can not say anything certain about it.
> 
> qla4xxx implements slightly differently, in the sense we 
> don't have the
> equivalent of         
> struct st_ccb ccb[MU_MAX_REQUEST]; 
> which is in struct st_hba. In other words we don't have a local array
> which like stex to keep track of the outstanding commands to the hba.
> 
> We had a discussion on this one while implementing block-layer tagging
> in qla4xxx and Jens Axboe added the test_and_set_bit() in the 
> following
> code in blk_queue_start_tag() to take care of it.
> 	do {
> 		tag = find_first_zero_bit(bqt->tag_map, bqt->max_depth);
> 		if (tag >= bqt->max_depth)
> 			return 1;
> 	} while (test_and_set_bit(tag, bqt->tag_map));
> Please see the following link for the discussion
> http://marc.theaimsgroup.com/?l=linux-scsi&m=115886351206726&w=2
> 
> Cheers
> David Somayajulu
> QLogic Corporation
>

Yes, this piece of code of allocating tag, in itself, is safe.
But the following

	if (unlikely(!__test_and_clear_bit(tag, bqt->tag_map))) {
		printk(KERN_ERR "%s: attempt to clear non-busy tag
(%d)\n",
		       __FUNCTION__, tag);
		return;
	}

code of freeing tag (in blk_queue_end_tag())seems to be using
unsafe __test_and_clear_bit instead of test_and_clear_bit.
I once changed it to test_and_clear_bit and thought it was fixed.
But the panic happened thereafter nonetheless(using gcc 3.4.6.
gcc 4.1.0 is better but still with kernel errors). bqt also needs
to be protected in this case. Replacing queue lock per device with
a host lock is a simple but logical fix for it. To introduce a
more refined lock is possible, but seems too tedious and elaborate
for this issue, since a queue lock is already out there, and a
hostwide lock is needed anyway.

Thanks,

Ed Lin

RE: [patch] scsi: use lock per host instead of per device for shared queue tag host

[David Somayajulu]

> It seems another driver(qla4xxx) is also using shared queue tag.
> It is natural to imagine there might be same symptom in that
> driver. But I don't know the driver and have no hardware so I
> can not say anything certain about it.

qla4xxx implements slightly differently, in the sense we don't have the
equivalent of         
struct st_ccb ccb[MU_MAX_REQUEST]; 
which is in struct st_hba. In other words we don't have a local array
which like stex to keep track of the outstanding commands to the hba.

We had a discussion on this one while implementing block-layer tagging
in qla4xxx and Jens Axboe added the test_and_set_bit() in the following
code in blk_queue_start_tag() to take care of it.
	do {
		tag = find_first_zero_bit(bqt->tag_map, bqt->max_depth);
		if (tag >= bqt->max_depth)
			return 1;
	} while (test_and_set_bit(tag, bqt->tag_map));
Please see the following link for the discussion
http://marc.theaimsgroup.com/?l=linux-scsi&m=115886351206726&w=2

Cheers
David Somayajulu
QLogic Corporation

RE: [patch] scsi: use lock per host instead of per device for shared queue tag host

[Ed Lin]

> -----Original Message-----
> From: Michael Reed [mailto:mdr@sgi.com] 
> Sent: Wednesday, January 24, 2007 7:59 AM
> To: Ed Lin
> Cc: linux-scsi; linux-kernel; james.Bottomley; jeff; Promise_Linux
> Subject: Re: [patch] scsi: use lock per host instead of per 
> device for shared queue tag host
> 
> 
> How 'bout a comment in scsh_host.h indicating that the 
> pointer will be NULL unless
> initialized by the driver?
> 
> "Protect shared block queue tag" is unique to stex.  Perhaps 
> have no comment on
> the variable declaration in scsi_host.h and explain why you 
> use it in stex.
> 
> Mike
> 
> 

Thanks for commenting. I agree more detailed explaination should
be better.

It seems another driver(qla4xxx) is also using shared queue tag.
It is natural to imagine there might be same symptom in that
driver. But I don't know the driver and have no hardware so I
can not say anything certain about it.

Ed Lin

Re: [patch] scsi: use lock per host instead of per device for shared queue tag host

[James Bottomley]

On Tue, 2007-01-23 at 16:53 -0800, Ed Lin wrote:
> The block layer uses lock to protect request queue. Every scsi device
> has a unique request queue, and queue lock is the default lock in struct
> request_queue. This is good for normal cases. But for a  host with
> shared queue tag (e.g. stex controllers), a queue lock per device means
> the shared queue tag is not protected when multiple devices are accessed
> at a same time.  This patch is a simple fix for this situation by introducing
> a host queue lock to protect shared queue tag. Without this patch we will
> see various kernel panics (including the BUG() and kernel errors in
> blk_queue_start_tag and blk_queue_end_tag of ll_rw_blk.c) when accessing
> different devices simultaneously (e.g. copying big file from one device to
> another in smp kernels).

This patch looks OK in principle.

However, are you sure you're not using a sledgehammer to crack a nut?
If the only reason you're doing this is because of the shared tag map,
then probably that should be the area you protect with a per-tag-map
lock.   The net effect of what you've done will be to serialise all
accesses to your storage devices.  For a small number of devices, this
probably won't matter than much, but for large numbers of devices,
you're probably going to introduce artificial performance degredation in
the I/O scheduler.

James

Re: [patch] scsi: use lock per host instead of per device for shared queue tag host

[Michael Reed]

How 'bout a comment in scsh_host.h indicating that the pointer will be NULL unless
initialized by the driver?

"Protect shared block queue tag" is unique to stex.  Perhaps have no comment on
the variable declaration in scsi_host.h and explain why you use it in stex.

Mike


Ed Lin wrote:
> The block layer uses lock to protect request queue. Every scsi device
> has a unique request queue, and queue lock is the default lock in struct
> request_queue. This is good for normal cases. But for a  host with
> shared queue tag (e.g. stex controllers), a queue lock per device means
> the shared queue tag is not protected when multiple devices are accessed
> at a same time.  This patch is a simple fix for this situation by introducing
> a host queue lock to protect shared queue tag. Without this patch we will
> see various kernel panics (including the BUG() and kernel errors in
> blk_queue_start_tag and blk_queue_end_tag of ll_rw_blk.c) when accessing
> different devices simultaneously (e.g. copying big file from one device to
> another in smp kernels).
> 
> This is against kernel 2.6.20-rc5.
> 
> Signed-off-by: Ed Lin <ed.lin@promise.com>
> ---
>  drivers/scsi/scsi_lib.c  |    2 +-
>  drivers/scsi/stex.c      |    2 ++
>  include/scsi/scsi_host.h |    3 +++
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff -purN a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> --- a/drivers/scsi/scsi_lib.c	2007-01-23 14:40:28.000000000 -0800
> +++ b/drivers/scsi/scsi_lib.c	2007-01-23 14:46:43.000000000 -0800
> @@ -1574,7 +1574,7 @@ struct request_queue *__scsi_alloc_queue
>  {
>  	struct request_queue *q;
>  
> -	q = blk_init_queue(request_fn, NULL);
> +	q = blk_init_queue(request_fn, shost->req_q_lock);
>  	if (!q)
>  		return NULL;
>  
> diff -purN a/drivers/scsi/stex.c b/drivers/scsi/stex.c
> --- a/drivers/scsi/stex.c	2007-01-23 14:40:28.000000000 -0800
> +++ b/drivers/scsi/stex.c	2007-01-23 14:48:59.000000000 -0800
> @@ -1254,6 +1254,8 @@ stex_probe(struct pci_dev *pdev, const s
>  	if (err)
>  		goto out_free_irq;
>  
> +	spin_lock_init(&host->__req_q_lock);
> +	host->req_q_lock = &host->__req_q_lock;
>  	err = scsi_init_shared_tag_map(host, host->can_queue);
>  	if (err) {
>  		printk(KERN_ERR DRV_NAME "(%s): init shared queue failed\n",
> diff -purN a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> --- a/include/scsi/scsi_host.h	2007-01-23 14:40:29.000000000 -0800
> +++ b/include/scsi/scsi_host.h	2007-01-23 14:57:04.000000000 -0800
> @@ -508,6 +508,9 @@ struct Scsi_Host {
>  	spinlock_t		default_lock;
>  	spinlock_t		*host_lock;
>  
> +	spinlock_t		__req_q_lock;
> +	spinlock_t		*req_q_lock;/* protect shared block queue tag */
> +
>  	struct mutex		scan_mutex;/* serialize scanning activity */
>  
>  	struct list_head	eh_cmd_q;
> 
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[patch] scsi: use lock per host instead of per device for shared queue tag host

[Ed Lin]

The block layer uses lock to protect request queue. Every scsi device
has a unique request queue, and queue lock is the default lock in struct
request_queue. This is good for normal cases. But for a  host with
shared queue tag (e.g. stex controllers), a queue lock per device means
the shared queue tag is not protected when multiple devices are accessed
at a same time.  This patch is a simple fix for this situation by introducing
a host queue lock to protect shared queue tag. Without this patch we will
see various kernel panics (including the BUG() and kernel errors in
blk_queue_start_tag and blk_queue_end_tag of ll_rw_blk.c) when accessing
different devices simultaneously (e.g. copying big file from one device to
another in smp kernels).

This is against kernel 2.6.20-rc5.

Signed-off-by: Ed Lin <ed.lin@promise.com>
---
 drivers/scsi/scsi_lib.c  |    2 +-
 drivers/scsi/stex.c      |    2 ++
 include/scsi/scsi_host.h |    3 +++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff -purN a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c	2007-01-23 14:40:28.000000000 -0800
+++ b/drivers/scsi/scsi_lib.c	2007-01-23 14:46:43.000000000 -0800
@@ -1574,7 +1574,7 @@ struct request_queue *__scsi_alloc_queue
 {
 	struct request_queue *q;
 
-	q = blk_init_queue(request_fn, NULL);
+	q = blk_init_queue(request_fn, shost->req_q_lock);
 	if (!q)
 		return NULL;
 
diff -purN a/drivers/scsi/stex.c b/drivers/scsi/stex.c
--- a/drivers/scsi/stex.c	2007-01-23 14:40:28.000000000 -0800
+++ b/drivers/scsi/stex.c	2007-01-23 14:48:59.000000000 -0800
@@ -1254,6 +1254,8 @@ stex_probe(struct pci_dev *pdev, const s
 	if (err)
 		goto out_free_irq;
 
+	spin_lock_init(&host->__req_q_lock);
+	host->req_q_lock = &host->__req_q_lock;
 	err = scsi_init_shared_tag_map(host, host->can_queue);
 	if (err) {
 		printk(KERN_ERR DRV_NAME "(%s): init shared queue failed\n",
diff -purN a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h	2007-01-23 14:40:29.000000000 -0800
+++ b/include/scsi/scsi_host.h	2007-01-23 14:57:04.000000000 -0800
@@ -508,6 +508,9 @@ struct Scsi_Host {
 	spinlock_t		default_lock;
 	spinlock_t		*host_lock;
 
+	spinlock_t		__req_q_lock;
+	spinlock_t		*req_q_lock;/* protect shared block queue tag */
+
 	struct mutex		scan_mutex;/* serialize scanning activity */
 
 	struct list_head	eh_cmd_q;