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]

 



Hi Martin,

> -----Original Message-----
> From: Martin Wilck [mailto:mwilck@xxxxxxxx]
> Sent: Wednesday, September 2, 2020 7:50 AM
> To: Sudhakar Panneerselvam <sudhakar.panneerselvam@xxxxxxxxxx>; Martin
> Petersen <martin.petersen@xxxxxxxxxx>; Michael Christie
> <michael.christie@xxxxxxxxxx>; target-devel@xxxxxxxxxxxxxxx; linux-
> scsi@xxxxxxxxxxxxxxx
> Cc: Shirley Ma <shirley.ma@xxxxxxxxxx>; Hannes Reinecke <hare@xxxxxxxx>;
> Daniel Wagner <daniel.wagner@xxxxxxxx>; Arun Easi <aeasi@xxxxxxxxxxx>
> Subject: Re: [PATCH v4 2/4] target: initialize LUN in transport_init_se_cmd().
> 
> Hello Sudhakar,
> 
> On Sun, 2020-06-07 at 19:58 +0000, Sudhakar Panneerselvam wrote:
> > Initialization of orig_fe_lun is moved to transport_init_se_cmd()
> > from
> > transport_lookup_cmd_lun(). This helps for the cases where the scsi
> > request
> > fails before the call to transport_lookup_cmd_lun() so that
> > trace_target_cmd_complete() can print the LUN information to the
> > trace
> > buffer. Due to this change, the lun parameter is removed from
> > transport_lookup_cmd_lun() and transport_lookup_tmr_lun().
> >
> > Signed-off-by: Sudhakar Panneerselvam <
> > sudhakar.panneerselvam@xxxxxxxxxx>
> > ---
> >  drivers/target/iscsi/iscsi_target.c    | 11 +++++------
> >  drivers/target/target_core_device.c    | 19 ++++++++-----------
> >  drivers/target/target_core_tmr.c       |  4 ++--
> >  drivers/target/target_core_transport.c | 12 +++++++-----
> >  drivers/target/target_core_xcopy.c     |  4 ++--
> >  drivers/usb/gadget/function/f_tcm.c    |  6 ++++--
> >  include/target/target_core_fabric.h    |  6 +++---
> >  7 files changed, 31 insertions(+), 31 deletions(-)
> >
> > [...]
> > diff --git a/drivers/target/target_core_transport.c
> > b/drivers/target/target_core_transport.c
> > index f2f7c5b818cc..7ea77933e64d 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > [...]
> > @@ -1790,7 +1792,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> > struct se_session *se_sess,
> >  	BUG_ON(!se_tpg);
> >
> >  	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
> > -			      0, DMA_NONE, TCM_SIMPLE_TAG, sense);
> > +			      0, DMA_NONE, TCM_SIMPLE_TAG, sense,
> > unpacked_lun);
> >  	/*
> >  	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req
> >  	 * allocation failure.
> 
> Between this hunk and the next one in target_submit_tmr(), there
> is this code:
> 
>         /*
>          * If this is ABORT_TASK with no explicit fabric provided LUN,
>          * go ahead and search active session tags for a match to figure
>          * out unpacked_lun for the original se_cmd.
>          */
>         if (tm_type == TMR_ABORT_TASK && (flags &
> TARGET_SCF_LOOKUP_LUN_FROM_TAG)) {
>                 if (!target_lookup_lun_from_tag(se_sess, tag, &unpacked_lun))
>                         goto failure;
>         }
> 
> > @@ -1818,7 +1820,7 @@ int target_submit_tmr(struct se_cmd *se_cmd,
> > struct se_session *se_sess,
> >  			goto failure;
> >  	}
> >
> > -	ret = transport_lookup_tmr_lun(se_cmd, unpacked_lun);
> > +	ret = transport_lookup_tmr_lun(se_cmd);
> >  	if (ret)
> >  		goto failure;
> >
> 
> 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.

Thanks
Sudhakar

> Regards
> 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