Re: [PATCH v5] sd: Check for unaligned partial completion
Martin, On 2/22/17 13:24, Martin K. Petersen wrote: >> "Damien" == Damien Le Moal 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.lem...@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, 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.lem...@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, 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.lem...@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" == Damien Le Moal 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
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.lem...@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
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" == Damien Le Moal 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
Martin, On 2/21/17 11:35, Martin K. Petersen wrote: >> "Damien" == Damien Le Moal 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.lem...@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" == Damien Le Moal 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
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.lem...@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
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
Christoph, On 2/17/17 17:25, Christoph Hellwig wrote: > Looks fine, > > Reviewed-by: Christoph Hellwig 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.lem...@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
Looks fine, Reviewed-by: Christoph Hellwig
[PATCH v5] sd: Check for unaligned partial completion
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
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
