| Message ID | 51D41F52.4000409@acm.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Wed, 2013-07-03 at 14:55 +0200, Bart Van Assche wrote: > Finish all outstanding I/O requests after fast_io_fail_tmo expired, > which speeds up failover in a multipath setup. This patch is a > reworked version of a patch from Sebastian Riemer. > > Reported-by: Sebastian Riemer <sebastian.riemer@profitbricks.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Acked-by: David Dillow <dillowda@ornl.gov> I don't believe I ack'd this; I don't want the callers doing the result shift, do it in srp_finish_req(). -- 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 07/03/13 16:08, David Dillow wrote: > On Wed, 2013-07-03 at 14:55 +0200, Bart Van Assche wrote: >> Finish all outstanding I/O requests after fast_io_fail_tmo expired, >> which speeds up failover in a multipath setup. This patch is a >> reworked version of a patch from Sebastian Riemer. >> >> Reported-by: Sebastian Riemer <sebastian.riemer@profitbricks.com> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> Acked-by: David Dillow <dillowda@ornl.gov> > > I don't believe I ack'd this; I don't want the callers doing the result > shift, do it in srp_finish_req(). My apologies. You are correct, this patch was not yet acknowledged by you. Regarding the shift itself: is it really that important whether the caller or callee performs that shift ? Having it in the caller has the advantage that the compiler can optimize the shift operation out because the number that is being shifted left is a constant. And if later on it would be necessary to set more fields of the SCSI result in a caller of srp_finish_req() then that will be possible without having to modify the srp_finish_req() function itself. Bart. -- 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, 2013-07-03 at 16:45 +0200, Bart Van Assche wrote: > Having it in the caller has the > advantage that the compiler can optimize the shift operation out because > the number that is being shifted left is a constant. srp_finish_req() is likely to be inlined, so the compiler will be able to make this optimization. Regardless, this is so far in the noise that it looses to readability. > And if later on it > would be necessary to set more fields of the SCSI result in a caller of > srp_finish_req() then that will be possible without having to modify the > srp_finish_req() function itself. Other than REQ_QUIET, what do you think would need to be added? I think we can cross that bridge when we get there, as I don't think REQ_QUIET should not be set in the LLDs. -- 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, 2013-07-03 at 10:57 -0400, David Dillow wrote: > On Wed, 2013-07-03 at 16:45 +0200, Bart Van Assche wrote: > > Having it in the caller has the > > advantage that the compiler can optimize the shift operation out because > > the number that is being shifted left is a constant. > > srp_finish_req() is likely to be inlined, so the compiler will be able > to make this optimization. Regardless, this is so far in the noise that > it looses to readability. Eh, just leave it alone. As much as I don't like it, it does look to be fairly common among the LLDs and other transport code. Acked-by: David Dillow <dillowda@ornl.gov> -- 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/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index f65701d..8ba4e9c 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -686,17 +686,29 @@ static void srp_free_req(struct srp_target_port *target, spin_unlock_irqrestore(&target->lock, flags); } -static void srp_reset_req(struct srp_target_port *target, struct srp_request *req) +static void srp_finish_req(struct srp_target_port *target, + struct srp_request *req, int result) { struct scsi_cmnd *scmnd = srp_claim_req(target, req, NULL); if (scmnd) { srp_free_req(target, req, scmnd, 0); - scmnd->result = DID_RESET << 16; + scmnd->result = result; scmnd->scsi_done(scmnd); } } +static void srp_terminate_io(struct srp_rport *rport) +{ + struct srp_target_port *target = rport->lld_data; + int i; + + for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) { + struct srp_request *req = &target->req_ring[i]; + srp_finish_req(target, req, DID_TRANSPORT_FAILFAST << 16); + } +} + static int srp_reconnect_target(struct srp_target_port *target) { struct Scsi_Host *shost = target->scsi_host; @@ -723,8 +735,7 @@ static int srp_reconnect_target(struct srp_target_port *target) for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) { struct srp_request *req = &target->req_ring[i]; - if (req->scmnd) - srp_reset_req(target, req); + srp_finish_req(target, req, DID_RESET << 16); } INIT_LIST_HEAD(&target->free_tx); @@ -1782,7 +1793,7 @@ static int srp_reset_device(struct scsi_cmnd *scmnd) for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) { struct srp_request *req = &target->req_ring[i]; if (req->scmnd && req->scmnd->device == scmnd->device) - srp_reset_req(target, req); + srp_finish_req(target, req, DID_RESET << 16); } return SUCCESS; @@ -2594,6 +2605,7 @@ static void srp_remove_one(struct ib_device *device) static struct srp_function_template ib_srp_transport_functions = { .rport_delete = srp_rport_delete, + .terminate_rport_io = srp_terminate_io, }; static int __init srp_init_module(void)