On Thu, 2011-12-01 at 19:59 +0100, Bart Van Assche wrote: > Make sure that the default block layer timeout is above the > InfiniBand transport layer timeout. The default block layer > timeout is 30 seconds. Typical values for the QP local ack > timeout and retry count are 19 and 7 respectively, which means > that it can take up to 60.1 seconds before a HCA submits an > error completion. The block layer timeout must be larger than > the transport layer timeout because otherwise it can happen > that an SRP response is received after the SCSI layer has > already killed a command.
Good idea, but a few nits: > +static int srp_slave_alloc(struct scsi_device *sdev) > +{ > + struct Scsi_Host *shost = sdev->host; > + struct srp_target_port *target = host_to_target(shost); > + > + if (!WARN_ON(target->rq_tmo_jiffies == 0)) > + blk_queue_rq_timeout(sdev->request_queue, > + target->rq_tmo_jiffies); What does having a WARN_ON here buy us? We should just set target->rq_tmo_jiffies to a default value in srp_create_target(), and adjust it as needed based on the CM response. Also, since this is a call-back routine, please put it after srp_reset_host() rather than in the middle of the target setup and CM handling. > static void srp_cm_rep_handler(struct ib_cm_id *cm_id, > struct srp_login_rsp *lrsp, > struct srp_target_port *target) > @@ -1431,6 +1442,24 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id, > if (ret) > goto error_free; > > + if (!WARN_ON((attr_mask & (IB_QP_TIMEOUT | IB_QP_RETRY_CNT)) == 0)) { Mentally unpacking that to realize the body gets executed if either of those is set took a bit of thought... I think you will be better served setting a default and reducing line that to if (attr_mask & (IP_QP_TIMEOUT | IB_QP_RETRY_CNT)) { Also, you need to handle the case where only one of those is set, and provide defaults when they aren't -- unless the spec says they both must always be set (I haven't looked), in which case you can modify the attr_mask test. > + uint64_t T_tr_ns, max_compl_time_ms; > + > + /* > + * Set target->rq_tmo_jiffies to one second more than the > + * largest time it can take before an error completion is > + * generated. > + */ The calculations have a fair number of magic numbers in them; may be useful to explain. You should be able to use nsecs_to_jiffies() instead of doing the math yourself, but I see that's not exported. Bummer, but we can leave fixing that for later, I think. > + T_tr_ns = 1ULL << (12 + qp_attr->timeout); > + max_compl_time_ms = qp_attr->retry_cnt * 4 * T_tr_ns; > + do_div(max_compl_time_ms, 1000 * 1000); > + target->rq_tmo_jiffies = > + msecs_to_jiffies(max_compl_time_ms + 1000); > + pr_debug("max. IB completion time = %lld ms; block layer" > + " timeout = %d jiffies\n", max_compl_time_ms, > + target->rq_tmo_jiffies); Is the debug statement really needed? Though I suppose it isn't really hurting anything... > diff --git a/drivers/infiniband/ulp/srp/ib_srp.h > b/drivers/infiniband/ulp/srp/ib_srp.h > index 020caf0..f010bd9 100644 > --- a/drivers/infiniband/ulp/srp/ib_srp.h > +++ b/drivers/infiniband/ulp/srp/ib_srp.h > @@ -138,6 +138,7 @@ struct srp_target_port { > u32 lkey; > u32 rkey; > enum srp_target_state state; > + u32 rq_tmo_jiffies; > unsigned int max_iu_len; Please don't put that in the middle of the options used in the hot path; it should go further down in the struct, somewhere after the comment /* Everything above this point is used in the hot path of * command processing. Try to keep them packed into cachelines. */ Bonus points if you can shove into a hole caused by packing rules. -- Dave Dillow National Center for Computational Science Oak Ridge National Laboratory (865) 241-6602 office -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html