On 12/20/20 6:37 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.
I'm curious. It's a "good thing" to check the command count in a better
way now, but was there any known instance of the count miscalculation in
the current code causing issues?
>
> Signed-off-by: Mike Christie <michael.christie@xxxxxxxxxx>
> ---
> drivers/scsi/libiscsi.c | 81 ++++++++++++++++++++++++++++++-------------------
> include/scsi/libiscsi.h | 2 ++
> 2 files changed, 51 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 796465e..f1ade91 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -2648,6 +2648,51 @@ 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;
> +
> + 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);
> + return -EINVAL;
> + }
> +
> + 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",
> + 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 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)
> + return -EINVAL;
> + printk(KERN_INFO "iscsi: Rounding can_queue to %d.\n",
> + total_cmds);
> + }
> +
> + scsi_cmds = total_cmds - ISCSI_MGMT_CMDS_MAX;
> + if (shost->can_queue && scsi_cmds > shost->can_queue) {
> + scsi_cmds = shost->can_queue - ISCSI_MGMT_CMDS_MAX;
> + printk(KERN_INFO "iscsi: requested cmds_max %u higher than driver limit. Using driver max %u\n",
> + requested_cmds_max, shost->can_queue);
> + }
If the device can queue, what if "can_queue" is equal to or less than
ISCSI_MGMT_CMDS_MAX?
> +
> + return scsi_cmds;
> +}
> +EXPORT_SYMBOL_GPL(iscsi_host_get_max_scsi_cmds);
> +
> /**
> * iscsi_host_add - add host to system
> * @shost: scsi host
> @@ -2800,7 +2845,7 @@ struct iscsi_cls_session *
> 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);
> @@ -2811,37 +2856,9 @@ struct iscsi_cls_session *
> 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;
Should this be "scsi_cmds <= 0" ? Having zero commands doesn't seem good.
> - }
> -
> - 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) +
> @@ -2857,7 +2874,7 @@ struct iscsi_cls_session *
> 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 44a9554..02f966e 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
>
