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 <[email protected]>
---
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;
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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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
> -----Original Message-----
> From: Michael Reed [mailto:[email protected]]
> 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
> 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
> -----Original Message-----
> From: David Somayajulu [mailto:[email protected]]
> 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
On Wed, Jan 24 2007, Ed Lin wrote:
>
>
> > -----Original Message-----
> > From: David Somayajulu [mailto:[email protected]]
> > 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
On Thu, Jan 25 2007, Jens Axboe wrote:
> On Wed, Jan 24 2007, Ed Lin wrote:
> >
> >
> > > -----Original Message-----
> > > From: David Somayajulu [mailto:[email protected]]
> > > 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
> -----Original Message-----
> From: Jens Axboe [mailto:[email protected]]
> 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:[email protected]]
> > > > 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
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