Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2020-09-03 at 15:00 +0200, Martin Wilck wrote:
> On Wed, 2020-09-02 at 08:14 -0700, Sudhakar Panneerselvam wrote:
> > Hi Martin,
> > 
> > > AFAICS, your patch breaks the case where the above code is
> > > executed
> > > to
> > > derive unpacked_lun from the tag. The updated value of
> > > unpacked_lun
> > > is
> > > never used. That would break aborts for the qla2xxx target.
> > > 
> > > Am I overlooking something? Can you please explain?
> > > 
> > 
> > You are right. This change breaks the qlogic abort task code path,
> > since it is the only transport that sets the
> > TARGET_SCF_LOOKUP_LUN_FROM_TAG flag making that condition true. My
> > apologies. I can send out a patch if you have not written one
> > already. Please let me know.
> 
> Please go ahead. I haven't written a patch - I'm not experienced
> enough
> with the target code to quickly grok whether simply moving the
> target_lookup_lun_from_tag() code upward would work, in particular
> wrt
> handling failures and cleaning up.

We may want to discuss whether TARGET_SCF_LOOKUP_LUN_FROM_TAG should be
removed entirely. As you said, qla2xxx is the only user of this
feature. And it actually uses it only in a single code path, 
__qlt_24xx_handle_abts() (everywhere else the LUN is known beforehand,
AFAICT).

If you look at the code of __qlt_24xx_handle_abts(), you can see that
it calls tcm_qla2xxx_find_cmd_by_tag(), which does the same thing
as target_lookup_lun_from_tag(): iterate over se_sess->sess_cmd_list
until a matching tag is found. It would clearly be equivalent, and more
efficient, if qla2xxx set the lun directly rather than relying on
common code iterating through the same list again, later.

Martin





[Index of Archives]     [Linux SCSI]     [Kernel Newbies]     [Linux SCSI Target Infrastructure]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Device Mapper]

  Powered by Linux