| Message ID | 20210118203430.4921-1-michael.christie@oracle.com |
|---|---|
| Headers | show |
| Series | iscsi fixes/cleanups | expand |
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.
On 1/27/21 1:45 PM, Lee Duncan wrote: > 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) I think INFO is good. This patch just copied the old code, but I'm not sure why I used ERR for the other cases where I fix up the value for the user. I'll fix those too like the one above this one, so we are consistent.