On Thu, Dec 15, 2011 at 8:37 PM, David Dillow <[email protected]> wrote:
> > +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.
That WARN_ON() statement is there to verify that rq_tmo_jiffies has
been initialized to a non-zero value - which is the case unless
something bad like memory corruption has happened. I can leave it out
if you want.
> > 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.
According to table 91 in the IB spec (section 11.2.4.2, Modify Queue
Pair), both the QP timeout and the retry count have to be set for RC
QP's upon the RTR to RTS transition.
> > + 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.
All these magic numbers come straight from the IB spec. I can add a
comment mentioning that if you want.
> > + 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...
That debug statement was convenient to verify the result of the
computation. But you are right, it's not really necessary. I'll leave
it out.
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
