[PATCH 0/2] scsi/libata: A potential tagging fix and improvement

[PATCH 0/2] scsi/libata: A potential tagging fix and improvement

[John Garry]

Two loosely related patches are included:

- Fix for scsi_realloc_sdev_budget_map(). I noticed that the budget token
  for scsi commands was way in excess of the device queue depth, so I
  think we need to fix the sbitmap depth. I need to test this more.

- libata change to use scsi command budget token for qc tag for SAS host.
  I marked this as RFC as for SAS hosts I don't see anything which
  guarantees that the budget size is <= 32 always.
  For libsas hosts we resize the device depth to 32 in the slave configure
  callback, but this seems an unreliable approach since not all hosts may
  call this.
  In addition, I am worried that even if we resize the device depth
  properly in the slave config callback, we may still try to alloc qc tag
  prior to this - in lun scan, for example.
  So we need a way to guarantee that the device queue depth is <= 32
  always, which I would be open to suggestions for.

John Garry (2):
  scsi: core: Fix sbitmap depth in scsi_realloc_sdev_budget_map()
  libata: Use scsi cmnd budget token for qc tag for SAS host

 drivers/ata/libata-core.c |  5 +++--
 drivers/ata/libata-sata.c | 21 ++++-----------------
 drivers/ata/libata-scsi.c |  2 +-
 drivers/ata/libata.h      |  4 ++--
 drivers/scsi/scsi_scan.c  |  5 +++++
 include/linux/libata.h    |  1 -
 6 files changed, 15 insertions(+), 23 deletions(-)

-- 
2.26.2

[PATCH 1/2] scsi: core: Fix sbitmap depth in scsi_realloc_sdev_budget_map()

[John Garry]

In commit edb854a3680b ("scsi: core: Reallocate device's budget map on
queue depth change"), the sbitmap for the device budget map may be
reallocated after the slave device depth is configured.

When the sbitmap is reallocated we use the result from
scsi_device_max_queue_depth() for the sbitmap size, but don't resize to
match the actual device queue depth.

Fix by resizing the sbitmap after reallocating the budget sbitmap.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/scsi_scan.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f4e6c68ac99e..2ef78083f1ef 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -223,6 +223,8 @@ static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev,
 	int ret;
 	struct sbitmap sb_backup;
 
+	depth = min_t(unsigned int, depth, scsi_device_max_queue_depth(sdev));
+
 	/*
 	 * realloc if new shift is calculated, which is caused by setting
 	 * up one new default queue depth after calling ->slave_configure
@@ -245,6 +247,9 @@ static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev,
 				scsi_device_max_queue_depth(sdev),
 				new_shift, GFP_KERNEL,
 				sdev->request_queue->node, false, true);
+	if (!ret)
+		sbitmap_resize(&sdev->budget_map, depth);
+
 	if (need_free) {
 		if (ret)
 			sdev->budget_map = sb_backup;
-- 
2.26.2

Re: [PATCH 1/2] scsi: core: Fix sbitmap depth in scsi_realloc_sdev_budget_map()

[Bart Van Assche]

On 3/15/22 03:39, John Garry wrote:
> In commit edb854a3680b ("scsi: core: Reallocate device's budget map on
> queue depth change"), the sbitmap for the device budget map may be
> reallocated after the slave device depth is configured.
> 
> When the sbitmap is reallocated we use the result from
> scsi_device_max_queue_depth() for the sbitmap size, but don't resize to
> match the actual device queue depth.
> 
> Fix by resizing the sbitmap after reallocating the budget sbitmap.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/scsi_scan.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index f4e6c68ac99e..2ef78083f1ef 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -223,6 +223,8 @@ static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev,
>   	int ret;
>   	struct sbitmap sb_backup;
>   
> +	depth = min_t(unsigned int, depth, scsi_device_max_queue_depth(sdev));
> +
>   	/*
>   	 * realloc if new shift is calculated, which is caused by setting
>   	 * up one new default queue depth after calling ->slave_configure
> @@ -245,6 +247,9 @@ static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev,
>   				scsi_device_max_queue_depth(sdev),
>   				new_shift, GFP_KERNEL,
>   				sdev->request_queue->node, false, true);
> +	if (!ret)
> +		sbitmap_resize(&sdev->budget_map, depth);

Hmm ... why to call both sbitmap_init_node() and sbitmap_resize() 
instead of combining both calls into a single call with the proper depth?

Thanks,

Bart.

Re: [PATCH 1/2] scsi: core: Fix sbitmap depth in scsi_realloc_sdev_budget_map()

[John Garry]

On 15/03/2022 14:33, Bart Van Assche wrote:
>> sbitmap sb_backup;
>> +    depth = min_t(unsigned int, depth, 
>> scsi_device_max_queue_depth(sdev));
>> +
>>       /*
>>        * realloc if new shift is calculated, which is caused by setting
>>        * up one new default queue depth after calling ->slave_configure
>> @@ -245,6 +247,9 @@ static int scsi_realloc_sdev_budget_map(struct 
>> scsi_device *sdev,
>>                   scsi_device_max_queue_depth(sdev),
>>                   new_shift, GFP_KERNEL,
>>                   sdev->request_queue->node, false, true);
>> +    if (!ret)
>> +        sbitmap_resize(&sdev->budget_map, depth);
> 
> Hmm ... why to call both sbitmap_init_node() and sbitmap_resize() 
> instead of combining both calls into a single call with the proper depth?

Hi Bart,

Is the user wants to change the queue depth later via sysfs we do not 
reallocate the sbitmap then. So we need to ensure that the size we 
reallocate here will satisfy the scsi device max depth. I'm referencing 
scsi_change_queue_depth() for this.

Thanks,
John

Re: [PATCH 1/2] scsi: core: Fix sbitmap depth in scsi_realloc_sdev_budget_map()

[Ming Lei]

On Tue, Mar 15, 2022 at 06:39:05PM +0800, John Garry wrote:
> In commit edb854a3680b ("scsi: core: Reallocate device's budget map on
> queue depth change"), the sbitmap for the device budget map may be
> reallocated after the slave device depth is configured.
> 
> When the sbitmap is reallocated we use the result from
> scsi_device_max_queue_depth() for the sbitmap size, but don't resize to
> match the actual device queue depth.
> 
> Fix by resizing the sbitmap after reallocating the budget sbitmap.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/scsi_scan.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index f4e6c68ac99e..2ef78083f1ef 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -223,6 +223,8 @@ static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev,
>  	int ret;
>  	struct sbitmap sb_backup;
>  
> +	depth = min_t(unsigned int, depth, scsi_device_max_queue_depth(sdev));
> +
>  	/*
>  	 * realloc if new shift is calculated, which is caused by setting
>  	 * up one new default queue depth after calling ->slave_configure
> @@ -245,6 +247,9 @@ static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev,
>  				scsi_device_max_queue_depth(sdev),
>  				new_shift, GFP_KERNEL,
>  				sdev->request_queue->node, false, true);
> +	if (!ret)
> +		sbitmap_resize(&sdev->budget_map, depth);
> +

Looks fine, 

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming

[PATCH RFC 2/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[John Garry]

For attaining a qc tag for a SAS host we need to allocate a bit in
ata_port.sas_tag_allocated bitmap.

However we already have a unique tag per device in range
[0, ATA_MAX_QUEUE) in the scsi cmnd budget token, so just use that
instead.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/ata/libata-core.c |  5 +++--
 drivers/ata/libata-sata.c | 21 ++++-----------------
 drivers/ata/libata-scsi.c |  2 +-
 drivers/ata/libata.h      |  4 ++--
 include/linux/libata.h    |  1 -
 5 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 0c854aebfe0b..2c0a550d3ecd 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4572,8 +4572,9 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
  *	None.
  */
 
-struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
+struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd *scmd)
 {
+	int tag = scsi_cmd_to_rq(scmd)->tag;
 	struct ata_port *ap = dev->link->ap;
 	struct ata_queued_cmd *qc;
 
@@ -4583,7 +4584,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
 
 	/* libsas case */
 	if (ap->flags & ATA_FLAG_SAS_HOST) {
-		tag = ata_sas_allocate_tag(ap);
+		tag = ata_sas_allocate_tag(ap, scmd);
 		if (tag < 0)
 			return NULL;
 	}
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 071158c0c44c..a4374fdffc43 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1268,29 +1268,16 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
 }
 EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
 
-int ata_sas_allocate_tag(struct ata_port *ap)
+int ata_sas_allocate_tag(struct ata_port *ap, struct scsi_cmnd *scmd)
 {
-	unsigned int max_queue = ap->host->n_tags;
-	unsigned int i, tag;
+	if (scmd->budget_token >= ATA_MAX_QUEUE)
+		return -1;
 
-	for (i = 0, tag = ap->sas_last_tag + 1; i < max_queue; i++, tag++) {
-		tag = tag < max_queue ? tag : 0;
-
-		/* the last tag is reserved for internal command. */
-		if (ata_tag_internal(tag))
-			continue;
-
-		if (!test_and_set_bit(tag, &ap->sas_tag_allocated)) {
-			ap->sas_last_tag = tag;
-			return tag;
-		}
-	}
-	return -1;
+	return scmd->budget_token;
 }
 
 void ata_sas_free_tag(unsigned int tag, struct ata_port *ap)
 {
-	clear_bit(tag, &ap->sas_tag_allocated);
 }
 
 /**
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index ed8be585a98f..45d63a2ba3ee 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -640,7 +640,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
 {
 	struct ata_queued_cmd *qc;
 
-	qc = ata_qc_new_init(dev, scsi_cmd_to_rq(cmd)->tag);
+	qc = ata_qc_new_init(dev, cmd);
 	if (qc) {
 		qc->scsicmd = cmd;
 		qc->scsidone = scsi_done;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 51e01acdd241..65302d7829fe 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -44,7 +44,7 @@ static inline void ata_force_cbl(struct ata_port *ap) { }
 #endif
 extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
 extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf);
-extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag);
+extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd *scmd);
 extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 			   u64 block, u32 n_block, unsigned int tf_flags,
 			   unsigned int tag, int class);
@@ -93,7 +93,7 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 
 /* libata-sata.c */
 #ifdef CONFIG_SATA_HOST
-int ata_sas_allocate_tag(struct ata_port *ap);
+int ata_sas_allocate_tag(struct ata_port *ap, struct scsi_cmnd *scmd);
 void ata_sas_free_tag(unsigned int tag, struct ata_port *ap);
 #else
 static inline int ata_sas_allocate_tag(struct ata_port *ap)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7f99b4d78822..3b9399f67b39 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -814,7 +814,6 @@ struct ata_port {
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
 
 	struct ata_queued_cmd	qcmd[ATA_MAX_QUEUE + 1];
-	unsigned long		sas_tag_allocated; /* for sas tag allocation only */
 	u64			qc_active;
 	int			nr_active_links; /* #links with active qcs */
 	unsigned int		sas_last_tag;	/* track next tag hw expects */
-- 
2.26.2

Re: [PATCH RFC 2/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[Damien Le Moal]

On 3/15/22 19:39, John Garry wrote:
> For attaining a qc tag for a SAS host we need to allocate a bit in
> ata_port.sas_tag_allocated bitmap.
> 
> However we already have a unique tag per device in range
> [0, ATA_MAX_QUEUE) in the scsi cmnd budget token, so just use that
> instead.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/ata/libata-core.c |  5 +++--
>  drivers/ata/libata-sata.c | 21 ++++-----------------
>  drivers/ata/libata-scsi.c |  2 +-
>  drivers/ata/libata.h      |  4 ++--
>  include/linux/libata.h    |  1 -
>  5 files changed, 10 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 0c854aebfe0b..2c0a550d3ecd 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4572,8 +4572,9 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
>   *	None.
>   */
>  
> -struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
> +struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd *scmd)
>  {
> +	int tag = scsi_cmd_to_rq(scmd)->tag;
>  	struct ata_port *ap = dev->link->ap;
>  	struct ata_queued_cmd *qc;
>  
> @@ -4583,7 +4584,7 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
>  
>  	/* libsas case */
>  	if (ap->flags & ATA_FLAG_SAS_HOST) {
> -		tag = ata_sas_allocate_tag(ap);
> +		tag = ata_sas_allocate_tag(ap, scmd);
>  		if (tag < 0)
>  			return NULL;
>  	}
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 071158c0c44c..a4374fdffc43 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -1268,29 +1268,16 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
>  }
>  EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
>  
> -int ata_sas_allocate_tag(struct ata_port *ap)
> +int ata_sas_allocate_tag(struct ata_port *ap, struct scsi_cmnd *scmd)
>  {
> -	unsigned int max_queue = ap->host->n_tags;
> -	unsigned int i, tag;
> +	if (scmd->budget_token >= ATA_MAX_QUEUE)
> +		return -1;
>  
> -	for (i = 0, tag = ap->sas_last_tag + 1; i < max_queue; i++, tag++) {
> -		tag = tag < max_queue ? tag : 0;
> -
> -		/* the last tag is reserved for internal command. */
> -		if (ata_tag_internal(tag))
> -			continue;
> -
> -		if (!test_and_set_bit(tag, &ap->sas_tag_allocated)) {
> -			ap->sas_last_tag = tag;
> -			return tag;
> -		}
> -	}
> -	return -1;
> +	return scmd->budget_token;
>  }

Since this is now not actually allocating a tag, I would rename this
something like ata_sas_get_tag(). Or even better, simply open code this
in ata_qc_new_init() since that is the only caller.

>  
>  void ata_sas_free_tag(unsigned int tag, struct ata_port *ap)
>  {
> -	clear_bit(tag, &ap->sas_tag_allocated);
>  }

This is called only in ata_qc_free(). With this change, the function is
empty, so let's completely remove it.

>  
>  /**
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index ed8be585a98f..45d63a2ba3ee 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -640,7 +640,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
>  {
>  	struct ata_queued_cmd *qc;
>  
> -	qc = ata_qc_new_init(dev, scsi_cmd_to_rq(cmd)->tag);
> +	qc = ata_qc_new_init(dev, cmd);
>  	if (qc) {
>  		qc->scsicmd = cmd;
>  		qc->scsidone = scsi_done;
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 51e01acdd241..65302d7829fe 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -44,7 +44,7 @@ static inline void ata_force_cbl(struct ata_port *ap) { }
>  #endif
>  extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
>  extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf);
> -extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag);
> +extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd *scmd);
>  extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
>  			   u64 block, u32 n_block, unsigned int tf_flags,
>  			   unsigned int tag, int class);
> @@ -93,7 +93,7 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  
>  /* libata-sata.c */
>  #ifdef CONFIG_SATA_HOST
> -int ata_sas_allocate_tag(struct ata_port *ap);
> +int ata_sas_allocate_tag(struct ata_port *ap, struct scsi_cmnd *scmd);
>  void ata_sas_free_tag(unsigned int tag, struct ata_port *ap);
>  #else
>  static inline int ata_sas_allocate_tag(struct ata_port *ap)
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 7f99b4d78822..3b9399f67b39 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -814,7 +814,6 @@ struct ata_port {
>  	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
>  
>  	struct ata_queued_cmd	qcmd[ATA_MAX_QUEUE + 1];
> -	unsigned long		sas_tag_allocated; /* for sas tag allocation only */
>  	u64			qc_active;
>  	int			nr_active_links; /* #links with active qcs */
>  	unsigned int		sas_last_tag;	/* track next tag hw expects */


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC 2/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[John Garry]

Hi Damien,

>> -	}
>> -	return -1;
>> +	return scmd->budget_token;
>>   }
> Since this is now not actually allocating a tag, I would rename this
> something like ata_sas_get_tag(). Or even better, simply open code this
> in ata_qc_new_init() since that is the only caller.

ok, I think it might be better to open code in ata_qc_new_init(), as 
suggested. That should avoid the need for the return -1 call.

> 
>>   
>>   void ata_sas_free_tag(unsigned int tag, struct ata_port *ap)
>>   {
>> -	clear_bit(tag, &ap->sas_tag_allocated);
>>   }
> This is called only in ata_qc_free(). With this change, the function is
> empty, so let's completely remove it.
> 

ok

>>   
>>   /**
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index ed8be585a98f..45d63a2ba3ee 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -640,7 +640,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
>>   {

Thanks,
John

Re: [PATCH RFC 2/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[Christoph Hellwig]

In addition to the comments from Damien:  I think we should kill
ata_qc_new_init as well.  It is a bit pointless and having it in
libata-core.c when it pokes into scsi internals also doesn't make sense.

So maybe something like:

diff --git a/Documentation/driver-api/libata.rst b/Documentation/driver-api/libata.rst
index d477e296bda5f2..311af516a3fd9c 100644
--- a/Documentation/driver-api/libata.rst
+++ b/Documentation/driver-api/libata.rst
@@ -424,12 +424,6 @@ How commands are issued
 -----------------------
 
 Internal commands
-    First, qc is allocated and initialized using :c:func:`ata_qc_new_init`.
-    Although :c:func:`ata_qc_new_init` doesn't implement any wait or retry
-    mechanism when qc is not available, internal commands are currently
-    issued only during initialization and error recovery, so no other
-    command is active and allocation is guaranteed to succeed.
-
     Once allocated qc's taskfile is initialized for the command to be
     executed. qc currently has two mechanisms to notify completion. One
     is via ``qc->complete_fn()`` callback and the other is completion
@@ -447,11 +441,6 @@ SCSI commands
     translated. No qc is involved in processing a simulated scmd. The
     result is computed right away and the scmd is completed.
 
-    For a translated scmd, :c:func:`ata_qc_new_init` is invoked to allocate a
-    qc and the scmd is translated into the qc. SCSI midlayer's
-    completion notification function pointer is stored into
-    ``qc->scsidone``.
-
     ``qc->complete_fn()`` callback is used for completion notification. ATA
     commands use :c:func:`ata_scsi_qc_complete` while ATAPI commands use
     :c:func:`atapi_qc_complete`. Both functions end up calling ``qc->scsidone``
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 0c854aebfe0bdc..811fceb4e647fa 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4563,42 +4563,6 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
 #endif /* __BIG_ENDIAN */
 }
 
-/**
- *	ata_qc_new_init - Request an available ATA command, and initialize it
- *	@dev: Device from whom we request an available command structure
- *	@tag: tag
- *
- *	LOCKING:
- *	None.
- */
-
-struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
-{
-	struct ata_port *ap = dev->link->ap;
-	struct ata_queued_cmd *qc;
-
-	/* no command while frozen */
-	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
-		return NULL;
-
-	/* libsas case */
-	if (ap->flags & ATA_FLAG_SAS_HOST) {
-		tag = ata_sas_allocate_tag(ap);
-		if (tag < 0)
-			return NULL;
-	}
-
-	qc = __ata_qc_from_tag(ap, tag);
-	qc->tag = qc->hw_tag = tag;
-	qc->scsicmd = NULL;
-	qc->ap = ap;
-	qc->dev = dev;
-
-	ata_qc_reinit(qc);
-
-	return qc;
-}
-
 /**
  *	ata_qc_free - free unused ata_queued_cmd
  *	@qc: Command to complete
@@ -4611,19 +4575,9 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag)
  */
 void ata_qc_free(struct ata_queued_cmd *qc)
 {
-	struct ata_port *ap;
-	unsigned int tag;
-
-	WARN_ON_ONCE(qc == NULL); /* ata_qc_from_tag _might_ return NULL */
-	ap = qc->ap;
-
 	qc->flags = 0;
-	tag = qc->tag;
-	if (ata_tag_valid(tag)) {
+	if (ata_tag_valid(qc->tag))
 		qc->tag = ATA_TAG_POISON;
-		if (ap->flags & ATA_FLAG_SAS_HOST)
-			ata_sas_free_tag(tag, ap);
-	}
 }
 
 void __ata_qc_complete(struct ata_queued_cmd *qc)
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 071158c0c44c1d..13e2c0d28b6937 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1268,31 +1268,6 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
 }
 EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
 
-int ata_sas_allocate_tag(struct ata_port *ap)
-{
-	unsigned int max_queue = ap->host->n_tags;
-	unsigned int i, tag;
-
-	for (i = 0, tag = ap->sas_last_tag + 1; i < max_queue; i++, tag++) {
-		tag = tag < max_queue ? tag : 0;
-
-		/* the last tag is reserved for internal command. */
-		if (ata_tag_internal(tag))
-			continue;
-
-		if (!test_and_set_bit(tag, &ap->sas_tag_allocated)) {
-			ap->sas_last_tag = tag;
-			return tag;
-		}
-	}
-	return -1;
-}
-
-void ata_sas_free_tag(unsigned int tag, struct ata_port *ap)
-{
-	clear_bit(tag, &ap->sas_tag_allocated);
-}
-
 /**
  *	sata_async_notification - SATA async notification handler
  *	@ap: ATA port where async notification is received
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index ed8be585a98f7c..5e0bc7b05a107e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -638,24 +638,44 @@ EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
 static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
 					      struct scsi_cmnd *cmd)
 {
+	struct ata_port *ap = dev->link->ap;
 	struct ata_queued_cmd *qc;
+	int tag;
 
-	qc = ata_qc_new_init(dev, scsi_cmd_to_rq(cmd)->tag);
-	if (qc) {
-		qc->scsicmd = cmd;
-		qc->scsidone = scsi_done;
-
-		qc->sg = scsi_sglist(cmd);
-		qc->n_elem = scsi_sg_count(cmd);
+	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
+		goto fail;
 
-		if (scsi_cmd_to_rq(cmd)->rq_flags & RQF_QUIET)
-			qc->flags |= ATA_QCFLAG_QUIET;
+	if (ap->flags & ATA_FLAG_SAS_HOST) {
+		/* XXX: add a comment here why SAS is different */
+		if (WARN_ON_ONCE(cmd->budget_token >= ATA_MAX_QUEUE))
+			goto fail;
+		tag = cmd->budget_token;
 	} else {
-		cmd->result = (DID_OK << 16) | SAM_STAT_TASK_SET_FULL;
-		scsi_done(cmd);
+		tag = scsi_cmd_to_rq(cmd)->tag;
 	}
 
+	qc = __ata_qc_from_tag(ap, tag);
+	qc->tag = qc->hw_tag = tag;
+	qc->scsicmd = NULL;
+	qc->ap = ap;
+	qc->dev = dev;
+
+	ata_qc_reinit(qc);
+
+	qc->scsicmd = cmd;
+	qc->scsidone = scsi_done;
+
+	qc->sg = scsi_sglist(cmd);
+	qc->n_elem = scsi_sg_count(cmd);
+
+	if (scsi_cmd_to_rq(cmd)->rq_flags & RQF_QUIET)
+		qc->flags |= ATA_QCFLAG_QUIET;
 	return qc;
+
+fail:
+	cmd->result = (DID_OK << 16) | SAM_STAT_TASK_SET_FULL;
+	scsi_done(cmd);
+	return NULL;
 }
 
 static void ata_qc_set_pc_nbytes(struct ata_queued_cmd *qc)
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 51e01acdd24107..191f8bfe038656 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -44,7 +44,6 @@ static inline void ata_force_cbl(struct ata_port *ap) { }
 #endif
 extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
 extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf);
-extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag);
 extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
 			   u64 block, u32 n_block, unsigned int tf_flags,
 			   unsigned int tag, int class);
@@ -91,18 +90,6 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 
 #define to_ata_port(d) container_of(d, struct ata_port, tdev)
 
-/* libata-sata.c */
-#ifdef CONFIG_SATA_HOST
-int ata_sas_allocate_tag(struct ata_port *ap);
-void ata_sas_free_tag(unsigned int tag, struct ata_port *ap);
-#else
-static inline int ata_sas_allocate_tag(struct ata_port *ap)
-{
-	return -EOPNOTSUPP;
-}
-static inline void ata_sas_free_tag(unsigned int tag, struct ata_port *ap) { }
-#endif
-
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI
 extern unsigned int ata_acpi_gtf_filter;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 7f99b4d788223b..3b9399f67b3902 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -814,7 +814,6 @@ struct ata_port {
 	unsigned int		cbl;	/* cable type; ATA_CBL_xxx */
 
 	struct ata_queued_cmd	qcmd[ATA_MAX_QUEUE + 1];
-	unsigned long		sas_tag_allocated; /* for sas tag allocation only */
 	u64			qc_active;
 	int			nr_active_links; /* #links with active qcs */
 	unsigned int		sas_last_tag;	/* track next tag hw expects */

Re: [PATCH RFC 2/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[John Garry]

On 16/03/2022 08:34, Christoph Hellwig wrote:
> In addition to the comments from Damien:  I think we should kill
> ata_qc_new_init as well.  It is a bit pointless and having it in
> libata-core.c when it pokes into scsi internals also doesn't make sense.
> 
> So maybe something like:

Seems reasonable to me at least.

But I'd send these as 2x separate patches.

>    *	sata_async_notification - SATA async notification handler
>    *	@ap: ATA port where async notification is received
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index ed8be585a98f7c..5e0bc7b05a107e 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -638,24 +638,44 @@ EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
>   static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
>   					      struct scsi_cmnd *cmd)
>   {
...

>   
> -		if (scsi_cmd_to_rq(cmd)->rq_flags & RQF_QUIET)
> -			qc->flags |= ATA_QCFLAG_QUIET;
> +	if (ap->flags & ATA_FLAG_SAS_HOST) {
> +		/* XXX: add a comment here why SAS is different */

I think this is simply because SAS hosts have shost.can_queue > 32

> +		if (WARN_ON_ONCE(cmd->budget_token >= ATA_MAX_QUEUE))
> +			goto fail;
> +		tag = cmd->budget_token;

thanks,
John

Re: [PATCH RFC 2/2] libata: Use scsi cmnd budget token for qc tag for SAS host

[Damien Le Moal]

On 3/16/22 19:46, John Garry wrote:
> On 16/03/2022 08:34, Christoph Hellwig wrote:
>> In addition to the comments from Damien:  I think we should kill
>> ata_qc_new_init as well.  It is a bit pointless and having it in
>> libata-core.c when it pokes into scsi internals also doesn't make sense.
>>
>> So maybe something like:
> 
> Seems reasonable to me at least.

Yep, that will be a nice cleanup.

> 
> But I'd send these as 2x separate patches.
> 
>>    *	sata_async_notification - SATA async notification handler
>>    *	@ap: ATA port where async notification is received
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index ed8be585a98f7c..5e0bc7b05a107e 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -638,24 +638,44 @@ EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
>>   static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
>>   					      struct scsi_cmnd *cmd)
>>   {
> ...
> 
>>   
>> -		if (scsi_cmd_to_rq(cmd)->rq_flags & RQF_QUIET)
>> -			qc->flags |= ATA_QCFLAG_QUIET;
>> +	if (ap->flags & ATA_FLAG_SAS_HOST) {
>> +		/* XXX: add a comment here why SAS is different */
> 
> I think this is simply because SAS hosts have shost.can_queue > 32
> 
>> +		if (WARN_ON_ONCE(cmd->budget_token >= ATA_MAX_QUEUE))
>> +			goto fail;
>> +		tag = cmd->budget_token;
> 
> thanks,
> John


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/2] scsi/libata: A potential tagging fix and improvement

[Damien Le Moal]

On 3/15/22 19:39, John Garry wrote:
> Two loosely related patches are included:
> 
> - Fix for scsi_realloc_sdev_budget_map(). I noticed that the budget token
>   for scsi commands was way in excess of the device queue depth, so I
>   think we need to fix the sbitmap depth. I need to test this more.
> 
> - libata change to use scsi command budget token for qc tag for SAS host.
>   I marked this as RFC as for SAS hosts I don't see anything which
>   guarantees that the budget size is <= 32 always.
>   For libsas hosts we resize the device depth to 32 in the slave configure
>   callback, but this seems an unreliable approach since not all hosts may
>   call this.
>   In addition, I am worried that even if we resize the device depth
>   properly in the slave config callback, we may still try to alloc qc tag
>   prior to this - in lun scan, for example.
>   So we need a way to guarantee that the device queue depth is <= 32
>   always, which I would be open to suggestions for.
> 
> John Garry (2):
>   scsi: core: Fix sbitmap depth in scsi_realloc_sdev_budget_map()
>   libata: Use scsi cmnd budget token for qc tag for SAS host
> 
>  drivers/ata/libata-core.c |  5 +++--
>  drivers/ata/libata-sata.c | 21 ++++-----------------
>  drivers/ata/libata-scsi.c |  2 +-
>  drivers/ata/libata.h      |  4 ++--
>  drivers/scsi/scsi_scan.c  |  5 +++++
>  include/linux/libata.h    |  1 -
>  6 files changed, 15 insertions(+), 23 deletions(-)
> 

I tested this and it is working fine for me. This actually solves the QD
not changing problem I had detected with the pm80xx driver.
Now, doing this:

# cat /sys/block/sde/device/queue_depth
32
# echo 16 > /sys/block/sde/device/queue_depth
# cat /sys/block/sde/device/queue_depth
16

is working as expected.

See my comments on patch 2 for getting final ack and tested tags :)

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/2] scsi/libata: A potential tagging fix and improvement

[John Garry]

On 16/03/2022 03:25, Damien Le Moal wrote:s

Hi Damien,

> I tested this and it is working fine for me. This actually solves the QD
> not changing problem I had detected with the pm80xx driver.
> Now, doing this:
> 
> # cat /sys/block/sde/device/queue_depth
> 32
> # echo 16 > /sys/block/sde/device/queue_depth
> # cat /sys/block/sde/device/queue_depth
> 16
> 

Having this working is down to the first patch. So I will resend that 
patch today separately so that we may look to have it included in 5.18, 
even though we're so late in the cycle...

> is working as expected.
> 
> See my comments on patch 2 for getting final ack and tested tags:)

OK, Thanks,
John