* [PATCH v2] mpt3sas: Force request partial completion alignment
@ 2017-01-24 15:47 Guilherme G. Piccoli
2017-01-25 4:46 ` Sreekanth Reddy
2017-01-25 23:46 ` Martin K. Petersen
0 siblings, 2 replies; 6+ messages in thread
From: Guilherme G. Piccoli @ 2017-01-24 15:47 UTC (permalink / raw)
To: linux-scsi, MPT-FusionLinux.pdl
Cc: sathya.prakash, chaitra.basappa, suganath-prabu.subramani,
sreekanth.reddy, brking, mauricfo, linuxram, gpiccoli
From: Ram Pai <linuxram@us.ibm.com>
The firmware or device, possibly under a heavy I/O load, can return
on a partial unaligned boundary. Scsi-ml expects these requests to be
completed on an alignment boundary. Scsi-ml blindly requeues the I/O
without checking the alignment boundary of the I/O request for the
remaining bytes. This leads to errors, since devices cannot perform
non-aligned read/write operations.
This patch fixes the issue in the driver. It aligns unaligned
completions of FS requests, by truncating them to the nearest
alignment boundary.
Reported-by: Mauricio Faria De Oliveira <mauricfo@linux.vnet.ibm.com>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
v1->v2:
* Improved printk, by showing some variables too [suggested by Sreekanth].
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 75f3fce..e52c942 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -4657,6 +4657,8 @@ _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;
+ struct request *req;
mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
@@ -4715,6 +4717,21 @@ _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;
+ req = scmd->request;
+ if (unlikely(sector_sz && req && (req->cmd_type == REQ_TYPE_FS) &&
+ (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 = (xfer_cnt / sector_sz) * 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);
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mpt3sas: Force request partial completion alignment
2017-01-24 15:47 [PATCH v2] mpt3sas: Force request partial completion alignment Guilherme G. Piccoli
@ 2017-01-25 4:46 ` Sreekanth Reddy
2017-01-25 23:46 ` Martin K. Petersen
1 sibling, 0 replies; 6+ messages in thread
From: Sreekanth Reddy @ 2017-01-25 4:46 UTC (permalink / raw)
To: Guilherme G. Piccoli
Cc: linux-scsi, PDL-MPT-FUSIONLINUX, Sathya Prakash, Chaitra Basappa,
Suganath Prabu Subramani, Brian King, mauricfo, linuxram
On Tue, Jan 24, 2017 at 9:17 PM, Guilherme G. Piccoli
<gpiccoli@linux.vnet.ibm.com> wrote:
> From: Ram Pai <linuxram@us.ibm.com>
>
> The firmware or device, possibly under a heavy I/O load, can return
> on a partial unaligned boundary. Scsi-ml expects these requests to be
> completed on an alignment boundary. Scsi-ml blindly requeues the I/O
> without checking the alignment boundary of the I/O request for the
> remaining bytes. This leads to errors, since devices cannot perform
> non-aligned read/write operations.
>
> This patch fixes the issue in the driver. It aligns unaligned
> completions of FS requests, by truncating them to the nearest
> alignment boundary.
>
> Reported-by: Mauricio Faria De Oliveira <mauricfo@linux.vnet.ibm.com>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
> v1->v2:
> * Improved printk, by showing some variables too [suggested by Sreekanth].
This patch looks good. Please consider this patch as
Acked-by: Sreekanth Reddy <Sreekanth.Reddy@broadcom.com>
>
> drivers/scsi/mpt3sas/mpt3sas_scsih.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 75f3fce..e52c942 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -4657,6 +4657,8 @@ _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;
> + struct request *req;
>
> mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
> scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
> @@ -4715,6 +4717,21 @@ _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;
> + req = scmd->request;
> + if (unlikely(sector_sz && req && (req->cmd_type == REQ_TYPE_FS) &&
> + (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 = (xfer_cnt / sector_sz) * 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);
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mpt3sas: Force request partial completion alignment
2017-01-24 15:47 [PATCH v2] mpt3sas: Force request partial completion alignment Guilherme G. Piccoli
2017-01-25 4:46 ` Sreekanth Reddy
@ 2017-01-25 23:46 ` Martin K. Petersen
2017-01-26 13:31 ` Guilherme G. Piccoli
1 sibling, 1 reply; 6+ messages in thread
From: Martin K. Petersen @ 2017-01-25 23:46 UTC (permalink / raw)
To: Guilherme G. Piccoli
Cc: linux-scsi, MPT-FusionLinux.pdl, sathya.prakash, chaitra.basappa,
suganath-prabu.subramani, sreekanth.reddy, brking, mauricfo,
linuxram
>>>>> "Guilherme" == Guilherme G Piccoli <gpiccoli@linux.vnet.ibm.com> writes:
Hi Guilherme,
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 75f3fce..e52c942 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -4657,6 +4657,8 @@ _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;
+ struct request *req;
mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
@@ -4715,6 +4717,21 @@ _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;
+ req = scmd->request;
+ if (unlikely(sector_sz && req && (req->cmd_type == REQ_TYPE_FS) &&
+ (xfer_cnt % sector_sz))) {
Maybe a bit zealous on the sanity checking...
+ sdev_printk(KERN_INFO, scmd->device,
+ "unaligned partial completion avoided (xfer_cnt=%u, sector_sz=%u)\n",
+ xfer_cnt, sector_sz);
+ xfer_cnt = (xfer_cnt / sector_sz) * sector_sz;
Not so keen on divisions. xfer_cnt = round_down(xfer_cnt, sector_sz), maybe?
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mpt3sas: Force request partial completion alignment
2017-01-25 23:46 ` Martin K. Petersen
@ 2017-01-26 13:31 ` Guilherme G. Piccoli
2017-01-26 17:02 ` Ram Pai
0 siblings, 1 reply; 6+ messages in thread
From: Guilherme G. Piccoli @ 2017-01-26 13:31 UTC (permalink / raw)
To: Martin K. Petersen
Cc: linux-scsi, MPT-FusionLinux.pdl, sathya.prakash, chaitra.basappa,
suganath-prabu.subramani, sreekanth.reddy, brking, mauricfo,
linuxram
On 01/25/2017 09:46 PM, Martin K. Petersen wrote:
>>>>>> "Guilherme" == Guilherme G Piccoli <gpiccoli@linux.vnet.ibm.com> writes:
>
> Hi Guilherme,
Hi Martin, thanks for the review!
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 75f3fce..e52c942 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -4657,6 +4657,8 @@ _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;
> + struct request *req;
>
> mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
> scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
> @@ -4715,6 +4717,21 @@ _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;
> + req = scmd->request;
> + if (unlikely(sector_sz && req && (req->cmd_type == REQ_TYPE_FS) &&
> + (xfer_cnt % sector_sz))) {
>
> Maybe a bit zealous on the sanity checking...
A bit...? heheh
Too much I'd say. Since this is dealing with a bogus FW scenario, I
found more safe to check everything...of course we can remove checks if
it's sure req isn't NULL ever. The sector_sz check is avoiding
degenerate cases, since our division below.
>
> + sdev_printk(KERN_INFO, scmd->device,
> + "unaligned partial completion avoided (xfer_cnt=%u, sector_sz=%u)\n",
> + xfer_cnt, sector_sz);
> + xfer_cnt = (xfer_cnt / sector_sz) * sector_sz;
>
> Not so keen on divisions. xfer_cnt = round_down(xfer_cnt, sector_sz), maybe?
>
Martin, I might be completely wrong here (please correct me if this is
the case), but isn't C standard integer division a truncation that acts
like a round down? I checked (what I think is) the specification of C
language (ISO/IEC 9899:1999), and it seems the division proposed by Ram
Pai is accurate in this case. Also, both variables are unsigned.
Let me know your thoughts.
Thanks,
Guilherme
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mpt3sas: Force request partial completion alignment
2017-01-26 13:31 ` Guilherme G. Piccoli
@ 2017-01-26 17:02 ` Ram Pai
2017-01-26 18:37 ` Guilherme G. Piccoli
0 siblings, 1 reply; 6+ messages in thread
From: Ram Pai @ 2017-01-26 17:02 UTC (permalink / raw)
To: Guilherme G. Piccoli
Cc: Martin K. Petersen, linux-scsi, MPT-FusionLinux.pdl,
sathya.prakash, chaitra.basappa, suganath-prabu.subramani,
sreekanth.reddy, brking, mauricfo
On Thu, Jan 26, 2017 at 11:31:53AM -0200, Guilherme G. Piccoli wrote:
> On 01/25/2017 09:46 PM, Martin K. Petersen wrote:
> >>>>>> "Guilherme" == Guilherme G Piccoli <gpiccoli@linux.vnet.ibm.com> writes:
> >
> > Hi Guilherme,
>
> Hi Martin, thanks for the review!
>
>
> >
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > index 75f3fce..e52c942 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > @@ -4657,6 +4657,8 @@ _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;
> > + struct request *req;
> >
> > mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
> > scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
> > @@ -4715,6 +4717,21 @@ _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;
> > + req = scmd->request;
> > + if (unlikely(sector_sz && req && (req->cmd_type == REQ_TYPE_FS) &&
> > + (xfer_cnt % sector_sz))) {
> >
> > Maybe a bit zealous on the sanity checking...
>
> A bit...? heheh
> Too much I'd say. Since this is dealing with a bogus FW scenario, I
> found more safe to check everything...of course we can remove checks if
> it's sure req isn't NULL ever. The sector_sz check is avoiding
> degenerate cases, since our division below.
>
>
> >
> > + sdev_printk(KERN_INFO, scmd->device,
> > + "unaligned partial completion avoided (xfer_cnt=%u, sector_sz=%u)\n",
> > + xfer_cnt, sector_sz);
> > + xfer_cnt = (xfer_cnt / sector_sz) * sector_sz;
> >
> > Not so keen on divisions. xfer_cnt = round_down(xfer_cnt, sector_sz), maybe?
> >
>
> Martin, I might be completely wrong here (please correct me if this is
> the case), but isn't C standard integer division a truncation that acts
> like a round down? I checked (what I think is) the specification of C
> language (ISO/IEC 9899:1999), and it seems the division proposed by Ram
> Pai is accurate in this case. Also, both variables are unsigned.
Guilherme, Its better to use round_down() instead of division. Among
other things it saves a few nanoseconds.
RP
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mpt3sas: Force request partial completion alignment
2017-01-26 17:02 ` Ram Pai
@ 2017-01-26 18:37 ` Guilherme G. Piccoli
0 siblings, 0 replies; 6+ messages in thread
From: Guilherme G. Piccoli @ 2017-01-26 18:37 UTC (permalink / raw)
To: Ram Pai
Cc: Martin K. Petersen, linux-scsi, MPT-FusionLinux.pdl,
sathya.prakash, chaitra.basappa, suganath-prabu.subramani,
sreekanth.reddy, brking, mauricfo
On 01/26/2017 03:02 PM, Ram Pai wrote:
> On Thu, Jan 26, 2017 at 11:31:53AM -0200, Guilherme G. Piccoli wrote:
>> On 01/25/2017 09:46 PM, Martin K. Petersen wrote:
>>>>>>>> "Guilherme" == Guilherme G Piccoli <gpiccoli@linux.vnet.ibm.com> writes:
>>>
>>> Hi Guilherme,
>>
>> Hi Martin, thanks for the review!
>>
>>
>>>
>>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>>> index 75f3fce..e52c942 100644
>>> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>>> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>>> @@ -4657,6 +4657,8 @@ _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;
>>> + struct request *req;
>>>
>>> mpi_reply = mpt3sas_base_get_reply_virt_addr(ioc, reply);
>>> scmd = _scsih_scsi_lookup_get_clear(ioc, smid);
>>> @@ -4715,6 +4717,21 @@ _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;
>>> + req = scmd->request;
>>> + if (unlikely(sector_sz && req && (req->cmd_type == REQ_TYPE_FS) &&
>>> + (xfer_cnt % sector_sz))) {
>>>
>>> Maybe a bit zealous on the sanity checking...
>>
>> A bit...? heheh
>> Too much I'd say. Since this is dealing with a bogus FW scenario, I
>> found more safe to check everything...of course we can remove checks if
>> it's sure req isn't NULL ever. The sector_sz check is avoiding
>> degenerate cases, since our division below.
>>
>>
>>>
>>> + sdev_printk(KERN_INFO, scmd->device,
>>> + "unaligned partial completion avoided (xfer_cnt=%u, sector_sz=%u)\n",
>>> + xfer_cnt, sector_sz);
>>> + xfer_cnt = (xfer_cnt / sector_sz) * sector_sz;
>>>
>>> Not so keen on divisions. xfer_cnt = round_down(xfer_cnt, sector_sz), maybe?
>>>
>>
>> Martin, I might be completely wrong here (please correct me if this is
>> the case), but isn't C standard integer division a truncation that acts
>> like a round down? I checked (what I think is) the specification of C
>> language (ISO/IEC 9899:1999), and it seems the division proposed by Ram
>> Pai is accurate in this case. Also, both variables are unsigned.
>
> Guilherme, Its better to use round_down() instead of division. Among
> other things it saves a few nanoseconds.
Thanks Ram and Martin for the suggestion and explanation. I just sent a V3.
Cheers,
Guilherme
>
> RP
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-26 18:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-24 15:47 [PATCH v2] mpt3sas: Force request partial completion alignment Guilherme G. Piccoli
2017-01-25 4:46 ` Sreekanth Reddy
2017-01-25 23:46 ` Martin K. Petersen
2017-01-26 13:31 ` Guilherme G. Piccoli
2017-01-26 17:02 ` Ram Pai
2017-01-26 18:37 ` Guilherme G. Piccoli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.