Re: [PATCH 5/7] libiscsi: add helper to calc max scsi cmds per session

From: Lee Duncan <lduncan@suse.com>
To: Mike Christie <michael.christie@oracle.com>,
	cleech@redhat.com, martin.petersen@oracle.com,
	linux-scsi@vger.kernel.org,
	james.bottomley@hansenpartnership.com
Cc: lutianxiong@huawei.com, linfeilong@huawei.com,
	liuzhiqiang26@huawei.com, haowenchao@huawei.com
Subject: Re: [PATCH 5/7] libiscsi: add helper to calc max scsi cmds per session
Date: Wed, 27 Jan 2021 11:45:43 -0800	[thread overview]
Message-ID: <8cccc83a-4f43-f3db-5f06-5a75f76e293e@suse.com> (raw)
In-Reply-To: <20210118203430.4921-6-michael.christie@oracle.com>

On 1/18/21 12:34 PM, Mike Christie wrote:
> This patch just breaks out the code that calculates the number
> of scsi cmds that will be used for a scsi session. It also adds
> a check that we don't go over the host's can_queue value.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/libiscsi.c | 84 +++++++++++++++++++++++++----------------
>  include/scsi/libiscsi.h |  2 +
>  2 files changed, 54 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index b271d3accd2a..195006a08e0d 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2648,6 +2648,54 @@ void iscsi_pool_free(struct iscsi_pool *q)
>  }
>  EXPORT_SYMBOL_GPL(iscsi_pool_free);
>  
> +int iscsi_host_get_max_scsi_cmds(struct Scsi_Host *shost,
> +				 uint16_t requested_cmds_max)
> +{
> +	int scsi_cmds, total_cmds = requested_cmds_max;
> +
> +check:
> +	if (!total_cmds)
> +		total_cmds = ISCSI_DEF_XMIT_CMDS_MAX;
> +	/*
> +	 * The iscsi layer needs some tasks for nop handling and tmfs,
> +	 * so the cmds_max must at least be greater than ISCSI_MGMT_CMDS_MAX
> +	 * + 1 command for scsi IO.
> +	 */
> +	if (total_cmds < ISCSI_TOTAL_CMDS_MIN) {
> +		printk(KERN_ERR "iscsi: invalid max cmds of %d. Must be a power of two that is at least %d.\n",
> +		       total_cmds, ISCSI_TOTAL_CMDS_MIN);
> +		return -EINVAL;
> +	}
> +
> +	if (total_cmds > ISCSI_TOTAL_CMDS_MAX) {
> +		printk(KERN_ERR "iscsi: invalid max cmds of %d. Must be a power of 2 less than or equal to %d.\n",
> +		       requested_cmds_max, ISCSI_TOTAL_CMDS_MAX);
> +		total_cmds = ISCSI_TOTAL_CMDS_MAX;
> +	}
> +
> +	if (!is_power_of_2(total_cmds)) {
> +		printk(KERN_ERR "iscsi: invalid max cmds of %d. Must be a power of 2.\n",
> +		       total_cmds);

I don't like mixing KERN_ERR and KERN_INFO for the same event. Can we
make it just one KERN_INFO? (since we keep going)

> +		total_cmds = rounddown_pow_of_two(total_cmds);
> +		if (total_cmds < ISCSI_TOTAL_CMDS_MIN)
> +			return -EINVAL;
> +		printk(KERN_INFO "iscsi: Rounding max cmds to %d.\n",
> +		       total_cmds);
> +	}
> +
> +	scsi_cmds = total_cmds - ISCSI_MGMT_CMDS_MAX;
> +	if (shost->can_queue && scsi_cmds > shost->can_queue) {
> +		total_cmds = shost->can_queue;
> +
> +		printk(KERN_INFO "iscsi: requested max cmds %u is higher than driver limit. Using driver limit %u\n",
> +		       requested_cmds_max, shost->can_queue);
> +		goto check;
> +	}
> +
> +	return scsi_cmds;
> +}
> +EXPORT_SYMBOL_GPL(iscsi_host_get_max_scsi_cmds);
> +
>  /**
>   * iscsi_host_add - add host to system
>   * @shost: scsi host
> @@ -2801,7 +2849,7 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
>  	struct iscsi_host *ihost = shost_priv(shost);
>  	struct iscsi_session *session;
>  	struct iscsi_cls_session *cls_session;
> -	int cmd_i, scsi_cmds, total_cmds = cmds_max;
> +	int cmd_i, scsi_cmds;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&ihost->lock, flags);
> @@ -2812,37 +2860,9 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
>  	ihost->num_sessions++;
>  	spin_unlock_irqrestore(&ihost->lock, flags);
>  
> -	if (!total_cmds)
> -		total_cmds = ISCSI_DEF_XMIT_CMDS_MAX;
> -	/*
> -	 * The iscsi layer needs some tasks for nop handling and tmfs,
> -	 * so the cmds_max must at least be greater than ISCSI_MGMT_CMDS_MAX
> -	 * + 1 command for scsi IO.
> -	 */
> -	if (total_cmds < ISCSI_TOTAL_CMDS_MIN) {
> -		printk(KERN_ERR "iscsi: invalid can_queue of %d. can_queue "
> -		       "must be a power of two that is at least %d.\n",
> -		       total_cmds, ISCSI_TOTAL_CMDS_MIN);
> +	scsi_cmds = iscsi_host_get_max_scsi_cmds(shost, cmds_max);
> +	if (scsi_cmds < 0)
>  		goto dec_session_count;
> -	}
> -
> -	if (total_cmds > ISCSI_TOTAL_CMDS_MAX) {
> -		printk(KERN_ERR "iscsi: invalid can_queue of %d. can_queue "
> -		       "must be a power of 2 less than or equal to %d.\n",
> -		       cmds_max, ISCSI_TOTAL_CMDS_MAX);
> -		total_cmds = ISCSI_TOTAL_CMDS_MAX;
> -	}
> -
> -	if (!is_power_of_2(total_cmds)) {
> -		printk(KERN_ERR "iscsi: invalid can_queue of %d. can_queue "
> -		       "must be a power of 2.\n", total_cmds);
> -		total_cmds = rounddown_pow_of_two(total_cmds);
> -		if (total_cmds < ISCSI_TOTAL_CMDS_MIN)
> -			goto dec_session_count;
> -		printk(KERN_INFO "iscsi: Rounding can_queue to %d.\n",
> -		       total_cmds);
> -	}
> -	scsi_cmds = total_cmds - ISCSI_MGMT_CMDS_MAX;
>  
>  	cls_session = iscsi_alloc_session(shost, iscsit,
>  					  sizeof(struct iscsi_session) +
> @@ -2858,7 +2878,7 @@ iscsi_session_setup(struct iscsi_transport *iscsit, struct Scsi_Host *shost,
>  	session->lu_reset_timeout = 15;
>  	session->abort_timeout = 10;
>  	session->scsi_cmds_max = scsi_cmds;
> -	session->cmds_max = total_cmds;
> +	session->cmds_max = scsi_cmds + ISCSI_MGMT_CMDS_MAX;
>  	session->queued_cmdsn = session->cmdsn = initial_cmdsn;
>  	session->exp_cmdsn = initial_cmdsn + 1;
>  	session->max_cmdsn = initial_cmdsn + 1;
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> index 44a9554aea62..02f966e9358f 100644
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -395,6 +395,8 @@ extern struct Scsi_Host *iscsi_host_alloc(struct scsi_host_template *sht,
>  extern void iscsi_host_remove(struct Scsi_Host *shost);
>  extern void iscsi_host_free(struct Scsi_Host *shost);
>  extern int iscsi_target_alloc(struct scsi_target *starget);
> +extern int iscsi_host_get_max_scsi_cmds(struct Scsi_Host *shost,
> +					uint16_t requested_cmds_max);
>  
>  /*
>   * session management
> 

If you choose to change the KERN_ERR I mentioned above, please add my
reviwed-by tag.