[PATCH] sg block layer tcqing fix

[PATCH] sg block layer tcqing fix

[Brian King]

[-- Attachment #1: Type: text/plain, Size: 2231 bytes --]

In bringing up a new LLD driver I am working on, I came across an oops 
related to sg and using the TCQ function provided by the block layer. 
The backtrace is shown below. I did some poking around and it looks like 
when doing SG_IO ioctls to devices running blk tagged command queuing, 
blk_queue_start_tag gets called, but blk_queue_end_tag never does. 
Attached is a patch against 2.6.0 which adds the call in sg_cmd_done and 
gets rid of the oops for me. Please apply.


-Brian


Unable to handle kernel paging request at virtual address
c5580e8c
  printing eip:
c02eacf8
*pde = 00016067
*pte = 05580000
Oops: 0002 [#1]
CPU:    1
EIP:    0060:[<c02eacf8>]    Not tainted
EFLAGS: 00010046
EIP is at blk_queue_start_tag+0x68/0x100
eax: c5580e88   ebx: 00000001   ecx: 00000000   edx: cb1884d0
esi: c5760e88   edi: cb1884c8   ebp: cb18849c   esp: c5dc3cd8
ds: 007b   es: 007b   ss: 0068
Process iprconfig (pid: 1511, threadinfo=c5dc2000 task=c642b9b0)
Stack: cdc12df8 c6442000 c02e9d3e cdc12df8 00000002 c899dbf8 c5760e88 
c6442000
        c03269dc cdc12df8 c5760e88 cdc12f58 c899dd84 c10da700 cdc12df8 
c5760e88
        cdc12df8 c02e9c29 00000002 c5760e88 cdc12df8 00000202 c02ec6a1 
cdc12df8
Call Trace:
  [<c02e9d3e>] elv_next_request+0x4e/0x100
  [<c03269dc>] scsi_request_fn+0x46c/0x4b0
  [<c02e9c29>] __elv_add_request+0x29/0x40
  [<c02ec6a1>] blk_insert_request+0x91/0x110
  [<c032525e>] scsi_do_req+0x4e/0xa0
  [<c032515a>] scsi_insert_special_req+0x3a/0x40
  [<c0350d18>] sg_common_write+0x168/0x1c0
  [<c0352060>] sg_cmd_done+0x0/0x250
  [<c0350b34>] sg_new_write+0x1f4/0x270
  [<c03519cf>] sg_ioctl+0xc5f/0xe50
  [<c01468af>] __get_free_pages+0x1f/0x50
  [<c0354190>] sg_page_malloc+0x40/0x100
  [<c0352dce>] sg_build_indirect+0x5e/0x1f0
  [<c03539a9>] sg_build_reserve+0x39/0x60
  [<c0353f2b>] sg_add_sfp+0xeb/0x120
  [<c034ff66>] sg_open+0x186/0x260
  [<c0165dcf>] get_empty_filp+0x4f/0x100
  [<c0170193>] chrdev_open+0x123/0x310
  [<c011e8f0>] default_wake_function+0x0/0x20
  [<c017a95b>] sys_ioctl+0x13b/0x2f7
  [<c016425e>] sys_open+0x7e/0x90
  [<c010996f>] syscall_call+0x7/0xb

Code: 89 70 04 89 06 31 c0 89 56 04 ff 47 10 89 77 08 83 c4 10 5b



-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

[-- Attachment #2: sg_tcq.patch --]
[-- Type: text/plain, Size: 861 bytes --]

diff -Naur linux-2.6.0/drivers/scsi/sg.c linux-2.6.0-sg/drivers/scsi/sg.c
--- linux-2.6.0/drivers/scsi/sg.c	Mon Dec  8 11:10:40 2003
+++ linux-2.6.0-sg/drivers/scsi/sg.c	Mon Dec 29 13:03:35 2003
@@ -1219,6 +1219,8 @@
 	Sg_device *sdp = NULL;
 	Sg_fd *sfp;
 	Sg_request *srp = NULL;
+	request_queue_t *q;
+	unsigned long flags = 0;
 
 	if (SCpnt && (SRpnt = SCpnt->sc_request))
 		srp = (Sg_request *) SRpnt->upper_private_data;
@@ -1284,6 +1286,12 @@
 	}
 	/* Rely on write phase to clean out srp status values, so no "else" */
 
+	q = SRpnt->sr_device->request_queue;
+	spin_lock_irqsave(q->queue_lock, flags);
+	if (blk_rq_tagged(SRpnt->sr_request))
+		blk_queue_end_tag(q, SRpnt->sr_request);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
 	scsi_release_request(SRpnt);
 	SRpnt = NULL;
 	if (sfp->closed) {	/* whoops this fd already released, cleanup */

Re: [PATCH] sg block layer tcqing fix

[Douglas Gilbert]

Brian King wrote:
> In bringing up a new LLD driver I am working on, I came across an oops 
> related to sg and using the TCQ function provided by the block layer. 
> The backtrace is shown below. I did some poking around and it looks like 
> when doing SG_IO ioctls to devices running blk tagged command queuing, 
> blk_queue_start_tag gets called, but blk_queue_end_tag never does. 
> Attached is a patch against 2.6.0 which adds the call in sg_cmd_done and 
> gets rid of the oops for me. Please apply.

Brian,
Thanks for this patch. As far as I can tell this patch is needed
for all SCSI char ULDs (in other words: sg, st and osst) when
used with a LLD that uses block layer queueing infrastructure.
At the moment that is only the 53c700 driver (maintained by James B.)
and that one you are working on (I suspect).

Given that no other ULDs call blk_queue_*_tag() routines it seems
a bit strange that sg, st and osst need to call blk_queue_end_tag()
prior to calling scsi_release_request(). Perhaps the cleanup can be
built into scsi_release_request() or a new variant (e.g.
scsi_release_special_request() ) could be introduced.

Doug Gilbert

Re: [PATCH] sg block layer tcqing fix

[Willem Riede]

On 2004.01.06 00:06, Douglas Gilbert wrote:
> Brian King wrote:
> > In bringing up a new LLD driver I am working on, I came across an oops 
> > related to sg and using the TCQ function provided by the block layer. 
> > The backtrace is shown below. I did some poking around and it looks like 
> > when doing SG_IO ioctls to devices running blk tagged command queuing, 
> > blk_queue_start_tag gets called, but blk_queue_end_tag never does. 
> > Attached is a patch against 2.6.0 which adds the call in sg_cmd_done and 
> > gets rid of the oops for me. Please apply.
> 
> Brian,
> Thanks for this patch. As far as I can tell this patch is needed
> for all SCSI char ULDs (in other words: sg, st and osst) when
> used with a LLD that uses block layer queueing infrastructure.
> At the moment that is only the 53c700 driver (maintained by James B.)
> and that one you are working on (I suspect).
> 
> Given that no other ULDs call blk_queue_*_tag() routines it seems
> a bit strange that sg, st and osst need to call blk_queue_end_tag()
> prior to calling scsi_release_request(). Perhaps the cleanup can be
> built into scsi_release_request() or a new variant (e.g.
> scsi_release_special_request() ) could be introduced.

IMHO, it would be much cleaner if sg, st and osst didn't need to be
aware of tagging, so I'd prefer that it be handled automagically in
the existing scsi_release_request().

Thanks, Willem Riede,
osst maintainer.

Re: [PATCH] sg block layer tcqing fix

[James Bottomley]

On Mon, 2004-01-05 at 23:06, Douglas Gilbert wrote:
> Given that no other ULDs call blk_queue_*_tag() routines it seems
> a bit strange that sg, st and osst need to call blk_queue_end_tag()
> prior to calling scsi_release_request(). Perhaps the cleanup can be
> built into scsi_release_request() or a new variant (e.g.
> scsi_release_special_request() ) could be introduced.

I agree.  Having the ULD stop tags but not start them is a layering
violation.  The attached patch will end the tag in the release request.

It's still appropriate to release the tag earlier, as the mid layer does
to free up tag slots for other requests.  Once the tag is ended, the tag
queue flag is cleared, so the check in release request now fails.

James

===== drivers/scsi/scsi.c 1.132 vs edited =====
--- 1.132/drivers/scsi/scsi.c	Tue Sep 30 09:24:17 2003
+++ edited/drivers/scsi/scsi.c	Tue Jan  6 09:10:25 2004
@@ -142,6 +142,23 @@
 
 void __scsi_release_request(struct scsi_request *sreq)
 {
+	struct request *req = sreq->sr_request;
+
+	/* unlikely because the tag was usually ended earlier
+	 *
+	 * NOTE: the lock should be held while checking the
+	 * flag.  However, any race here requiring the lock
+	 * would be a gross error. */
+	if (unlikely(blk_rq_tagged(req))) {
+		unsigned long flags;
+		struct request_queue *q = req->q;
+
+		spin_lock_irqsave(q->queue_lock, flags);
+		blk_queue_end_tag(q, req);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+	}
+
+
 	if (likely(sreq->sr_command != NULL)) {
 		struct scsi_cmnd *cmd = sreq->sr_command;
 

Re: [PATCH] sg block layer tcqing fix

[Jens Axboe]

On Tue, Jan 06 2004, James Bottomley wrote:
> On Mon, 2004-01-05 at 23:06, Douglas Gilbert wrote:
> > Given that no other ULDs call blk_queue_*_tag() routines it seems
> > a bit strange that sg, st and osst need to call blk_queue_end_tag()
> > prior to calling scsi_release_request(). Perhaps the cleanup can be
> > built into scsi_release_request() or a new variant (e.g.
> > scsi_release_special_request() ) could be introduced.
> 
> I agree.  Having the ULD stop tags but not start them is a layering
> violation.  The attached patch will end the tag in the release request.
> 
> It's still appropriate to release the tag earlier, as the mid layer does
> to free up tag slots for other requests.  Once the tag is ended, the tag
> queue flag is cleared, so the check in release request now fails.
> 
> James
> 
> ===== drivers/scsi/scsi.c 1.132 vs edited =====
> --- 1.132/drivers/scsi/scsi.c	Tue Sep 30 09:24:17 2003
> +++ edited/drivers/scsi/scsi.c	Tue Jan  6 09:10:25 2004
> @@ -142,6 +142,23 @@
>  
>  void __scsi_release_request(struct scsi_request *sreq)
>  {
> +	struct request *req = sreq->sr_request;
> +
> +	/* unlikely because the tag was usually ended earlier
> +	 *
> +	 * NOTE: the lock should be held while checking the
> +	 * flag.  However, any race here requiring the lock
> +	 * would be a gross error. */

I don't agree with this comment - why would you need to have the queue
lock to check req->flags?

-- 
Jens Axboe

Re: [PATCH] sg block layer tcqing fix

[James Bottomley]

On Tue, 2004-01-06 at 09:22, Jens Axboe wrote:
> On Tue, Jan 06 2004, James Bottomley wrote:
> > +	/* unlikely because the tag was usually ended earlier
> > +	 *
> > +	 * NOTE: the lock should be held while checking the
> > +	 * flag.  However, any race here requiring the lock
> > +	 * would be a gross error. */
> 
> I don't agree with this comment - why would you need to have the queue
> lock to check req->flags?

Because the usual scsi method of doing this is:

	spin_lock_irqsave(q->queue_lock, flags);
	if (blk_rq_tagged(req))
		blk_queue_end_tag(q, req);
	spin_unlock_irqrestore(q->queue_lock, flags);

which was done because the REQ_QUEUE flag is altered by the
blk_queue_end_tag(), so I wanted to note that I'd done this one
differently deliberately.

On the other hand, I don't see the possibility of a legal race to do
this in any of our other block tag ending code either...perhaps I should
just convert them all to this form.

James

Re: [PATCH] sg block layer tcqing fix

[Jens Axboe]

On Tue, Jan 06 2004, James Bottomley wrote:
> On Tue, 2004-01-06 at 09:22, Jens Axboe wrote:
> > On Tue, Jan 06 2004, James Bottomley wrote:
> > > +	/* unlikely because the tag was usually ended earlier
> > > +	 *
> > > +	 * NOTE: the lock should be held while checking the
> > > +	 * flag.  However, any race here requiring the lock
> > > +	 * would be a gross error. */
> > 
> > I don't agree with this comment - why would you need to have the queue
> > lock to check req->flags?
> 
> Because the usual scsi method of doing this is:
> 
> 	spin_lock_irqsave(q->queue_lock, flags);
> 	if (blk_rq_tagged(req))
> 		blk_queue_end_tag(q, req);
> 	spin_unlock_irqrestore(q->queue_lock, flags);
> 
> which was done because the REQ_QUEUE flag is altered by the
> blk_queue_end_tag(), so I wanted to note that I'd done this one
> differently deliberately.

That doesn't mean it's not safe to check req->flags outside the lock,
it's definitely not safe to call blk_queue_end_tag() without it though.
The above should probably be:

	if (blk_rq_tagged(req)) {
		spin_lock_irqsave(q->queue_lock, flags);
		blk_queue_end_tag(q, req);
		spin_unlock_irqrestore(q->queue_lock, flags);
	}

> On the other hand, I don't see the possibility of a legal race to do
> this in any of our other block tag ending code either...perhaps I should
> just convert them all to this form.

I think so, yes. There should only be one passing down the request.

-- 
Jens Axboe