Re: [Scst-devel] [SPF:fail] Re: FC initiator performance tanks once target mode is enabled

Re: [Scst-devel] [SPF:fail] Re: FC initiator performance tanks once target mode is enabled

[Steve Magnani]

Nicholas,

On Sat, 2013-12-14 at 10:51 -0600, Dr. Greg Wettstein wrote:
> On Dec 12, 11:45am, "Nicholas A. Bellinger" wrote:
> > > What I would prefer myself is to have a single set of target drivers
> > > that works for both LIO and SCST. That would not only make both projects
> > > easier to maintain but also would expose all target drivers to wider
> > > testing. However, since the interface between target core and target
> > > drivers is very different for LIO and for SCST reaching this goal would
> > > be a challenging project by itself.
> 
> > After spending 18+ months on qla2xxx target code, and getting it
> > into a state that it could withstand public scrutiny for mainline
> > acceptance, I have no interest in making changes to mainline driver
> > code for supporting someone else's out-of-tree stuff.
> 
> I believe, to be fair, that if it took 18 months to get the qla2x00t
> code ported into the kernel that it would have taken even longer to
> get fibre-channel support for the in-kernel target stack if the
> process would have started from scratch.  The kernel implementation
> stands very heavily on engineering work which Vlad, ID7 and others put
> into the target code.
> 
> Secondary to implementing the sqatgt driver I have been through every
> line of both Vlad's code and your derivative work.  It is a straight
> forward exercise to follow both pieces of work side by side.
> 
> So despite all the hostility and rancor which surrounds the two major
> Linux SCSI target implementations, lets concede that the in-kernel
> fibre-channel driver benefited a great deal from the efforts of the
> SCST community.  This coming from someone who arranged for a small
> amount of financial support for some of the code which is currently
> residing in the in-kernel Qlogic target driver.
> 
> > We don't add interfaces into mainline drive code to support
> > out-of-tree projects, because quite frankly, it gives less incentive
> > for the out-of-tree holdouts to use, improve and contribute to
> > mainline target code.
> 
> But the kernel does provide services to external users through a
> binary API supplied by exported symbols.  Your work on the Qlogic
> fibre-channel target driver includes all but one small function needed
> to allow SCST to use the in kernel driver.
> 
> To Nicholas' credit, the engineering work done on the qla2x00t driver,
> as a component of porting it into the kernel, provides almost all of
> the resources needed for a straight forward integration of SCST.
> Having anticipated this debate we focused on designing the sqatgt
> interface driver so it would integrate cleanly with the architectural
> model selected for the kernel.
> 
> Bart, if you could review the in-kernel Qlogic driver you will find
> the the tcm_qla2xxx.c module interfaces the Qlogic driver with the
> in-kernel SCSI target engine.  Similar functionality is provided by
> the scst_qla2xxx.c in the sqatgt driver for SCST.  So there is a
> reasonably straight forward and standardized approach for getting both
> projects on a common driver infrastructure, which as you note benefits
> everyone.
> 
> The only missing piece of the puzzle is an exported function to
> enumerate the target interfaces which are available.  Since there are
> exported functions to enable/disable interfaces, handle session
> management and a variety of other functions it doesn't seem
> unreasonable to provide a method of enumerating which interfaces are
> available for target mode.
> 
> So Nicholas in the spirit of everyone working for the common good of
> the storage community I'm hoping you will support our proposal of the
> following patch to Andrew Vasquez and the Qlogic maintainers:
> 
> ---------------------------------------------------------------------------
> +void
> +qlt_get_target_list(void (*callback)(struct scsi_qla_host *))
> +{
> +       struct qla_tgt *tgt;
> +
> +        mutex_lock(&qla_tgt_mutex);
> +        list_for_each_entry(tgt, &qla_tgt_glist, tgt_list_entry) {
> +               (*callback)(tgt->vha);
> +       }
> +        mutex_unlock(&qla_tgt_mutex);
> +
> +       return;
> +}
> +EXPORT_SYMBOL(qlt_get_target_list);
> ---------------------------------------------------------------------------
> 
> Which provides the only missing piece needed for SCST to live happily
> on as a modular extension to the kernel.  With this in place we can
> all focus on more pressing issues, such as why Steve Magnani can't get
> two ports on the same ISP chip to run in different modes.

I have code from QLogic that appears to resolve the issue with which I
started this thread, namely crummy initiator performance when the same
physical port is configured for dual initiator and target mode. QLogic's
code appears to be a fusion of the mainline qla2xxx driver and SCST,
which makes it taxonomically close to Greg's sqatgt proposal.

Once I figure out which portions of QLogic's code resolve the issue I
would be happy to post them as patches to mainline. But I can't justify
the effort to do that if Greg's proposal is DOA, since I wouldn't be
able to make use of the resulting patched code.

As an engineer with a problem to solve and a deadline, I don't much care
whether the name of the solution is LIO or SCST. But it is a necessity
of my project that the target implementation reside in userland. AFAIK
LIO does not support this, nor plan to. 

I agree with core kernel maintainers that encouraging out-of-tree binary
drivers (a la NVidia) is a Bad Thing, but that's not what's being
requested here. Please don't throw the baby out with the bathwater.

Regards,
------------------------------------------------------------------------
 Steven J. Magnani               "I claim this network for MARS!
 www.digidescorp.com              Earthling, return my space modulator!"

 #include <standard.disclaimer>

Re: [Scst-devel] [SPF:fail] Re: FC initiator performance tanks once target mode is enabled

[Nicholas A. Bellinger]

On Mon, 2013-12-23 at 09:13 -0600, Steve Magnani wrote:
> Nicholas,
> 
> On Sat, 2013-12-14 at 10:51 -0600, Dr. Greg Wettstein wrote:
> > On Dec 12, 11:45am, "Nicholas A. Bellinger" wrote:

<SNIP>

> > > We don't add interfaces into mainline drive code to support
> > > out-of-tree projects, because quite frankly, it gives less incentive
> > > for the out-of-tree holdouts to use, improve and contribute to
> > > mainline target code.
> > 
> > But the kernel does provide services to external users through a
> > binary API supplied by exported symbols.  Your work on the Qlogic
> > fibre-channel target driver includes all but one small function needed
> > to allow SCST to use the in kernel driver.
> > 
> > To Nicholas' credit, the engineering work done on the qla2x00t driver,
> > as a component of porting it into the kernel, provides almost all of
> > the resources needed for a straight forward integration of SCST.
> > Having anticipated this debate we focused on designing the sqatgt
> > interface driver so it would integrate cleanly with the architectural
> > model selected for the kernel.
> > 
> > Bart, if you could review the in-kernel Qlogic driver you will find
> > the the tcm_qla2xxx.c module interfaces the Qlogic driver with the
> > in-kernel SCSI target engine.  Similar functionality is provided by
> > the scst_qla2xxx.c in the sqatgt driver for SCST.  So there is a
> > reasonably straight forward and standardized approach for getting both
> > projects on a common driver infrastructure, which as you note benefits
> > everyone.
> > 
> > The only missing piece of the puzzle is an exported function to
> > enumerate the target interfaces which are available.  Since there are
> > exported functions to enable/disable interfaces, handle session
> > management and a variety of other functions it doesn't seem
> > unreasonable to provide a method of enumerating which interfaces are
> > available for target mode.
> > 
> > So Nicholas in the spirit of everyone working for the common good of
> > the storage community I'm hoping you will support our proposal of the
> > following patch to Andrew Vasquez and the Qlogic maintainers:
> > 
> > ---------------------------------------------------------------------------
> > +void
> > +qlt_get_target_list(void (*callback)(struct scsi_qla_host *))
> > +{
> > +       struct qla_tgt *tgt;
> > +
> > +        mutex_lock(&qla_tgt_mutex);
> > +        list_for_each_entry(tgt, &qla_tgt_glist, tgt_list_entry) {
> > +               (*callback)(tgt->vha);
> > +       }
> > +        mutex_unlock(&qla_tgt_mutex);
> > +
> > +       return;
> > +}
> > +EXPORT_SYMBOL(qlt_get_target_list);
> > ---------------------------------------------------------------------------
> > 
> > Which provides the only missing piece needed for SCST to live happily
> > on as a modular extension to the kernel.  With this in place we can
> > all focus on more pressing issues, such as why Steve Magnani can't get
> > two ports on the same ISP chip to run in different modes.
> 
> I have code from QLogic that appears to resolve the issue with which I
> started this thread, namely crummy initiator performance when the same
> physical port is configured for dual initiator and target mode. QLogic's
> code appears to be a fusion of the mainline qla2xxx driver and SCST,
> which makes it taxonomically close to Greg's sqatgt proposal.
> 

Glad to hear that you where able to resolve the issue on your setup.

However, as you've noticed mixed mode operation is currently unsupported
in mainline qla_target.c code, due to the fact that there is not yet a
clean method to transition between target + initiator mode for a given
qla2xxx port.

All of the methods for doing this in the original out-of-tree code where
terribly ugly hacks (and continue to be), and hence where left-out for
the mainline merge.

> Once I figure out which portions of QLogic's code resolve the issue I
> would be happy to post them as patches to mainline. But I can't justify
> the effort to do that if Greg's proposal is DOA, since I wouldn't be
> able to make use of the resulting patched code.
> 

My issue with Greg's proposal is the additional of new interfaces
specific to out-of-tree code, that no mainline code utilizes.  To
repeat, we do not add interfaces that are specific to out-of-tree code,
regardless of what subsystem / driver they reside.

So what needs to happen with Greg's effort is to convert to using
existing interfaces for qla2xxx port registration that tcm_qla2xxx is
already using, eg: qlt_lport_register() + qlt_lport_deregister(), and
make them work for his use-case.

They already provide what is required for an a mainline or out-of-tree
target stack to function, and yes, even ensure that multiple target
stacks can play nicely with qla_target.c logic at the same time.

> As an engineer with a problem to solve and a deadline, I don't much care
> whether the name of the solution is LIO or SCST. But it is a necessity
> of my project that the target implementation reside in userland. AFAIK
> LIO does not support this, nor plan to. 
> 

Not true.  The patches for target_core_user.c where posted last month by
Shaohua Li, and after review over the next weeks will end up being a
v3.14 -> v3.15 item for mainline.

> I agree with core kernel maintainers that encouraging out-of-tree binary
> drivers (a la NVidia) is a Bad Thing, but that's not what's being
> requested here. Please don't throw the baby out with the bathwater.
> 

That's not the issue here.  Greg's effort needs to use existing qla2xxx
port registration interfaces instead of creating a new one specific to
out-of-tree code.

--nab

Re: [Scst-devel] [SPF:fail] Re: FC initiator performance tanks once target mode is enabled

[Dr. Greg Wettstein]

On Dec 23, 12:20pm, "Nicholas A. Bellinger" wrote:
} Subject: Re: [Scst-devel] [SPF:fail] Re: FC initiator performance tanks on

Hi, hope the week is going well for everyone.

At the risk of wading into this controversy any further I thought it
might be helpful to clarify some issues given the dynamics of this
situation.

> > Once I figure out which portions of QLogic's code resolve the issue I
> > would be happy to post them as patches to mainline. But I can't justify
> > the effort to do that if Greg's proposal is DOA, since I wouldn't be
> > able to make use of the resulting patched code.

> My issue with Greg's proposal is the additional of new interfaces
> specific to out-of-tree code, that no mainline code utilizes.  To
> repeat, we do not add interfaces that are specific to out-of-tree
> code, regardless of what subsystem / driver they reside.

I think the overriding issue to consider here is there was no desire
by the SCST community to keep the codebase out of the mainline kernel.

This isn't a case of a group attempting to leverage kernel resources
while hiding their own efforts.  As I noted previously the Qlogic
target code base, modulo your significant efforts, was largely a
result of the work which was initiated in the SCST community.

Given that, I believe it is not unreasonable to review whether or not
the target code has, what a reasonable person would consider, is a
necessary and sufficient set of interfaces.

> So what needs to happen with Greg's effort is to convert to using
> existing interfaces for qla2xxx port registration that tcm_qla2xxx is
> already using, eg: qlt_lport_register() + qlt_lport_deregister(), and
> make them work for his use-case.
> 
> They already provide what is required for an a mainline or out-of-tree
> target stack to function, and yes, even ensure that multiple target
> stacks can play nicely with qla_target.c logic at the same time.

Everyone's time is at a premium but I believe this discussion would be
facilitated by a review of the sqatgt code.  Here are a couple of
extracts which may be helpful:

+static int sqa_enable_tgt(struct scst_tgt *scst_tgt, bool enable)
+{
+       struct qla_tgt *tgt;
+       struct scsi_qla_host *vha;
+
+       TRACE_ENTRY();
+
+       tgt = scst_tgt_get_tgt_priv(scst_tgt);
+       vha = tgt->vha;

.. [ removed ] ...

+       qlt_lport_register(&sqa_qla2xxx_template, wwn_to_u64(vha->port_name),
+                          sqa_lport_callback, scst_tgt);
+       qlt_enable_vha(vha);
+
+       TRACE_EXIT();
+       return 0;
+}

----

+static void sqa_target_release_cb(struct scsi_qla_host *vha)
+{
+       TRACE_ENTRY();
+
+       if (vha->hw->tgt.tgt_ops == NULL)
+               return;

.. [ removed ] ...

+       qlt_lport_deregister(vha);
+
+       TRACE_EXIT();
+       return;
+}

In SCST the qlogic port registration occurs when target mode is
enabled on the interface, ie:

	echo 1 >/sys/kernel/scst_tgt/targets/sqatgt/PWWN/enabled

Since the exported qla_target interfaces operate on 'struct
scsi_qla_host' pointer values a list of those pointers which the
Qlogic driver core has initialized as target capable is a requirement.
Since that list is static, as is the lock which protects it, a
callback interface seemed an appropriate method of obtaining this
information.

If there are suggestions for an alternative strategy I would be happy
to implement it.

Otherwise, a review of the sqatgt code will review that it implements
all of the function callback pointers in 'struct qla_tgt_func_tmpl' as
well as only using symbols which are officially exported.  It is my
contention that obtaining a list of target capable interfaces is
something which would be considered a necessary element of a properly
architected target mode driver.

The MAINTAINERS file suggests that Andrew Vasquez is the custodian of
everything in 'drivers/scsi/qla2xxx'.  If there is no consensus on an
alternative implementation I will submit a patch to Andrew for his
review.  As others have noted, having everyone on a standard codebase
would seem to be of maximal benefit to the kernel.

> > I agree with core kernel maintainers that encouraging out-of-tree binary
> > drivers (a la NVidia) is a Bad Thing, but that's not what's being
> > requested here. Please don't throw the baby out with the bathwater.

> That's not the issue here.  Greg's effort needs to use existing
> qla2xxx port registration interfaces instead of creating a new one
> specific to out-of-tree code.

As I detailed above, the sqatgt driver does use the existing
(de)-registration interfaces.  Unfortunately, that interface requires
a pointer to a target capable device definition structure, which the
callback provides.  Something of utility to not only SCST but any
other potential SCSI target stack which may evolve.

Which by definition would be considered 'out-of-tree'.

> --nab

I hope the above is helpful.

Best wishes for a productive remainder of the week.

}-- End of excerpt from "Nicholas A. Bellinger"

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.           Specializing in information infra-structure
Fargo, ND  58102            development.
PH: 701-281-1686
FAX: 701-281-3949           EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"Prioritization is a favorite management buzzword.  What it really means
 is stuff that isn't going to get done."
                                -- C.J. Peters, MD
                                   Chief, Special Pathogens Branch, CDC.