On Sun, Feb 26, 2012 at 6:32 AM, David Dillow <[email protected]> wrote:
> On Sat, 2012-01-14 at 12:41 +0000, Bart Van Assche wrote:
> > Enlarge the block layer timeout such that it is above the
> > InfiniBand transport layer timeout. This is necessary to avoid
> > that an SRP response is received after the SCSI layer has
> > already killed the associated SCSI command. Note: the timeout is
> > only set for SCSI disk devices but not for any other type of
> > SCSI device (M/O disk, tape, CD-ROM, ...).
>
> Why disk only? Different default timeouts? If so, there's a better
> solution.
If you have a look at the sd, sr and st source code, you will see that
the timeout set in slave_configure() will be honored only for regular
and MO disks but not for any other type of SCSI device. The default
timeout for MO disks is already more than large enough. By the way, if
you have a look at drivers/scsi/ibmvscsi/ibmvscsi.c, you will see that
in that driver a similar approach is taken for setting the timeout.
> > +static uint32_t srp_compute_rq_tmo(struct ib_qp_attr *qp_attr, int
> > attr_mask)
> > +{
> > + uint64_t T_tr_ns, max_compl_time_ms;
> > + uint32_t rq_tmo_jiffies;
> > +
> > + /*
> > + * According to section 11.2.4.2 in the IBTA spec (Modify Queue Pair,
> > + * table 91), both the QP timeout and the retry count have to be set
> > + * for RC QP's during the RTR to RTS transition.
> > + */
> > + WARN_ON((attr_mask & (IB_QP_TIMEOUT | IB_QP_RETRY_CNT)) !=
> > + (IB_QP_TIMEOUT | IB_QP_RETRY_CNT));
>
> Are you guaranteed to get those attributes set if they aren't changed
> from the default?
Yes. ib_cm_init_qp_attr() has to set these for the RTR to RTS
transition, otherwise it would violate the IBTA spec.
>> +static int srp_slave_configure(struct scsi_device *sdev)
>> +{
>> + struct Scsi_Host *shost = sdev->host;
>> + struct srp_target_port *target = host_to_target(shost);
>> + struct request_queue *q = sdev->request_queue;
>> + unsigned long timeout;
>> +
>> + WARN_ON(target->rq_tmo_jiffies == 0);
>> + if (sdev->type == TYPE_DISK) {
>> + timeout = max_t(unsigned, 30 * HZ, target->rq_tmo_jiffies);
>> + blk_queue_rq_timeout(q, timeout);
>> + }
>
> I think you can get rid of any dependency on the default values by just
> doing
>
> if (q->rq_timeout < target->rq_tmo_jiffies)
> blk_queue_rq_timeout(q, timeout);
That might cause the timeout to become smaller than 30s. I'm not sure
using smaller timeouts than the current default would be wise.
> This -- or the changes to srp_compute_rq_tmo() -- would also avoid the
> need for the WARN_ON().
The only reason that WARN_ON() is present is to verify that no timeout
has been set yet by sd, just in case the timeout computation code in
sd would ever be moved. I can leave out that WARN_ON() statement if
you want.
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
