| Message ID | 20180627080345.27500-1-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
On 6/27/2018 11:03 AM, Sagi Grimberg wrote: > We need to set can_queue earlier than when enabling the scsi host. > in a blk-mq enabled environment, the tagset allocation is taken > from can_queue which cannot be modified later. Also, pass an updated > .can_queue to iscsi_session_setup to have enough iscsi tasks allocated > in the session kfifo. > > Reported-by: Karandeep Chahal <karandeepchahal@gmail.com> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > index 9a6434c31db2..61cc47da2fec 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -633,8 +633,8 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, > */ > if (ep) { > iser_conn = ep->dd_data; > - max_cmds = iser_conn->max_cmds; > shost->sg_tablesize = iser_conn->scsi_sg_tablesize; > + shost->can_queue = min_t(u16, cmds_max, iser_conn->max_cmds); can we move it up before the if/else and set it once ? > > mutex_lock(&iser_conn->state_mutex); > if (iser_conn->state != ISER_CONN_UP) { > @@ -660,6 +660,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, > } > mutex_unlock(&iser_conn->state_mutex); > } else { > + shost->can_queue = min_t(u16, cmds_max, ISER_DEF_XMIT_CMDS_MAX); > max_cmds = ISER_DEF_XMIT_CMDS_MAX; > if (iscsi_host_add(shost, NULL)) > goto free_host; > @@ -676,21 +677,14 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, > iser_warn("max_sectors was reduced from %u to %u\n", > iser_max_sectors, shost->max_sectors); > > - if (cmds_max > max_cmds) { > - iser_info("cmds_max changed from %u to %u\n", > - cmds_max, max_cmds); > - cmds_max = max_cmds; > - } > - > cls_session = iscsi_session_setup(&iscsi_iser_transport, shost, > - cmds_max, 0, > + shost->can_queue, 0, > sizeof(struct iscsi_iser_task), > initial_cmdsn, 0); > if (!cls_session) > goto remove_host; > session = cls_session->dd_data; > > - shost->can_queue = session->scsi_cmds_max; > return cls_session; > > remove_host: > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c >> b/drivers/infiniband/ulp/iser/iscsi_iser.c >> index 9a6434c31db2..61cc47da2fec 100644 >> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >> @@ -633,8 +633,8 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, >> */ >> if (ep) { >> iser_conn = ep->dd_data; >> - max_cmds = iser_conn->max_cmds; >> shost->sg_tablesize = iser_conn->scsi_sg_tablesize; >> + shost->can_queue = min_t(u16, cmds_max, iser_conn->max_cmds); > > can we move it up before the if/else and set it once ? The different conditions has different min arguments... Its really old tools support, maybe we can simply drop it instead of letting it rot forever? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/28/2018 5:42 PM, Sagi Grimberg wrote: > >>> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c >>> b/drivers/infiniband/ulp/iser/iscsi_iser.c >>> index 9a6434c31db2..61cc47da2fec 100644 >>> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >>> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >>> @@ -633,8 +633,8 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, >>> */ >>> if (ep) { >>> iser_conn = ep->dd_data; >>> - max_cmds = iser_conn->max_cmds; >>> shost->sg_tablesize = iser_conn->scsi_sg_tablesize; >>> + shost->can_queue = min_t(u16, cmds_max, iser_conn->max_cmds); >> >> can we move it up before the if/else and set it once ? > > The different conditions has different min arguments... Its really old > tools support, maybe we can simply drop it instead of letting it rot > forever? ohh I didn't see the different arguments.. I think we can keep it meanwhile to avoid from getting complains regarding competability. code looks fine, Reviewed-by: Max Gurtovoy <maxg@mellanox.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 27, 2018 at 11:03:45AM +0300, Sagi Grimberg wrote: > We need to set can_queue earlier than when enabling the scsi host. > in a blk-mq enabled environment, the tagset allocation is taken > from can_queue which cannot be modified later. Also, pass an updated > .can_queue to iscsi_session_setup to have enough iscsi tasks allocated > in the session kfifo. > > Reported-by: Karandeep Chahal <karandeepchahal@gmail.com> > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > Reviewed-by: Max Gurtovoy <maxg@mellanox.com> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) Applied to for-next, thanks Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index 9a6434c31db2..61cc47da2fec 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -633,8 +633,8 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, */ if (ep) { iser_conn = ep->dd_data; - max_cmds = iser_conn->max_cmds; shost->sg_tablesize = iser_conn->scsi_sg_tablesize; + shost->can_queue = min_t(u16, cmds_max, iser_conn->max_cmds); mutex_lock(&iser_conn->state_mutex); if (iser_conn->state != ISER_CONN_UP) { @@ -660,6 +660,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, } mutex_unlock(&iser_conn->state_mutex); } else { + shost->can_queue = min_t(u16, cmds_max, ISER_DEF_XMIT_CMDS_MAX); max_cmds = ISER_DEF_XMIT_CMDS_MAX; if (iscsi_host_add(shost, NULL)) goto free_host; @@ -676,21 +677,14 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep, iser_warn("max_sectors was reduced from %u to %u\n", iser_max_sectors, shost->max_sectors); - if (cmds_max > max_cmds) { - iser_info("cmds_max changed from %u to %u\n", - cmds_max, max_cmds); - cmds_max = max_cmds; - } - cls_session = iscsi_session_setup(&iscsi_iser_transport, shost, - cmds_max, 0, + shost->can_queue, 0, sizeof(struct iscsi_iser_task), initial_cmdsn, 0); if (!cls_session) goto remove_host; session = cls_session->dd_data; - shost->can_queue = session->scsi_cmds_max; return cls_session; remove_host:
We need to set can_queue earlier than when enabling the scsi host. in a blk-mq enabled environment, the tagset allocation is taken from can_queue which cannot be modified later. Also, pass an updated .can_queue to iscsi_session_setup to have enough iscsi tasks allocated in the session kfifo. Reported-by: Karandeep Chahal <karandeepchahal@gmail.com> Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- drivers/infiniband/ulp/iser/iscsi_iser.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)