Fix an infinite loop in the SRP initiator
Subsystems (2)
cover Fix an infinite loop in the SRP initiator
Submitting a SCSI request through the SG_IO mechanism with a scatterlist that is longer than what is supported by the SRP initiator triggers an infinite loop. This patch series fixes that behavior.
The individual patches in this series are as follows:
0001-IB-srp-Fix-a-spelling-error.patch 0002-IB-srp-Document-srp_map_data-return-value.patch 0003-IB-srp-Rename-work-request-ID-labels.patch 0004-IB-srp-Fix-a-potential-queue-overflow-in-an-error-pa.patch 0005-IB-srp-Fix-srp_map_data-error-paths.patch 0006-IB-srp-Introduce-target-mr_pool_size.patch 0007-IB-srp-Avoid-that-mapping-failure-triggers-an-infini.patch --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/7 IB/srp: Fix a spelling error
--- drivers/infiniband/ulp/srp/ib_srp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/ulp/srp/ib_srp.c +1 -1
@@ -1407,7 +1407,7 @@ static int srp_map_sg_entry(struct srp_map_state *state,/* * If the last entry of the MR wasn't a full page, then we need to * close it out and start a new one -- we can only merge at page -* boundries. +* boundaries. */ ret =0 ;if (len != dev->mr_page_size)
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sagi Grimberg
Reviewed-byRe: [PATCH 1/7] IB/srp: Fix a spelling error
Looks good,
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/7 IB/srp: Document srp_map_data() return value
In reply to a message not in the archive--- drivers/infiniband/ulp/srp/ib_srp.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/infiniband/ulp/srp/ib_srp.c +9 -0
@@ -1497,6 +1497,15 @@ out:return ret; } +/** +* srp_map_data() - map SCSI data buffer onto an SRP request +* @scmnd: SCSI command to map +* @ch: SRP RDMA channel +* @req: SRP request +* +* Returns the length in bytes of the SRP_CMD IU or a negative value if +* mapping failed. +*/ static int srp_map_data (struct scsi_cmnd *scmnd,struct srp_rdma_ch *ch,struct srp_request *req) {
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sagi Grimberg
Reviewed-byRe: [PATCH 2/7] IB/srp: Document srp_map_data() return value
Looks good,
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/7 IB/srp: Rename work request ID labels
In reply to a message not in the archiveWork request IDs in the SRP initiator driver are either a pointer or a value that is not a valid pointer. Since the local invalidate and fast registration work requests IDs are not used as masks drop the suffix "mask" from their name.
--- drivers/infiniband/ulp/srp/ib_srp.c | 8 ++++---- drivers/infiniband/ulp/srp/ib_srp.h | 7 +++---- 2 files changed, 7 insertions(+), 8 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +4 -4
@@ -1049,7 +1049,7 @@ static int srp_inv_rkey(struct srp_rdma_ch *ch, u32 rkey)struct ib_send_wr *bad_wr;struct ib_send_wr wr = { .opcode =IB_WR_LOCAL_INV , - .wr_id =LOCAL_INV_WR_ID_MASK , + .wr_id =LOCAL_INV_WR_ID , .next =NULL , .num_sge =0 , .send_flags =0 ,
@@ -1325,7 +1325,7 @@ static int srp_map_finish_fr(struct srp_map_state *state,memset (&wr,0 ,sizeof (wr)); wr.opcode =IB_WR_FAST_REG_MR ; - wr.wr_id =FAST_REG_WR_ID_MASK ; + wr.wr_id =FAST_REG_WR_ID ; wr.wr.fast_reg.iova_start = state->base_dma_addr; wr.wr.fast_reg.page_list = desc->frpl; wr.wr.fast_reg.page_list_len = state->npages;
@@ -1940,11 +1940,11 @@ static void srp_handle_qp_err(u64 wr_id, enum ib_wc_status wc_status, }if (ch->connected && !target->qp_in_error) { -if (wr_id &LOCAL_INV_WR_ID_MASK ) { +if (wr_id ==LOCAL_INV_WR_ID ) {shost_printk (KERN_ERR , target->scsi_host,PFX "LOCAL_INV failed with status %s (%d)\n" ,ib_wc_status_msg (wc_status), wc_status); - }else if (wr_id &FAST_REG_WR_ID_MASK ) { + }else if (wr_id ==FAST_REG_WR_ID ) {shost_printk (KERN_ERR , target->scsi_host,PFX "FAST_REG_MR failed status %s (%d)\n" ,ib_wc_status_msg (wc_status), wc_status);
drivers/infiniband/ulp/srp/ib_srp.h +3 -4
@@ -67,10 +67,9 @@ enum {SRP_MAX_PAGES_PER_MR =512 , -LOCAL_INV_WR_ID_MASK =1 , -FAST_REG_WR_ID_MASK =2 , - - SRP_LAST_WR_ID = 0xfffffffcU, +LOCAL_INV_WR_ID =1 , +FAST_REG_WR_ID =2 , + SRP_LAST_WR_ID = 3, }; enum srp_target_state {
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sagi Grimberg
Reviewed-byRe: [PATCH 3/7] IB/srp: Rename work request ID labels
Looks good,
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 4/7 IB/srp: Fix a potential queue overflow in an error path
In reply to a message not in the archiveWait until memory registration has finished in the srp_queuecommand() error path before invalidating memory regions to avoid a send queue overflow.
--- drivers/infiniband/ulp/srp/ib_srp.c | 41 ++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +34 -7
@@ -1044,7 +1044,7 @@ static int srp_connect_ch(struct srp_rdma_ch *ch, bool multich) } } -static int srp_inv_rkey (struct srp_rdma_ch *ch,u32 rkey) +static int srp_inv_rkey (struct srp_rdma_ch *ch,u32 rkey,u32 send_flags) {struct ib_send_wr *bad_wr;struct ib_send_wr wr = {
@@ -1052,16 +1052,32 @@ static int srp_inv_rkey(struct srp_rdma_ch *ch, u32 rkey) .wr_id = LOCAL_INV_WR_ID, .next = NULL, .num_sge = 0, - .send_flags = 0, + .send_flags = send_flags, .ex.invalidate_rkey = rkey, };return ib_post_send (ch->qp, &wr, &bad_wr); } +static bool srp_wait_until_done (struct srp_rdma_ch *ch,int i,long timeout) +{ +WARN_ON_ONCE (timeout <=0 ); + +for ( ; i >0 ; i--) { +spin_lock_irq (&ch->lock); +srp_send_completion (ch->send_cq, ch); +spin_unlock_irq (&ch->lock); + +if (wait_for_completion_timeout (&ch->done, timeout) >0 ) +return true; + } +return false; +} +static void srp_unmap_data (struct scsi_cmnd *scmnd,struct srp_rdma_ch *ch, -struct srp_request *req) +struct srp_request *req, +bool wait_for_first_unmap) {struct srp_target_port *target = ch->target;struct srp_device *dev = target->srp_host->srp_dev;
@@ -1077,13 +1093,19 @@ static void srp_unmap_data(struct scsi_cmnd *scmnd,struct srp_fr_desc **pfr;for (i = req->nmdesc, pfr = req->fr_list; i >0 ; i--, pfr++) { - res =srp_inv_rkey (ch, (*pfr)->mr->rkey); + res =srp_inv_rkey (ch, (*pfr)->mr->rkey, + wait_for_first_unmap ? +IB_SEND_SIGNALED :0 );if (res <0 ) {shost_printk (KERN_ERR , target->scsi_host,PFX "Queueing INV WR for rkey %#x failed (%d)\n" , (*pfr)->mr->rkey, res);queue_work (system_long_wq, &target->tl_err_work); + }else if (wait_for_first_unmap) { + wait_for_first_unmap = false; +WARN_ON_ONCE (!srp_wait_until_done (ch,10 , +msecs_to_jiffies (100 ))); } }if (req->nmdesc)
@@ -1144,7 +1166,7 @@ static void srp_free_req(struct srp_rdma_ch *ch, struct srp_request *req, {unsigned long flags; -srp_unmap_data (scmnd, ch, req); +srp_unmap_data (scmnd, ch, req, false);spin_lock_irqsave (&ch->lock, flags); ch->req_lim += req_lim_delta;
@@ -1982,7 +2004,12 @@ static void srp_send_completion(struct ib_cq *cq, void *ch_ptr)struct srp_iu *iu;while (ib_poll_cq (cq,1 , &wc) >0 ) { -if (likely (wc.status ==IB_WC_SUCCESS )) { +if (unlikely (wc.wr_id ==LOCAL_INV_WR_ID )) { +complete (&ch->done); +if (wc.status !=IB_WC_SUCCESS ) +srp_handle_qp_err (wc.wr_id, wc.status, true, + ch); + }else if (likely (wc.status ==IB_WC_SUCCESS )) { iu = (struct srp_iu *) (uintptr_t ) wc.wr_id;list_add (&iu->list, &ch->free_tx); }else {
@@ -2084,7 +2111,7 @@ unlock_rport:return ret; err_unmap: -srp_unmap_data (scmnd, ch, req); +srp_unmap_data (scmnd, ch, req, true); err_iu:srp_put_tx_iu (ch, iu,SRP_IU_CMD );
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 4/7] IB/srp: Fix a potential queue overflow in an error path
Quoted 3 lines · Bart Van Assche
Wait until memory registration has finished in the
srp_queuecommand()error path before invalidating memory regions to avoid a send queue overflow.
This looks backwards to me... Why do we even post anything on our queue-pair to begin with if we got an unsupported sg list?
Can't we perform a simple sanity check on the sg list instead? --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 5638F08D.9070206-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org
Re: [PATCH 4/7] IB/srp: Fix a potential queue overflow in an error path
Quoted 9 lines · Sagi Grimberg
Quoted 3 lines · Bart Van Assche
Wait until memory registration has finished in the
srp_queuecommand()error path before invalidating memory regions to avoid a send queue overflow.This looks backwards to me... Why do we even post anything on our queue-pair to begin with if we got an unsupported sg list?
Can't we perform a simple sanity check on the sg list instead?
Hello Sagi,
There is one memory descriptor pool per RDMA channel and RDMA channels are typically used by more than one CPU. This means that memory registration for more than one command can happen concurrently from a single memory descriptor pool. This is why checking how many memory descriptors are left before memory registration occurs wouldn't be sufficient.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 4/7] IB/srp: Fix a potential queue overflow in an error path
Quoted 13 lines · Bart Van Assche
Quoted 9 lines · Sagi Grimberg
Quoted 3 lines · Bart Van Assche
Wait until memory registration has finished in the
srp_queuecommand()error path before invalidating memory regions to avoid a send queue overflow.This looks backwards to me... Why do we even post anything on our queue-pair to begin with if we got an unsupported sg list?
Can't we perform a simple sanity check on the sg list instead?
Hello Sagi,
Quoted 7 lines
There is one memory descriptor pool per RDMA channel and RDMA channels are typically used by more than one CPU. This means that memory registration for more than one command can happen concurrently from a single memory descriptor pool.
This is why checking how many memory
descriptors are left before memory registration occurs wouldn't be sufficient.
How harm would it do to make the entire SG list mapping channel-wide atomic? I'm asking because it's really a non-trivial flow you're introducing.
Sagi.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 56391181.8040207-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org
Re: [PATCH 4/7] IB/srp: Fix a potential queue overflow in an error path
Quoted 22 lines · Sagi Grimberg
Quoted 13 lines · Bart Van Assche
Quoted 9 lines · Sagi Grimberg
Quoted 3 lines · Bart Van Assche
Wait until memory registration has finished in the
srp_queuecommand()error path before invalidating memory regions to avoid a send queue overflow.This looks backwards to me... Why do we even post anything on our queue-pair to begin with if we got an unsupported sg list?
Can't we perform a simple sanity check on the sg list instead?
Hello Sagi,
Quoted 7 lines
There is one memory descriptor pool per RDMA channel and RDMA channels are typically used by more than one CPU. This means that memory registration for more than one command can happen concurrently from a single memory descriptor pool.
This is why checking how many memory
descriptors are left before memory registration occurs wouldn't be sufficient.
Quoted 3 lines
How harm would it do to make the entire SG list mapping channel-wide atomic? I'm asking because it's really a non-trivial flow you're introducing.
Hello Sagi,
Sorry but I strongly prefer not to introduce new contention points in the SRP initiator driver.
Thanks,
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 4/7] IB/srp: Fix a potential queue overflow in an error path
Quoted 5 lines
Hello Sagi,
Sorry but I strongly prefer not to introduce new contention points in the SRP initiator driver.
Yea... Me neither.
What if you just have a simple check on the SG list and reserve the required descriptors up front? Would that work?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 5639157C.2060107-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org
Re: [PATCH 4/7] IB/srp: Fix a potential queue overflow in an error path
Quoted 7 lines · Sagi Grimberg
Quoted 2 lines
Sorry but I strongly prefer not to introduce new contention points in the SRP initiator driver.
Yea... Me neither.
What if you just have a simple check on the SG list and reserve the required descriptors up front? Would that work?
Such a check wouldn't be that simple because the only way to perform such a check is either by doubling the number of ib_map_mr_sg() calls or by performing additional memory allocations.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 4/7] IB/srp: Fix a potential queue overflow in an error path
Quoted 3 lines · Bart Van Assche
Such a check wouldn't be that simple because the only way to perform such a check is either by doubling the number of
ib_map_mr_sg()calls or by performing additional memory allocations.
The other option woud be to disallow gappy SG lists unless supported by the hardware with a single MR similar to iSER. While this would leave the SRP protocol support for multiple SG entries per command unused it would significantly simplify the driver.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 4/7] IB/srp: Fix a potential queue overflow in an error path
Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Bart Van Assche
Such a check wouldn't be that simple because the only way to perform such a check is either by doubling the number of
ib_map_mr_sg()calls or by performing additional memory allocations.The other option woud be to disallow gappy SG lists unless supported by the hardware with a single MR similar to iSER. While this would leave the SRP protocol support for multiple SG entries per command unused it would significantly simplify the driver.
Hello Christoph,
I will have a look into this after the kernel 4.4 merge window has been closed.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 5/7 IB/srp: Fix srp_map_data() error paths
In reply to a message not in the archiveEnsure that req->nmdesc is set correctly in srp_map_sg() if mapping fails. Avoid that mapping failure causes a memory descriptor leak. Report srp_map_sg() failure to the caller.
--- drivers/infiniband/ulp/srp/ib_srp.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +11 -4
@@ -1473,7 +1473,6 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch, } } - req->nmdesc = state->nmdesc; ret =0 ; out:
@@ -1594,7 +1593,10 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch, target->indirect_size, DMA_TO_DEVICE);memset (&state,0 ,sizeof (state)); -srp_map_sg (&state, ch, req, scat, count); + ret =srp_map_sg (&state, ch, req, scat, count); + req->nmdesc = state.nmdesc; +if (ret <0 ) + goto unmap;/* We've mapped the request, now pull as much of the indirect * descriptor table as we can into the command buffer. If this
@@ -1617,7 +1619,8 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch, !target->allow_ext_sg)) {shost_printk (KERN_ERR , target->scsi_host,"Could not fit S/G list into SRP_CMD\n" ); -return -EIO ; + ret = -EIO ; + goto unmap; } count =min (state.ndesc, target->cmd_sg_cnt);
@@ -1635,7 +1638,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch, ret =srp_map_idb (ch, req, state.gen.next, state.gen.end, idb_len, &idb_rkey);if (ret <0 ) -return ret; + goto unmap; req->nmdesc++; }else { idb_rkey = target->global_mr->rkey;
@@ -1661,6 +1664,10 @@ map_complete: cmd->buf_fmt = fmt;return len; + +unmap: +srp_unmap_data (scmnd, ch, req, true); +return ret; }/*
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 6/7 IB/srp: Introduce target->mr_pool_size
In reply to a message not in the archiveThis patch does not change any functionality.
--- drivers/infiniband/ulp/srp/ib_srp.c | 6 +++--- drivers/infiniband/ulp/srp/ib_srp.h | 1 + 2 files changed, 4 insertions(+), 3 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +3 -3
@@ -315,7 +315,7 @@ static struct ib_fmr_pool *srp_alloc_fmr_pool(struct srp_target_port *target)struct ib_fmr_pool_param fmr_param;memset (&fmr_param,0 ,sizeof (fmr_param)); - fmr_param.pool_size = target->scsi_host->can_queue; + fmr_param.pool_size = target->mr_pool_size; fmr_param.dirty_watermark = fmr_param.pool_size /4 ; fmr_param.cache =1 ; fmr_param.max_pages_per_fmr = dev->max_pages_per_mr;
@@ -449,8 +449,7 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target) {struct srp_device *dev = target->srp_host->srp_dev; -return srp_create_fr_pool (dev->dev, dev->pd, - target->scsi_host->can_queue, +return srp_create_fr_pool (dev->dev, dev->pd, target->mr_pool_size, dev->max_pages_per_mr); }
@@ -3247,6 +3246,7 @@ static ssize_t srp_create_target(struct device *dev, } target_host->sg_tablesize = target->sg_tablesize; + target->mr_pool_size = target->scsi_host->can_queue; target->indirect_size = target->sg_tablesize *sizeof (struct srp_direct_buf ); target->max_iu_len = sizeof (struct srp_cmd) +
drivers/infiniband/ulp/srp/ib_srp.h +1 -0
@@ -205,6 +205,7 @@ struct srp_target_port {char target_name[32 ];unsigned int scsi_id;unsigned int sg_tablesize; +int mr_pool_size;int queue_size;int req_ring_size;int comp_vector;
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Sagi Grimberg
Reviewed-byRe: [PATCH 6/7] IB/srp: Introduce target->mr_pool_size
Looks good,
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 7/7 IB/srp: Avoid that mapping failure triggers an infinite loop
In reply to a message not in the archive--- drivers/infiniband/ulp/srp/ib_srp.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/infiniband/ulp/srp/ib_srp.c +2 -0
@@ -1666,6 +1666,8 @@ map_complete: unmap:srp_unmap_data (scmnd, ch, req, true); +if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size) + ret = -E2BIG ;return ret; }
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop
Can you spare a few words on this change in the change log?
Quoted 12 lines
Signed-off-by Bart Van Assche <[email protected]>Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Sebastian Parschauer <[email protected]>--- drivers/infiniband/ulp/srp/ib_srp.c | 2 ++ 1 file changed, 2 insertions(+)drivers/infiniband/ulp/srp/ib_srp.c +0
-0
drivers/infiniband/ulp/srp/ib_srp.c map_complete: · line 1672
Quoted 7 lines
drivers/infiniband/ulp/srp/ib_srp.c +2
-0@@ -1666,6 +1666,8 @@ map_complete: unmap:srp_unmap_data (scmnd, ch, req, true); +if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size) + ret = -E2BIG ;return ret; }
Why return E2BIG for ENOMEM as well?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop
Quoted 16 lines · Sagi Grimberg
Can you spare a few words on this change in the change log?
Quoted 13 lines
Signed-off-by Bart Van Assche <[email protected]>Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Sebastian Parschauer <[email protected]>--- drivers/infiniband/ulp/srp/ib_srp.c | 2 ++ 1 file changed, 2 insertions(+)drivers/infiniband/ulp/srp/ib_srp.c +0
-0@@ -1666,6 +1666,8 @@ map_complete:
Quoted 8 lines
drivers/infiniband/ulp/srp/ib_srp.c map_complete: · line 1672
Quoted 6 lines
drivers/infiniband/ulp/srp/ib_srp.c +2
-0unmap:srp_unmap_data (scmnd, ch, req, true); +if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size) + ret = -E2BIG ;return ret; }Why return E2BIG for ENOMEM as well?
Hello Sagi,
The srp_queuecommand() function translates ENOMEM into QUEUE_FULL which causes the SCSI mid-layer to retry the command. All other error codes are translated into DID_ERROR which causes the SCSI command to fail. This is why this patch fixes an infinite loop. I will add this explanation to the patch description when I repost this patch.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop
Quoted 27 lines · Bart Van Assche
Quoted 17 lines · Sagi Grimberg
Can you spare a few words on this change in the change log?
Quoted 14 lines
Signed-off-by Bart Van Assche <[email protected]>Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Sebastian Parschauer <[email protected]>--- drivers/infiniband/ulp/srp/ib_srp.c | 2 ++ 1 file changed, 2 insertions(+)drivers/infiniband/ulp/srp/ib_srp.c +0
-0@@ -1666,6 +1666,8 @@ map_complete:Quoted 8 lines
drivers/infiniband/ulp/srp/ib_srp.c map_complete: · line 1672
Quoted 6 lines
drivers/infiniband/ulp/srp/ib_srp.c +2
-0unmap:srp_unmap_data (scmnd, ch, req, true); +if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size) + ret = -E2BIG ;return ret; }Why return E2BIG for ENOMEM as well?
Quoted 5 lines
Hello Sagi,
The
srp_queuecommand()function translates ENOMEM intoQUEUE_FULLwhich causes the SCSI mid-layer to retry the command. All other error codes are translated intoDID_ERRORwhich causes the SCSI command to fail.
That's what I meant, ENOMEM is transient by nature. Why would you not want the scsi layer to requeue? I do understand this for the nmdesc condition but for the ENOMEM?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 5639040F.1040503-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org
Re: [PATCH 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop
Quoted 30 lines · Sagi Grimberg
Quoted 29 lines · Bart Van Assche
Quoted 17 lines · Sagi Grimberg
Can you spare a few words on this change in the change log?
Quoted 14 lines
Signed-off-by Bart Van Assche <[email protected]>Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Sebastian Parschauer <[email protected]>--- drivers/infiniband/ulp/srp/ib_srp.c | 2 ++ 1 file changed, 2 insertions(+)drivers/infiniband/ulp/srp/ib_srp.c +0
-0@@ -1666,6 +1666,8 @@ map_complete:Quoted 8 lines
drivers/infiniband/ulp/srp/ib_srp.c map_complete: · line 1672
Quoted 6 lines
drivers/infiniband/ulp/srp/ib_srp.c +2
-0unmap:srp_unmap_data (scmnd, ch, req, true); +if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size) + ret = -E2BIG ;return ret; }Why return E2BIG for ENOMEM as well?
Hello Sagi,
Quoted 7 lines
Quoted 3 lines
The
srp_queuecommand()function translates ENOMEM intoQUEUE_FULLwhich causes the SCSI mid-layer to retry the command. All other error codes are translated intoDID_ERRORwhich causes the SCSI command to fail.That's what I meant, ENOMEM is transient by nature. Why would you not want the scsi layer to requeue? I do understand this for the nmdesc condition but for the ENOMEM?
Hello Sagi,
Since the ret == -ENOMEM test is redundant in the above code that test can be left out. Would that make this patch more clear to you ?
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop
Quoted 5 lines
Hello Sagi,
Since the ret ==
-ENOMEMtest is redundant in the above code that test can be left out. Would that make this patch more clear to you ?
It will.
Thanks,
Sagi.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 6 lines
Submitting a SCSI request through the
SG_IOmechanism with a scatterlist that is longer than what is supported by the SRP initiator triggers an infinite loop. This patch series fixes that behavior.The individual patches in this series are as follows:
Quoted 7 lines
0001-IB-srp-Fix-a-spelling-error.patch 0002-IB-srp-Document-srp_map_data-return-value.patch 0003-IB-srp-Rename-work-request-ID-labels.patch 0004-IB-srp-Fix-a-potential-queue-overflow-in-an-error-pa.patch 0005-IB-srp-Fix-srp_map_data-error-paths.patch 0006-IB-srp-Introduce-target-mr_pool_size.patch 0007-IB-srp-Avoid-that-mapping-failure-triggers-an-infini.patch
Hi Bart,
I didn't look at the patches yet but are these patches on top of the registration changes I posted for 4.4? I am hoping Doug will merge them so if they aren't it might be a good idea to respin the series on top of that.
Cheers,
Sagi.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 14 lines · Sagi Grimberg
Quoted 6 lines
Submitting a SCSI request through the
SG_IOmechanism with a scatterlist that is longer than what is supported by the SRP initiator triggers an infinite loop. This patch series fixes that behavior.The individual patches in this series are as follows:
Quoted 7 lines
0001-IB-srp-Fix-a-spelling-error.patch 0002-IB-srp-Document-srp_map_data-return-value.patch 0003-IB-srp-Rename-work-request-ID-labels.patch 0004-IB-srp-Fix-a-potential-queue-overflow-in-an-error-pa.patch 0005-IB-srp-Fix-srp_map_data-error-paths.patch 0006-IB-srp-Introduce-target-mr_pool_size.patch 0007-IB-srp-Avoid-that-mapping-failure-triggers-an-infini.patch
Quoted 4 lines
I didn't look at the patches yet but are these patches on top of the registration changes I posted for 4.4? I am hoping Doug will merge them so if they aren't it might be a good idea to respin the series on top of that.
Hello Sagi,
These patches have been prepared on top of v4.3-rc7. Doug, did I see correctly that you have not yet queued Sagi's registration API changes for the v4.4 merge window ?
Thanks,
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html