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
-
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/
