[RFC] uas host lock removal conversion

[RFC] uas host lock removal conversion

[Matthew Wilcox]

I thought I'd take a crack at removing the host lock from uas.  I think
I can now switch to using GFP_NOIO (since interrupts are enabled by
scsi_request_fn()), and I have to use _irq spinlocks to protect the list
that is modified from interrupt context (same reason :-).

Comments?

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5b6bbae..9e881cd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1443,7 +1443,7 @@ static void scsi_softirq_done(struct request *rq)
  *
  * Returns:     Nothing
  *
- * Lock status: IO request lock assumed to be held when called.
+ * Lock status: request queue lock held, interrupts disabled
  */
 static void scsi_request_fn(struct request_queue *q)
 {
@@ -1530,10 +1530,6 @@ static void scsi_request_fn(struct request_queue *q)
 		scsi_target(sdev)->target_busy++;
 		shost->host_busy++;
 
-		/*
-		 * XXX(hch): This is rather suboptimal, scsi_dispatch_cmd will
-		 *		take the lock again.
-		 */
 		spin_unlock_irq(shost->host_lock);
 
 		/*
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 23f0dd9..a2cff4b 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -433,8 +433,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
 	return 0;
 }
 
-static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
-					void (*done)(struct scsi_cmnd *))
+static int uas_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
 {
 	struct scsi_device *sdev = cmnd->device;
 	struct uas_dev_info *devinfo = sdev->hostdata;
@@ -453,8 +452,6 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 		cmdinfo->stream = 1;
 	}
 
-	cmnd->scsi_done = done;
-
 	cmdinfo->state = ALLOC_STATUS_URB | SUBMIT_STATUS_URB |
 			ALLOC_CMD_URB | SUBMIT_CMD_URB;
 
@@ -475,24 +472,22 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd,
 		cmdinfo->stream = 0;
 	}
 
-	err = uas_submit_urbs(cmnd, devinfo, GFP_ATOMIC);
+	err = uas_submit_urbs(cmnd, devinfo, GFP_NOIO);
 	if (err) {
 		/* If we did nothing, give up now */
 		if (cmdinfo->state & SUBMIT_STATUS_URB) {
 			usb_free_urb(cmdinfo->status_urb);
 			return SCSI_MLQUEUE_DEVICE_BUSY;
 		}
-		spin_lock(&uas_work_lock);
+		spin_lock_irq(&uas_work_lock);
 		list_add_tail(&cmdinfo->list, &uas_work_list);
-		spin_unlock(&uas_work_lock);
+		spin_unlock_irq(&uas_work_lock);
 		schedule_work(&uas_work);
 	}
 
 	return 0;
 }
 
-static DEF_SCSI_QCMD(uas_queuecommand)
-
 static int uas_eh_abort_handler(struct scsi_cmnd *cmnd)
 {
 	struct scsi_device *sdev = cmnd->device;

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Land iscsi iser performance patches to upstream

[Zhiqi Tao]

Hi,

Could anyone advise if there is any plan to push these patches to land 
the upstream?

Patch 1/3
http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg14533.html
Patch 2/3
http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg14381.html
Patch 3/3
http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg14398.html


In particular patch 2 and 3 are matching the iscsi_tcp.c values and have 
improved the performance that we got. It will be beneficial to include 
them into the vanilla kernel.

Thanks!
Zhiqi

Re: [RFC] uas host lock removal conversion

[Christoph Hellwig]

On Wed, Dec 15, 2010 at 10:02:57PM -0700, Matthew Wilcox wrote:
> 
> I thought I'd take a crack at removing the host lock from uas.  I think
> I can now switch to using GFP_NOIO (since interrupts are enabled by
> scsi_request_fn()), and I have to use _irq spinlocks to protect the list
> that is modified from interrupt context (same reason :-).

->qeuecommand can be called from software interrupt context, so you'll
need to stick to GFP_ATOMIC.

Re: [RFC] uas host lock removal conversion

[Matthew Wilcox]

On Thu, Dec 16, 2010 at 05:44:58AM -0500, Christoph Hellwig wrote:
> On Wed, Dec 15, 2010 at 10:02:57PM -0700, Matthew Wilcox wrote:
> > 
> > I thought I'd take a crack at removing the host lock from uas.  I think
> > I can now switch to using GFP_NOIO (since interrupts are enabled by
> > scsi_request_fn()), and I have to use _irq spinlocks to protect the list
> > that is modified from interrupt context (same reason :-).
> 
> ->qeuecommand can be called from software interrupt context, so you'll
> need to stick to GFP_ATOMIC.

Ah, I couldn't find a path that did that.  Could you point it out to me?
I'll send a patch to update that documentation.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC] uas host lock removal conversion

[James Bottomley]

On Thu, 2010-12-16 at 07:00 -0700, Matthew Wilcox wrote:
> On Thu, Dec 16, 2010 at 05:44:58AM -0500, Christoph Hellwig wrote:
> > On Wed, Dec 15, 2010 at 10:02:57PM -0700, Matthew Wilcox wrote:
> > > 
> > > I thought I'd take a crack at removing the host lock from uas.  I think
> > > I can now switch to using GFP_NOIO (since interrupts are enabled by
> > > scsi_request_fn()), and I have to use _irq spinlocks to protect the list
> > > that is modified from interrupt context (same reason :-).
> > 
> > ->qeuecommand can be called from software interrupt context, so you'll
> > need to stick to GFP_ATOMIC.
> 
> Ah, I couldn't find a path that did that.  Could you point it out to me?
> I'll send a patch to update that documentation.

Well, the documentation (scsi_mid_low_api.txt) says this.  The reason is
the blk_run_queue in softirq context.

James

Re: Land iscsi iser performance patches to upstream

[Mike Christie]

On 12/16/2010 12:15 AM, Zhiqi Tao wrote:
> Hi,
>
> Could anyone advise if there is any plan to push these patches to land
> the upstream?
>
> Patch 1/3
> http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg14533.html
> Patch 2/3
> http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg14381.html
> Patch 3/3
> http://www.mail-archive.com/linux-scsi@vger.kernel.org/msg14398.html
>
>
> In particular patch 2 and 3 are matching the iscsi_tcp.c values and have
> improved the performance that we got. It will be beneficial to include
> them into the vanilla kernel.
>

Hey Or,

These patches look ok except for #1. Did you guys have some weird 
restrictions? Or did you have some weird restrictions but the iser 
driver handles it (see iser_data_buf_aligned_len related code). I think 
instead of 0 you want to set the dma alignment to 4K?

Re: Land iscsi iser performance patches to upstream

[Or Gerlitz]

On 1/4/2011 12:32 AM, Mike Christie wrote:
> These patches look ok except for #1. Did you guys have some weird 
> restrictions? Or did you have some weird restrictions but the iser 
> driver handles it (see iser_data_buf_aligned_len related code). I 
> think instead of 0 you want to set the dma alignment to 4K? 

Hi Mike,

Looks like I missed this ~3 years old patch-set, will look into that and 
let you know.

Or.

Re: Land iscsi iser performance patches to upstream

[Or Gerlitz]

On 12/16/2010 12:15 AM, Zhiqi Tao wrote:
>> In particular patch 2 and 3 are matching the iscsi_tcp.c values and 
>> have improved the performance that we got. It will be beneficial to 
>> include  them into the vanilla kernel.
When saying "patch 2 and 3 have improved the performance that we got" - 
did you mean the performance with iser or with iscsi-tcp? if with iser, 
can you spare few words on the test and what was the performance 
improvement?

These patches allow to support e.g 1MB size commands, vs. the 512KB 
limit of the upstream bits, however, we manage to see peak performance 
with much IO sizes (e.g 128KB, 256KB), so I wasn't sure what these 
patches could buy you performance wise.

Or.