[REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[Hans de Goede]

Hi All,

While working on making error handling in the uas driver more robust,
I noticed that all the commands being send to a sata ssd hooked up
over uas were untagged, where I would expect tcq to be used, as that
is the big advantage of uas over usb-storage / bot.

Taking the uas.c file from 3.17, and building it for 3.16 restores
the use of tcq (debugged by adding a printk blk_rq_tagged + request->tag).

So either uas is doing something wrong which happened to work in
3.16, or something has broken in 3.17.

I've already added debug printk-s of scsi_device->tagged_supported,
queue_depth, ordered_tags and simple_tags and those all look good
(1, 29, 1, 1).

I've also tried setting disable_blk_mq and that does not help.

Any hints to help debugging this further (other then a bisect) are
appreciated. If no-one has any smart ideas I guess I'll end up doing
a full bisect.

Regards,

Hans

Re: [REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[Christoph Hellwig]

On Tue, Sep 09, 2014 at 11:15:24AM +0200, Hans de Goede wrote:
> Taking the uas.c file from 3.17, and building it for 3.16 restores
> the use of tcq (debugged by adding a printk blk_rq_tagged + request->tag).
> 
> So either uas is doing something wrong which happened to work in
> 3.16, or something has broken in 3.17.
> 
> I've already added debug printk-s of scsi_device->tagged_supported,
> queue_depth, ordered_tags and simple_tags and those all look good
> (1, 29, 1, 1).
> 
> I've also tried setting disable_blk_mq and that does not help.
> 
> Any hints to help debugging this further (other then a bisect) are
> appreciated. If no-one has any smart ideas I guess I'll end up doing
> a full bisect.

scsi-mq isn't enabled by default, so setting disable_blk_mq should
indeed not make a difference.

One interesting thing with uas is that it uses scsi_init_shared_tag_map,
which only few drivers do.

Can you apply a debug patch like the one below and see if that helps to
poinpoint down the issue?

diff --git a/block/blk-tag.c b/block/blk-tag.c
index a185b86..584db25 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -175,8 +175,10 @@ int blk_queue_init_tags(struct request_queue *q, int depth,
 			return rc;
 		queue_flag_set(QUEUE_FLAG_QUEUED, q);
 		return 0;
-	} else
+	} else {
+		printk("%s: grabbing reference to shared tag structure!\n", __func__);
 		atomic_inc(&tags->refcnt);
+	}
 
 	/*
 	 * assign it, all done
diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h
index cdcc90b..7f3b5cb 100644
--- a/include/scsi/scsi_tcq.h
+++ b/include/scsi/scsi_tcq.h
@@ -164,8 +164,13 @@ static inline int scsi_init_shared_tag_map(struct Scsi_Host *shost, int depth)
 	 */
 	if (!shost->bqt) {
 		shost->bqt = blk_init_tags(depth);
-		if (!shost->bqt)
+		if (!shost->bqt) {
+			printk("failed to init host-wide tag map!\n");
 			return -ENOMEM;
+		}
+		printk("initialized host-wide tag map!\n");
+	} else {
+		printk("host-wide tag map already set!\n");
 	}
 
 	return 0;

Re: [REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[Hans de Goede]

Hi,

On 09/09/2014 05:27 PM, Christoph Hellwig wrote:
> On Tue, Sep 09, 2014 at 11:15:24AM +0200, Hans de Goede wrote:
>> Taking the uas.c file from 3.17, and building it for 3.16 restores
>> the use of tcq (debugged by adding a printk blk_rq_tagged + request->tag).
>>
>> So either uas is doing something wrong which happened to work in
>> 3.16, or something has broken in 3.17.
>>
>> I've already added debug printk-s of scsi_device->tagged_supported,
>> queue_depth, ordered_tags and simple_tags and those all look good
>> (1, 29, 1, 1).
>>
>> I've also tried setting disable_blk_mq and that does not help.
>>
>> Any hints to help debugging this further (other then a bisect) are
>> appreciated. If no-one has any smart ideas I guess I'll end up doing
>> a full bisect.
> 
> scsi-mq isn't enabled by default, so setting disable_blk_mq should
> indeed not make a difference.
> 
> One interesting thing with uas is that it uses scsi_init_shared_tag_map,
> which only few drivers do.
> 
> Can you apply a debug patch like the one below and see if that helps to
> poinpoint down the issue?

I've applied the patch, this results in the following new dmesg output
when using uas:

[  120.602632] initialized host-wide tag map!

Thank you for looking into this.

Regards,

Hans

Re: [REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[Christoph Hellwig]

On Wed, Sep 10, 2014 at 09:21:24AM +0200, Hans de Goede wrote:
> I've applied the patch, this results in the following new dmesg output
> when using uas:
> 
> [  120.602632] initialized host-wide tag map!
> 
> Thank you for looking into this.

So we're initializing the tag map, but scsi_activate_tcq doesn't pick it
up.  I can't really come up with a good explanation for it, but there
even without that there is an elephant in the room:  as part of the
scsi-mq series I moved the bqt field used for this into a union with the
new blk_mq_tag_set.  Below is a patch to get rid of that union, can you
try if that fixes it?


diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index d0f69a3..bcffff2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -584,10 +584,8 @@ struct Scsi_Host {
 	 * Area to keep a shared tag map (if needed, will be
 	 * NULL if not).
 	 */
-	union {
-		struct blk_queue_tag	*bqt;
-		struct blk_mq_tag_set	tag_set;
-	};
+	struct blk_queue_tag	*bqt;
+	struct blk_mq_tag_set	tag_set;
 
 	atomic_t host_busy;		   /* commands actually active on low-level */
 	atomic_t host_blocked;

Re: [REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[Christoph Hellwig]

On Wed, Sep 10, 2014 at 09:21:24AM +0200, Hans de Goede wrote:
> I've applied the patch, this results in the following new dmesg output
> when using uas:
> 
> [  120.602632] initialized host-wide tag map!
> 
> Thank you for looking into this.

So we're initializing the tag map, but scsi_activate_tcq doesn't pick it
up.  I can't really come up with a good explanation for it, but there
even without that there is an elephant in the room:  as part of the
scsi-mq series I moved the bqt field used for this into a union with the
new blk_mq_tag_set.  Below is a patch to get rid of that union, can you
try if that fixes it?


diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index d0f69a3..bcffff2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -584,10 +584,8 @@ struct Scsi_Host {
 	 * Area to keep a shared tag map (if needed, will be
 	 * NULL if not).
 	 */
-	union {
-		struct blk_queue_tag	*bqt;
-		struct blk_mq_tag_set	tag_set;
-	};
+	struct blk_queue_tag	*bqt;
+	struct blk_mq_tag_set	tag_set;
 
 	atomic_t host_busy;		   /* commands actually active on low-level */
 	atomic_t host_blocked;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[Hans de Goede]

Hi,

On 09/10/2014 05:45 PM, Christoph Hellwig wrote:
> On Wed, Sep 10, 2014 at 09:21:24AM +0200, Hans de Goede wrote:
>> I've applied the patch, this results in the following new dmesg output
>> when using uas:
>>
>> [  120.602632] initialized host-wide tag map!
>>
>> Thank you for looking into this.
> 
> So we're initializing the tag map, but scsi_activate_tcq doesn't pick it
> up.  I can't really come up with a good explanation for it, but there
> even without that there is an elephant in the room:  as part of the
> scsi-mq series I moved the bqt field used for this into a union with the
> new blk_mq_tag_set.  Below is a patch to get rid of that union, can you
> try if that fixes it?

Unfortunately that does not fix it :|

Regards,

Hans

> 
> 
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index d0f69a3..bcffff2 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -584,10 +584,8 @@ struct Scsi_Host {
>  	 * Area to keep a shared tag map (if needed, will be
>  	 * NULL if not).
>  	 */
> -	union {
> -		struct blk_queue_tag	*bqt;
> -		struct blk_mq_tag_set	tag_set;
> -	};
> +	struct blk_queue_tag	*bqt;
> +	struct blk_mq_tag_set	tag_set;
>  
>  	atomic_t host_busy;		   /* commands actually active on low-level */
>  	atomic_t host_blocked;
> 

Re: [REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[Hans de Goede]

Hi,

On 09/10/2014 05:45 PM, Christoph Hellwig wrote:
> On Wed, Sep 10, 2014 at 09:21:24AM +0200, Hans de Goede wrote:
>> I've applied the patch, this results in the following new dmesg output
>> when using uas:
>>
>> [  120.602632] initialized host-wide tag map!
>>
>> Thank you for looking into this.
> 
> So we're initializing the tag map, but scsi_activate_tcq doesn't pick it
> up.  I can't really come up with a good explanation for it, but there
> even without that there is an elephant in the room:  as part of the
> scsi-mq series I moved the bqt field used for this into a union with the
> new blk_mq_tag_set.  Below is a patch to get rid of that union, can you
> try if that fixes it?

Unfortunately that does not fix it :|

Regards,

Hans

> 
> 
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index d0f69a3..bcffff2 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -584,10 +584,8 @@ struct Scsi_Host {
>  	 * Area to keep a shared tag map (if needed, will be
>  	 * NULL if not).
>  	 */
> -	union {
> -		struct blk_queue_tag	*bqt;
> -		struct blk_mq_tag_set	tag_set;
> -	};
> +	struct blk_queue_tag	*bqt;
> +	struct blk_mq_tag_set	tag_set;
>  
>  	atomic_t host_busy;		   /* commands actually active on low-level */
>  	atomic_t host_blocked;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[Christoph Hellwig]

On Thu, Sep 11, 2014 at 12:01:13PM +0200, Hans de Goede wrote:
> > So we're initializing the tag map, but scsi_activate_tcq doesn't pick it
> > up.  I can't really come up with a good explanation for it, but there
> > even without that there is an elephant in the room:  as part of the
> > scsi-mq series I moved the bqt field used for this into a union with the
> > new blk_mq_tag_set.  Below is a patch to get rid of that union, can you
> > try if that fixes it?
> 
> Unfortunately that does not fix it :|

Alright, I guess we need the big bisect hammer unfortunately.

We should be able to start with trying the first and last commit
of the big SCSI merge:

 start:		fcc95a763444017288b318d48367098850c23c0d
 end:		c21a2c1a4973c8dde32557970fdb44eaa9489aeb

Re: [REGRESSION 3.17] scsi (uas) disks no longer using tagged command queuing

[Hans de Goede]

Hi,

On 09/11/2014 06:13 PM, Christoph Hellwig wrote:
> On Thu, Sep 11, 2014 at 12:01:13PM +0200, Hans de Goede wrote:
>>> So we're initializing the tag map, but scsi_activate_tcq doesn't pick it
>>> up.  I can't really come up with a good explanation for it, but there
>>> even without that there is an elephant in the room:  as part of the
>>> scsi-mq series I moved the bqt field used for this into a union with the
>>> new blk_mq_tag_set.  Below is a patch to get rid of that union, can you
>>> try if that fixes it?
>>
>> Unfortunately that does not fix it :|
> 
> Alright, I guess we need the big bisect hammer unfortunately.
> 
> We should be able to start with trying the first and last commit
> of the big SCSI merge:
> 
>  start:		fcc95a763444017288b318d48367098850c23c0d
>  end:		c21a2c1a4973c8dde32557970fdb44eaa9489aeb

The bisect points to:

[hans@shalem linux]$ git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[d285203cf647d7c97db3a1c33794315c9008593f] scsi: add support for a blk-mq
based I/O path.

I've double checked that that commit is bad, and one commit earlier in the
history is good, so that really is the one.

I've tried again to fix the union thing you spotted, which is introduced
by that commit, and again it does not help.

I've taken a quick look at the commit, and my guess with be this has
something to do with the changes surrounding __scsi_alloc_queue .

Regards,

Hans

[PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Christoph Hellwig]

Please try the fix below, looks like the commit broke TCQ for all drivers 
using block-level tagging.

---
>From 865a19b760d2786fe37d3b5c151a4ecea4c0e95e Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 12 Sep 2014 16:00:19 -0700
Subject: scsi: fix regression that accidentally disabled block-based tcq

The scsi blk-mq support accidentally flipped a conditional, which lead to
never enabling block based tcq when using the legacy request path.

Fixes: d285203cf647d7c9 scsi: add support for a blk-mq based I/O path.
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/scsi/scsi_tcq.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h
index cdcc90b..e645835 100644
--- a/include/scsi/scsi_tcq.h
+++ b/include/scsi/scsi_tcq.h
@@ -68,7 +68,7 @@ static inline void scsi_activate_tcq(struct scsi_device *sdev, int depth)
 		return;
 
 	if (!shost_use_blk_mq(sdev->host) &&
-	    blk_queue_tagged(sdev->request_queue))
+	    !blk_queue_tagged(sdev->request_queue))
 		blk_queue_init_tags(sdev->request_queue, depth,
 				    sdev->host->bqt);
 
-- 
1.9.1

Re: [PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Hans de Goede]

Hi,

On 09/13/2014 01:00 AM, Christoph Hellwig wrote:
> Please try the fix below, looks like the commit broke TCQ for all drivers 
> using block-level tagging.

Yes this one does the trick and fixes things. Note the git tree I used for
testing also had your previous fix to split up the blk_tcq union in 2
separate struct members. Let me know if you want me to re-test without that
fix.

Thanks & Regards,

Hans


> 
> ---
> From 865a19b760d2786fe37d3b5c151a4ecea4c0e95e Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 12 Sep 2014 16:00:19 -0700
> Subject: scsi: fix regression that accidentally disabled block-based tcq
> 
> The scsi blk-mq support accidentally flipped a conditional, which lead to
> never enabling block based tcq when using the legacy request path.
> 
> Fixes: d285203cf647d7c9 scsi: add support for a blk-mq based I/O path.
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/scsi/scsi_tcq.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h
> index cdcc90b..e645835 100644
> --- a/include/scsi/scsi_tcq.h
> +++ b/include/scsi/scsi_tcq.h
> @@ -68,7 +68,7 @@ static inline void scsi_activate_tcq(struct scsi_device *sdev, int depth)
>  		return;
>  
>  	if (!shost_use_blk_mq(sdev->host) &&
> -	    blk_queue_tagged(sdev->request_queue))
> +	    !blk_queue_tagged(sdev->request_queue))
>  		blk_queue_init_tags(sdev->request_queue, depth,
>  				    sdev->host->bqt);
>  
> 

Re: [PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Christoph Hellwig]

On Sat, Sep 13, 2014 at 12:28:41PM +0200, Hans de Goede wrote:
> Yes this one does the trick and fixes things. Note the git tree I used for
> testing also had your previous fix to split up the blk_tcq union in 2
> separate struct members. Let me know if you want me to re-test without that
> fix.

I'm pretty sure that isn't needed, so double testing it indeed doesn't
would be helpful.  I'd like to add your Tested-by: tag after that, too.

Re: [PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Hans de Goede]

Hi,

On 09/13/2014 07:50 PM, Christoph Hellwig wrote:
> On Sat, Sep 13, 2014 at 12:28:41PM +0200, Hans de Goede wrote:
>> Yes this one does the trick and fixes things. Note the git tree I used for
>> testing also had your previous fix to split up the blk_tcq union in 2
>> separate struct members. Let me know if you want me to re-test without that
>> fix.
> 
> I'm pretty sure that isn't needed, so double testing it indeed doesn't
> would be helpful.

I can confirm that things still work fine with the union left in place.

>  I'd like to add your Tested-by: tag after that, too.

Ack:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

review-ping: Re: [PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Christoph Hellwig]

Can I get some reviews for this patch so that we can get into 3.17-rc?

Re: [PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Webb Scales]

On 9/12/14 7:00 PM, Christoph Hellwig wrote:
> Please try the fix below, looks like the commit broke TCQ for all drivers
> using block-level tagging.
>
> ---
>  From 865a19b760d2786fe37d3b5c151a4ecea4c0e95e Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 12 Sep 2014 16:00:19 -0700
> Subject: scsi: fix regression that accidentally disabled block-based tcq
>
> The scsi blk-mq support accidentally flipped a conditional, which lead to
> never enabling block based tcq when using the legacy request path.
>
> Fixes: d285203cf647d7c9 scsi: add support for a blk-mq based I/O path.
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   include/scsi/scsi_tcq.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h
> index cdcc90b..e645835 100644
> --- a/include/scsi/scsi_tcq.h
> +++ b/include/scsi/scsi_tcq.h
> @@ -68,7 +68,7 @@ static inline void scsi_activate_tcq(struct scsi_device *sdev, int depth)
>   		return;
>   
>   	if (!shost_use_blk_mq(sdev->host) &&
> -	    blk_queue_tagged(sdev->request_queue))
> +	    !blk_queue_tagged(sdev->request_queue))
>   		blk_queue_init_tags(sdev->request_queue, depth,
>   				    sdev->host->bqt);
>   


Reviewed-by: Webb Scales <webbnh@hp.com>

Re: [PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Hans de Goede]

Hi,

On 9/12/14 7:00 PM, Christoph Hellwig wrote:
> Please try the fix below, looks like the commit broke TCQ for all drivers
> using block-level tagging.
>
> ---
>  From 865a19b760d2786fe37d3b5c151a4ecea4c0e95e Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 12 Sep 2014 16:00:19 -0700
> Subject: scsi: fix regression that accidentally disabled block-based tcq
>
> The scsi blk-mq support accidentally flipped a conditional, which lead to
> never enabling block based tcq when using the legacy request path.
>
> Fixes: d285203cf647d7c9 scsi: add support for a blk-mq based I/O path.
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   include/scsi/scsi_tcq.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h
> index cdcc90b..e645835 100644
> --- a/include/scsi/scsi_tcq.h
> +++ b/include/scsi/scsi_tcq.h
> @@ -68,7 +68,7 @@ static inline void scsi_activate_tcq(struct scsi_device *sdev, int depth)
>   		return;
>   
>   	if (!shost_use_blk_mq(sdev->host) &&
> -	    blk_queue_tagged(sdev->request_queue))
> +	    !blk_queue_tagged(sdev->request_queue))
>   		blk_queue_init_tags(sdev->request_queue, depth,
>   				    sdev->host->bqt);
>   

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

Re: [PATCH] scsi: fix regression that accidentally disabled block-based tcq

[Jeff Moyer]

Christoph Hellwig <hch@infradead.org> writes:

> Please try the fix below, looks like the commit broke TCQ for all drivers 
> using block-level tagging.
>
> ---
> From 865a19b760d2786fe37d3b5c151a4ecea4c0e95e Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 12 Sep 2014 16:00:19 -0700
> Subject: scsi: fix regression that accidentally disabled block-based tcq
>
> The scsi blk-mq support accidentally flipped a conditional, which lead to
> never enabling block based tcq when using the legacy request path.
>
> Fixes: d285203cf647d7c9 scsi: add support for a blk-mq based I/O path.
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/scsi/scsi_tcq.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h
> index cdcc90b..e645835 100644
> --- a/include/scsi/scsi_tcq.h
> +++ b/include/scsi/scsi_tcq.h
> @@ -68,7 +68,7 @@ static inline void scsi_activate_tcq(struct scsi_device *sdev, int depth)
>  		return;
>  
>  	if (!shost_use_blk_mq(sdev->host) &&
> -	    blk_queue_tagged(sdev->request_queue))
> +	    !blk_queue_tagged(sdev->request_queue))
>  		blk_queue_init_tags(sdev->request_queue, depth,
>  				    sdev->host->bqt);

Introduced by d285203:

-       if (!blk_queue_tagged(sdev->request_queue))
+       if (!shost_use_blk_mq(sdev->host) &&
+           blk_queue_tagged(sdev->request_queue))

Seems like an obvious fix.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>