Re: [PATCH v4] sd: Check for unaligned partial completion

[Christoph Hellwig <hch@lst.de>]

Hi Damien,

this looks reasonable, but we should ask Guilherme and Ram to confirm
it fixes their originally reported issue.  I've added them to Cc.

On Wed, Feb 15, 2017 at 11:12:30AM +0900, Damien Le Moal wrote:
> Commit "mpt3sas: Force request partial completion alignment" was not
> considering the case of REQ_TYPE_FS commands not operating on sector
> size units (e.g. REQ_OP_ZONE_REPORT and its 64B aligned partial
> replies). This could result is incorrectly retrying (forever) those
> commands.
> 
> Move the partial completion alignement check of mpt3sas to a generic
> implementation in sd_done so that the check comes after good_bytes and
> resid corrections done in that function depending on the request
> command. The check is added to the initial command switch so that
> commands with special payload size handling are ignored.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> 
> ---
> 
> Changes from v3:
> - Moved check to initial switch-case so that commands with special payload
>   handling are ignored.
> 
> Changes from v2:
> - Fixed good_bytes calculation after correction of unaligned resid
>   It should be good_bytes=scsi_buflen() - resid, and not good_bytes-=resid
> 
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 15 ---------------
>  drivers/scsi/sd.c                    | 20 ++++++++++++++++++++
>  2 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 0b5b423..1961535 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -4658,7 +4658,6 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
>  	struct MPT3SAS_DEVICE *sas_device_priv_data;
>  	u32 response_code = 0;
>  	unsigned long flags;
> -	unsigned int sector_sz;
>  
>  	mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
>  	scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
> @@ -4717,20 +4716,6 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
>  	}
>  
>  	xfer_cnt = le32_to_cpu(mpi_reply->TransferCount);
> -
> -	/* In case of bogus fw or device, we could end up having
> -	 * unaligned partial completion. We can force alignment here,
> -	 * then scsi-ml does not need to handle this misbehavior.
> -	 */
> -	sector_sz = scmd->device->sector_size;
> -	if (unlikely(scmd->request->cmd_type == REQ_TYPE_FS && sector_sz &&
> -		     xfer_cnt % sector_sz)) {
> -		sdev_printk(KERN_INFO, scmd->device,
> -		    "unaligned partial completion avoided (xfer_cnt=%u, sector_sz=%u)\n",
> -			    xfer_cnt, sector_sz);
> -		xfer_cnt = round_down(xfer_cnt, sector_sz);
> -	}
> -
>  	scsi_set_resid(scmd, scsi_bufflen(scmd) - xfer_cnt);
>  	if (ioc_status & MPI2_IOCSTATUS_FLAG_LOG_INFO_AVAILABLE)
>  		log_info =  le32_to_cpu(mpi_reply->IOCLogInfo);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 1f5d92a..d05a328 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1790,6 +1790,8 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  {
>  	int result = SCpnt->result;
>  	unsigned int good_bytes = result ? 0 : scsi_bufflen(SCpnt);
> +	unsigned int sector_size = SCpnt->device->sector_size;
> +	unsigned int resid;
>  	struct scsi_sense_hdr sshdr;
>  	struct scsi_disk *sdkp = scsi_disk(SCpnt->request->rq_disk);
>  	struct request *req = SCpnt->request;
> @@ -1820,6 +1822,24 @@ static int sd_done(struct scsi_cmnd *SCpnt)
>  			scsi_set_resid(SCpnt, blk_rq_bytes(req));
>  		}
>  		break;
> +	default:
> +		/*
> +	 	 * In case of bogus fw or device, we could end up having
> +	 	 * an unaligned partial completion. Check this here and force
> +	 	 * alignment.
> +	 	 */
> +		resid = scsi_get_resid(SCpnt);
> +		if (resid & (sector_size - 1)) {
> +			SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, SCpnt,
> +				"Unaligned partial completion (resid=%u, sector_sz=%u)\n",
> +				resid, sector_size));
> +			resid = round_up(resid, sector_size);
> +			if (resid < good_bytes)
> +				good_bytes -= resid;
> +			else
> +				good_bytes = 0;
> +			scsi_set_resid(SCpnt, resid);
> +		}
>  	}
>  
>  	if (result) {
> -- 
> 2.9.3
> 
> Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:
> 
> This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.
---end quoted text---