On Thu, Dec 15, 2011 at 7:08 PM, David Dillow <[email protected]> wrote:
> On Thu, 2011-12-01 at 19:58 +0100, Bart Van Assche wrote:
> > Currently the ib_srp driver allows to log in multiple times to the
> > same target via its sysfs interface. This leads to each target LUN
> > being imported multiple times at the initiator side, something that
> > should not be allowed.
>
> I'm of mixed opinion on this one. It seems like we're working around a
> broken target -- we request single RDMA channel operation in our login
> requests, so the standard says that the target should disconnect all
> other connections for that I_T nexus when it receives a new login
> request. That should keep us from having multiple instances of LUNs from
> the same target over a single srp_host (HCA).
>
> In the past, we've seen connections having issues but not going away,
> and the only way to kill them was to create a new connection to the RAID
> controller, which would then kill the old one. It's been a few years
> since I've seen this, so the details are a bit fuzzy, but I recall the
> current behavior being useful.
>
> However, I'm not adverse to going beyond the standard if it makes sense,
> and your later patch to allow disconnecting channels without removing
> the module would fix the use case above. What problem are we solving
> here other than a potentially confused admin?
I agree that relogin is a useful feature. But I doubt the way it works
now is fine because during a certain time window all LUNs appear twice
at the initiator side - half of them working, half of them not.
Relogin is still possible with this patch set, but it requires an
extra step: deleting a target before attempting to log in again.
> > + spin_lock(&host->target_lock);
> > + list_for_each_entry(t, &host->target_list, list) {
> > + WARN_ON(t == target);
> > + if (target->id_ext == t->id_ext &&
> > + target->ioc_guid == t->ioc_guid &&
> > + memcmp(target->orig_dgid, t->orig_dgid, 16) == 0 &&
> > + target->path.pkey == t->path.pkey &&
> > + target->service_id == t->service_id) {
>
> The spec discusses the I_T nexus defined by the initiator and target
> port identifiers as the unit for disambiguation when running in single
> channel mode. The initiator port identifier is controlled by the port's
> GUID combined with target->initiator_ext and the target port identifier
> is defined by target->{id_ext,ioc_guid}.
>
> From the standard's perspective, why should we care about pkey, dgid (or
> orig_dgid as above) or the service id? Those don't change the I_T nexus,
> so the target should disconnect previous channels appropriately.
I'm fine with leaving out the PKey and the service ID. But I'm not
sure it's fine to leave the DGID out because leaving out the DGID will
prevent setting up redundant connections from an initiator to a target
with multiple active IB ports. The reason the orig_dgid is being used
in the above comparison instead of dgid is because these two ID's may
differ due to automatic path migration.
> > static ssize_t srp_create_target(struct device *dev,
> > struct device_attribute *attr,
> > const char *buf, size_t count)
> > @@ -2133,6 +2161,10 @@ static ssize_t srp_create_target(struct device *dev,
> > if (ret)
> > goto err;
> >
> > + ret = srp_target_exists(host, target);
> > + if (ret)
> > + goto err;
> > +
>
> We need to tell the user that we didn't create a new connection because
> one already exists.
Does that mean that you'd like to see a pr_err() in addition to the
error code returned via the sysfs write() call ?
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
