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
