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: <ae63bff76f384ad7d28aa0833beba6860171aea9.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:26:56 -0400
In-Reply-To: <20220427183250.291881-1-djeffery@redhat.com>
References: <20220427183250.291881-1-djeffery@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 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>