On Fri, 2013-06-28 at 14:53 +0200, Bart Van Assche wrote:
> Add the necessary functions in the SRP transport module to allow
> an SRP initiator driver to implement transport layer error handling
> similar to the functionality already provided by the FC transport
> layer. This includes:
> - Support for implementing fast_io_fail_tmo, the time that should
> elapse after having detected a transport layer problem and
> before failing I/O.
> - Support for implementing dev_loss_tmo, the time that should
> elapse after having detected a transport layer problem and
> before removing a remote port.
>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> Cc: Roland Dreier <roland@xxxxxxxxxxxxxxx>
> Cc: James Bottomley <JBottomley@xxxxxxxxxxxxx>
> Cc: David Dillow <dillowda@xxxxxxxx>
> Cc: Vu Pham <vu@xxxxxxxxxxxx>
> Cc: Sebastian Riemer <sebastian.riemer@xxxxxxxxxxxxxxxx>
> ---
> Documentation/ABI/stable/sysfs-transport-srp | 37 ++
> drivers/scsi/scsi_transport_srp.c | 477 +++++++++++++++++++++++++-
> include/scsi/scsi_transport_srp.h | 62 +++-
> 3 files changed, 573 insertions(+), 3 deletions(-)
> +/**
> + * srp_tmo_valid() - check timeout combination validity
> + *
> + * If both a fast I/O fail and a device loss timeout have been configured then
> + * the fast I/O fail timeout must be below the device loss timeout.
> + */
> +int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
> +{
> + return (fast_io_fail_tmo < 0 || dev_loss_tmo < 0 ||
> + fast_io_fail_tmo < dev_loss_tmo) &&
> + fast_io_fail_tmo < LONG_MAX / HZ &&
> + dev_loss_tmo < LONG_MAX / HZ ? 0 : -EINVAL;
> +}
There's really no need to fit it in one line to save space -- it makes
it much easier to read if you unpack it and return -EINVAL for the
problem cases.
They should also be capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT instead of
LONG_MAX / HZ, I think.
> +static ssize_t store_reconnect_delay(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, const size_t count)
> +{
> + struct srp_rport *rport = transport_class_to_srp_rport(dev);
> + char ch[16];
> + int res, delay;
> +
> + sprintf(ch, "%.*s", min_t(int, sizeof(ch) - 1, count), buf);
> + res = kstrtoint(ch, 0, &delay);
> + if (res)
> + goto out;
I don't see the need for the sprintf? kstrtoint() will give you -ERANGE
if the value doesn't fit in an int...
Ah, NULL terminated string... except sysfs guarantees this for us for
the store method -- see fill_write_buffer() in fs/sysfs.c.
> +static ssize_t store_srp_rport_fast_io_fail_tmo(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct srp_rport *rport = transport_class_to_srp_rport(dev);
> + char ch[16], *p;
> + int res;
> + int fast_io_fail_tmo;
> +
> + sprintf(ch, "%.*s", min_t(int, sizeof(ch) - 1, count), buf);
> + p = strchr(ch, '\n');
> + if (p)
> + *p = '\0';
Again, no need for the sprintf if you don't modify the buffer? Instead
of using strchr() to make the strcmp() work with newlines, just do
if (!strcmp(buf, "off") || !strcmp(buf, "off\n")) {
fast_io_fail_tmo = 1;
} else {
res = kstrtoint(buf, 0, &fast_io_fail_tmo);
...
instead?
Same comment applies for store_srp_rport_dev_loss_tmo().
I haven't fully gone over the logic just yet, so I'm relying a bit on
Hannes and Mike's discussion from the last round. This looks to be
pretty reasonably sorted, though, based on the level of review I've been
able to do so far...
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
