Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <90ce634faf2484405a51b4628be5b61be5a0f0a6.camel@redhat.com>
Subject: Re: [PATCH] target: pscsi: set SCF_TREAT_READ_AS_NORMAL flag only
 if there is valid data
From:   Laurence Oberman <loberman@redhat.com>
To:     David Jeffery <djeffery@redhat.com>, target-devel@vger.kernel.org
Cc:     "Martin K . Petersen" <martin.petersen@oracle.com>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Wed, 27 Apr 2022 15:30:44 -0400
In-Reply-To: <ae63bff76f384ad7d28aa0833beba6860171aea9.camel@redhat.com>
References: <20220427183250.291881-1-djeffery@redhat.com>
         <ae63bff76f384ad7d28aa0833beba6860171aea9.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2022-04-27 at 15:26 -0400, Laurence Oberman wrote:
> On Wed, 2022-04-27 at 14:32 -0400, David Jeffery wrote:
> > With tape devices, the SCF_TREAT_READ_AS_NORMAL flag is used by the
> > target
> > subsystem to mark commands which have both data to return as well
> > as
> > sense data. But with pscsi, SCF_TREAT_READ_AS_NORMAL can be set
> > even
> > if
> > there is no data to return. The SCF_TREAT_READ_AS_NORMAL flag
> > causes
> > the
> > target core to call iscsit datain callbacks even if there is no
> > data,
> > which
> > iscsit does not support. This results in iscsit going into an error
> > state
> > requiring recovery and being unable to complete the command to the
> > initiator.
> > 
> > This issue can be resolved by fixing pscsi to only set
> > SCF_TREAT_READ_AS_NORMAL if there is valid data to return along
> > side
> > the
> > sense data.
> > 
> > Fixes: bd81372065fa ("scsi: target: transport should handle st
> > FM/EOM/ILI reads")
> > Signed-off-by: David Jeffery <djeffery@redhat.com>
> > Tested-by: Laurence Oberman <loberman@redhat.com>
> > ---
> >  drivers/target/target_core_pscsi.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_pscsi.c
> > b/drivers/target/target_core_pscsi.c
> > index ff292b75e23f..60dafe4c581b 100644
> > --- a/drivers/target/target_core_pscsi.c
> > +++ b/drivers/target/target_core_pscsi.c
> > @@ -588,7 +588,7 @@ static void pscsi_destroy_device(struct
> > se_device
> > *dev)
> >  }
> >  
> >  static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status,
> > -			       unsigned char *req_sense)
> > +			       unsigned char *req_sense, int
> > valid_data)
> >  {
> >  	struct pscsi_dev_virt *pdv = PSCSI_DEV(cmd->se_dev);
> >  	struct scsi_device *sd = pdv->pdv_sd;
> > @@ -681,7 +681,7 @@ static void pscsi_complete_cmd(struct se_cmd
> > *cmd, u8 scsi_status,
> >  		 * back despite framework assumption that a
> >  		 * check condition means there is no data
> >  		 */
> > -		if (sd->type == TYPE_TAPE &&
> > +		if (sd->type == TYPE_TAPE && valid_data &&
> >  		    cmd->data_direction == DMA_FROM_DEVICE) {
> >  			/*
> >  			 * is sense data valid, fixed format,
> > @@ -1032,6 +1032,7 @@ static void pscsi_req_done(struct request
> > *req,
> > blk_status_t status)
> >  	struct se_cmd *cmd = req->end_io_data;
> >  	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
> >  	enum sam_status scsi_status = scmd->result & 0xff;
> > +	int valid_data = cmd->data_length - scmd->resid_len;
> >  	u8 *cdb = cmd->priv;
> >  
> >  	if (scsi_status != SAM_STAT_GOOD) {
> > @@ -1039,12 +1040,11 @@ static void pscsi_req_done(struct request
> > *req, blk_status_t status)
> >  			" 0x%02x Result: 0x%08x\n", cmd, cdb[0], scmd-
> > > result);
> > 
> >  	}
> >  
> > -	pscsi_complete_cmd(cmd, scsi_status, scmd->sense_buffer);
> > +	pscsi_complete_cmd(cmd, scsi_status, scmd->sense_buffer,
> > valid_data);
> >  
> >  	switch (host_byte(scmd->result)) {
> >  	case DID_OK:
> > -		target_complete_cmd_with_length(cmd, scsi_status,
> > -			cmd->data_length - scmd->resid_len);
> > +		target_complete_cmd_with_length(cmd, scsi_status,
> > valid_data);
> >  		break;
> >  	default:
> >  		pr_debug("PSCSI Host Byte exception at cmd: %p CDB:"
> 
> We added a bucnh of debug to track this down, and the fix is solid
> and
> makes perfect sense.
> 
> Reviewed-by: Laurence Oberman <loberman@redhat.com>

Martin, please add
Reported-by: Scott Hamilton <scott.hamilton@atos.net>