[PATCH v5] sd: Check for unaligned partial completion

[PATCH v5] sd: Check for unaligned partial completion

[Damien Le Moal]

Commit f2e767bb5d6e ("mpt3sas: Force request partial completion alignment")
was not considering the case of REQ_TYPE_FS commands not operating on
logical block 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 ignores REQ_TYPE_FS
requests with special payload size handling (REQ_OP_DISCARD,
REQ_OP_WRITE_SAME, REQ_OP_ZONE_REPORT and REQ_OP_ZONE_RESET). For the
remaining REQ_OP_FLUSH, REQ_OP_READ and REQ_OP_WRITE, we only need to
check the resid alignment, correct it if necessary and then correct
good_bytes. Note that in this case, good_bytes will always initially be 0
or aligned on the device logical block size, so correcting resid alignment
will always result in good_bytes also being properly aligned.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
Fixes: f2e767bb5d6e ("mpt3sas: Force request partial completion alignment")

---

Changes from v4:
- As suggested by Bart, make sure resid does not exceed good_bytes
- Use sd_printk for message instead of going through scsi log

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                    | 17 +++++++++++++++++
 2 files changed, 17 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..525b162 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,21 @@ 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)) {
+			sd_printk(KERN_INFO, sdkp,
+				"Unaligned partial completion (resid=%u, sector_sz=%u)\n",
+				resid, sector_size);
+			resid = min(good_bytes, round_up(resid, sector_size));
+			good_bytes -= resid;
+			scsi_set_resid(SCpnt, resid);
+		}
 	}
 
 	if (result) {
-- 
2.9.3

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

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Damien Le Moal]

Christoph,

On 2/17/17 17:25, Christoph Hellwig wrote:
> Looks fine,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks.

Martin, James,

It looks like this patch did not make it into final 4.10.
This is really bad. Now ZBC support is broken from the start on mpt3sas
HBAs (so a huge chunk of the market).

Is there any possibility to get this added to 4.10.0 ?

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

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

[Bart Van Assche]

On 02/16/2017 04:20 PM, Damien Le Moal wrote:
> Move the partial completion alignement check of mpt3sas to a generic
> implementation in sd_done so that the check ignores REQ_TYPE_FS
> requests with special payload size handling (REQ_OP_DISCARD,
> REQ_OP_WRITE_SAME, REQ_OP_ZONE_REPORT and REQ_OP_ZONE_RESET).

Hello Damien,

Since the resid adjustment code is skipped for REQ_OP_DISCARD and
REQ_OP_WRITE_SAME: does the mpt3sas firmware ensure that 'resid' is
properly aligned for these request types? Did I perhaps miss something?

Thanks,

Bart.

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

[Damien Le Moal]

Bart,

On 2/21/17 02:34, Bart Van Assche wrote:
> On 02/16/2017 04:20 PM, Damien Le Moal wrote:
>> Move the partial completion alignement check of mpt3sas to a generic
>> implementation in sd_done so that the check ignores REQ_TYPE_FS
>> requests with special payload size handling (REQ_OP_DISCARD,
>> REQ_OP_WRITE_SAME, REQ_OP_ZONE_REPORT and REQ_OP_ZONE_RESET).
> 
> Hello Damien,
> 
> Since the resid adjustment code is skipped for REQ_OP_DISCARD and
> REQ_OP_WRITE_SAME: does the mpt3sas firmware ensure that 'resid' is
> properly aligned for these request types? Did I perhaps miss something?

No, I do not think anything special is done for these commands in the
driver (I did not see any code to that effect).

But for discard, since the payload is one sector, it always should be
aligned (or 0). So the actual value from the driver is ignored and
overwritten in the switch-case code with the actual request size on
success (not the payload size) and 0 on error. If anything, if the HW
returned a non-0 resid, we should mark the command as failed.

For write-same, it is basically the same. The payload is one page
containing the sector range to discard (soon multiple ranges actually).
If the commands succeeded, then clearly resid should be 0 and the value
returned by the HW ignored. In case of a failed command, no matter what
resid is, the discard is marked as entirely failed. Similarly to
write-same, a success with a non-0 resid is non-sensical. So we could
force the request as failed here in this case.

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

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

[Martin K. Petersen]

>>>>> "Damien" == Damien Le Moal <damien.lemoal@wdc.com> writes:

Hi Damien,

Damien> Move the partial completion alignement check of mpt3sas to a
Damien> generic implementation in sd_done so that the check ignores
Damien> REQ_TYPE_FS requests with special payload size handling
Damien> (REQ_OP_DISCARD, REQ_OP_WRITE_SAME, REQ_OP_ZONE_REPORT and
Damien> REQ_OP_ZONE_RESET). For the remaining REQ_OP_FLUSH, REQ_OP_READ
Damien> and REQ_OP_WRITE, we only need to check the resid alignment,
Damien> correct it if necessary and then correct good_bytes. Note that
Damien> in this case, good_bytes will always initially be 0 or aligned
Damien> on the device logical block size, so correcting resid alignment
Damien> will always result in good_bytes also being properly aligned.

I'm still not keen on having two orthogonal sanity checks wrt. figuring
out how much of a request has been completed.

Also, I find your approach hard to follow in the case where
sd_completed_bytes() is called after the resid has been adjusted. It
works, but it's not immediately obvious that that's the case. Which to
me is an indication that this entire thing needs a thorough cleanup.

If you don't feel like mucking more with this, I understand. In that
case might pick up your patch and attempt to clean things up later.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Damien Le Moal]

Martin,

On 2/21/17 11:35, Martin K. Petersen wrote:
>>>>>> "Damien" == Damien Le Moal <damien.lemoal@wdc.com> writes:
> 
> Hi Damien,
> 
> Damien> Move the partial completion alignement check of mpt3sas to a
> Damien> generic implementation in sd_done so that the check ignores
> Damien> REQ_TYPE_FS requests with special payload size handling
> Damien> (REQ_OP_DISCARD, REQ_OP_WRITE_SAME, REQ_OP_ZONE_REPORT and
> Damien> REQ_OP_ZONE_RESET). For the remaining REQ_OP_FLUSH, REQ_OP_READ
> Damien> and REQ_OP_WRITE, we only need to check the resid alignment,
> Damien> correct it if necessary and then correct good_bytes. Note that
> Damien> in this case, good_bytes will always initially be 0 or aligned
> Damien> on the device logical block size, so correcting resid alignment
> Damien> will always result in good_bytes also being properly aligned.
> 
> I'm still not keen on having two orthogonal sanity checks wrt. figuring
> out how much of a request has been completed.
> 
> Also, I find your approach hard to follow in the case where
> sd_completed_bytes() is called after the resid has been adjusted. It
> works, but it's not immediately obvious that that's the case. Which to
> me is an indication that this entire thing needs a thorough cleanup.
> 
> If you don't feel like mucking more with this, I understand. In that
> case might pick up your patch and attempt to clean things up later.

Initially, I didn't want to change more than I did so that the patch
quickly make it to 4.10 and we get ZBC working with LSI HBAs. Since this
did not happen and we have more time ahead, I can respin everything into
sd_completed_bytes to clean things up.

I will resend a patch.

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

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

[Martin K. Petersen]

>>>>> "Damien" == Damien Le Moal <damien.lemoal@wdc.com> writes:

Damien,

Damien> Initially, I didn't want to change more than I did so that the
Damien> patch quickly make it to 4.10 and we get ZBC working with LSI
Damien> HBAs. Since this did not happen and we have more time ahead, I
Damien> can respin everything into sd_completed_bytes to clean things
Damien> up.

Damien> I will resend a patch.

Great, thanks! Would be nice to get this cleaned up.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Bart Van Assche]

On 02/20/2017 06:35 PM, Martin K. Petersen wrote:
> I'm still not keen on having two orthogonal sanity checks wrt. figuring
> out how much of a request has been completed.
> 
> Also, I find your approach hard to follow in the case where
> sd_completed_bytes() is called after the resid has been adjusted. It
> works, but it's not immediately obvious that that's the case. Which to
> me is an indication that this entire thing needs a thorough cleanup.

Hello Martin and Damien,

How about the following:
* Add a function to the block layer that reports whether or not the
  request is a medium access request. The number of transferred bytes
  for a medium access request is a multiple of the logical block size.
  (The terminology "medium access command" comes from the SCSI specs.)
* Use that function instead of "scmd->request->cmd_type == REQ_TYPE_FS"
  in the mpt3sas driver.
* Do not modify sd_done().

This approach has the advantage that the mpt3sas firmware bug workaround
does not slow down the hot path of the sd driver when another LLD than
mpt3sas is used.

Bart.

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

[Damien Le Moal]

Bart,

On 2/21/17 13:21, Bart Van Assche wrote:
> On 02/20/2017 06:35 PM, Martin K. Petersen wrote:
>> I'm still not keen on having two orthogonal sanity checks wrt. figuring
>> out how much of a request has been completed.
>>
>> Also, I find your approach hard to follow in the case where
>> sd_completed_bytes() is called after the resid has been adjusted. It
>> works, but it's not immediately obvious that that's the case. Which to
>> me is an indication that this entire thing needs a thorough cleanup.
> 
> Hello Martin and Damien,
> 
> How about the following:
> * Add a function to the block layer that reports whether or not the
>   request is a medium access request. The number of transferred bytes
>   for a medium access request is a multiple of the logical block size.
>   (The terminology "medium access command" comes from the SCSI specs.)
> * Use that function instead of "scmd->request->cmd_type == REQ_TYPE_FS"
>   in the mpt3sas driver.
> * Do not modify sd_done().
> 
> This approach has the advantage that the mpt3sas firmware bug workaround
> does not slow down the hot path of the sd driver when another LLD than
> mpt3sas is used.

I think we would still need the check for REQ_TYPE_FS to avoid
interfering with SG_IO commands. As for the "medium access command"
test, I am not sure if the block layer is the right place to define that
since a request operation may map to different commands depending on the
device type (e.g. REQ_OP_DISCARD which can become unmap or write same in
SCSI).

Martin,

Which approach do you prefer ? Keeping everything contained to mpt3sas
(so basically just fixing the problematic patch), or cleaning up
everything with sd_completed_bytes rewrite ?

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

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

[Martin K. Petersen]

>>>>> "Damien" == Damien Le Moal <damien.lemoal@wdc.com> writes:

Damien,

Damien> I think we would still need the check for REQ_TYPE_FS to avoid
Damien> interfering with SG_IO commands. As for the "medium access
Damien> command" test, I am not sure if the block layer is the right
Damien> place to define that since a request operation may map to
Damien> different commands depending on the device type

Yeah, we should put that logic in sd.c since that's the entity preparing
the commands.

Damien> Which approach do you prefer ? Keeping everything contained to
Damien> mpt3sas (so basically just fixing the problematic patch), or
Damien> cleaning up everything with sd_completed_bytes rewrite ?

I was originally in favor of just patching mpt3sas since it's clearly
broken (a disk can't write a partial sector). But I don't think a device
driver should know how to special case REPORT ZONES or similar. That's
clearly SBC/ZBC territory, so I prefer the sd_completed_bytes()
approach.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Damien Le Moal]

Martin,

On 2/22/17 13:24, Martin K. Petersen wrote:
> Damien> I think we would still need the check for REQ_TYPE_FS to avoid
> Damien> interfering with SG_IO commands. As for the "medium access
> Damien> command" test, I am not sure if the block layer is the right
> Damien> place to define that since a request operation may map to
> Damien> different commands depending on the device type
> 
> Yeah, we should put that logic in sd.c since that's the entity preparing
> the commands.

OK. Will do. Bart already sent me some untested patches doing that. I
will work with him to clean that up.

> Damien> Which approach do you prefer ? Keeping everything contained to
> Damien> mpt3sas (so basically just fixing the problematic patch), or
> Damien> cleaning up everything with sd_completed_bytes rewrite ?
> 
> I was originally in favor of just patching mpt3sas since it's clearly
> broken (a disk can't write a partial sector). But I don't think a device
> driver should know how to special case REPORT ZONES or similar. That's
> clearly SBC/ZBC territory, so I prefer the sd_completed_bytes()
> approach.

OK. Cooking now...

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

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

[Damien Le Moal]

Martin,

On 2/22/17 13:24, Martin K. Petersen wrote:
>>>>>> "Damien" == Damien Le Moal <damien.lemoal@wdc.com> writes:
> 
> Damien,
> 
> Damien> I think we would still need the check for REQ_TYPE_FS to avoid
> Damien> interfering with SG_IO commands. As for the "medium access
> Damien> command" test, I am not sure if the block layer is the right
> Damien> place to define that since a request operation may map to
> Damien> different commands depending on the device type
> 
> Yeah, we should put that logic in sd.c since that's the entity preparing
> the commands.
> 
> Damien> Which approach do you prefer ? Keeping everything contained to
> Damien> mpt3sas (so basically just fixing the problematic patch), or
> Damien> cleaning up everything with sd_completed_bytes rewrite ?
> 
> I was originally in favor of just patching mpt3sas since it's clearly
> broken (a disk can't write a partial sector). But I don't think a device
> driver should know how to special case REPORT ZONES or similar. That's
> clearly SBC/ZBC territory, so I prefer the sd_completed_bytes()
> approach.

I do not see the problematic resid correction code in the mpt3sas driver
in 4.11/scsi-queue branch. Is this expected ? Did the mpt3sas driver
updates removed it ?

It looks like that branch is based on 4.10.0-rc2. The mpt3sas was
applied in rc7. Which branch should I use for basing the patches ?

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com

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

[Damien Le Moal]

Martin,

On 2/23/17 13:44, Damien Le Moal wrote:
> I do not see the problematic resid correction code in the mpt3sas driver
> in 4.11/scsi-queue branch. Is this expected ? Did the mpt3sas driver
> updates removed it ?
> 
> It looks like that branch is based on 4.10.0-rc2. The mpt3sas was
> applied in rc7. Which branch should I use for basing the patches ?

Please ignore. The code is in 4.11/scsi-fixes. Of course...
Sorry about the noise.

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com