[PATCH 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[PATCH 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[Damien Le Moal]

The ZBC_IN command (REPORT ZONES) reply length is always a multiple of
64B and thus may not be aligned on the device LBA size.
For this command, retry due to the unaligned completion length is
incorrect so do not check alignment of the reply length.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 0b5b423..f04b872 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -4723,8 +4723,9 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
 	 * 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)) {
+	if (scmd->request->cmd_type == REQ_TYPE_FS &&
+	    scmd->cmnd[0] != ZBC_IN &&
+	    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);
-- 
2.9.3

Re: [PATCH 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[Bart Van Assche]

On Mon, 2017-02-13 at 14:11 +0900, Damien Le Moal wrote:
> The ZBC_IN command (REPORT ZONES) reply length is always a multiple of
> 64B and thus may not be aligned on the device LBA size.
> For this command, retry due to the unaligned completion length is
> incorrect so do not check alignment of the reply length.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 0b5b423..f04b872 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -4723,8 +4723,9 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
>  	 * 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)) {
> +	if (scmd->request->cmd_type == REQ_TYPE_FS &&
> +	    scmd->cmnd[0] != ZBC_IN &&
> +	    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);

Hello Damien,

What software generated a ZBC_IN request with type REQ_TYPE_FS? Shouldn't
all ZBC_IN requests be assigned type REQ_TYPE_BLOCK_PC?

Thanks,

Bart.

Re: [PATCH 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[Damien Le Moal]

Bart,

On 2/14/17 03:57, Bart Van Assche wrote:
> On Mon, 2017-02-13 at 14:11 +0900, Damien Le Moal wrote:
>> The ZBC_IN command (REPORT ZONES) reply length is always a multiple of
>> 64B and thus may not be aligned on the device LBA size.
>> For this command, retry due to the unaligned completion length is
>> incorrect so do not check alignment of the reply length.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>> index 0b5b423..f04b872 100644
>> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
>> @@ -4723,8 +4723,9 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
>>  	 * 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)) {
>> +	if (scmd->request->cmd_type == REQ_TYPE_FS &&
>> +	    scmd->cmnd[0] != ZBC_IN &&
>> +	    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);
> 
> Hello Damien,
> 
> What software generated a ZBC_IN request with type REQ_TYPE_FS? Shouldn't
> all ZBC_IN requests be assigned type REQ_TYPE_BLOCK_PC?

Any application that issues a BLKREPORTZONE ioctl (e.g. mkfs.f2fs or
libzbc tools) or kernel component calling blkdev_report_zones (e.g.
f2fs) will generate one or more REQ_OP_ZONE_REPORT BIO which translate
into a REQ_TYPE_FS requests for the command ZBC_IN. That command does
not operate on LBAs. The transfer length of the request can be any
length larger than 64B and the reply length will be at least 64B and
aligned on a multiple of 64.

The patch "mpt3sas: Force request partial completion alignment" applied
last week forgot this case, and frankly, I did too despite having looked
at it.

Martin,

We really need this fix to go into 4.10. Without it, the REPORT
ZONES/ZBC_IN command can be retried forever by the HBA.

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 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[Bart Van Assche]

On Tue, 2017-02-14 at 12:45 +0900, Damien Le Moal wrote:
> On 2/14/17 03:57, Bart Van Assche wrote:
> > On Mon, 2017-02-13 at 14:11 +0900, Damien Le Moal wrote:
> > > The ZBC_IN command (REPORT ZONES) reply length is always a multiple of
> > > 64B and thus may not be aligned on the device LBA size.
> > > For this command, retry due to the unaligned completion length is
> > > incorrect so do not check alignment of the reply length.
> > > 
> > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> > > ---
> > >  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > > index 0b5b423..f04b872 100644
> > > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> > > @@ -4723,8 +4723,9 @@ _scsih_io_done(struct MPT3SAS_ADAPTER *ioc, u16 smid, u8 msix_index, u32 reply)
> > >  	 * 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)) {
> > > +	if (scmd->request->cmd_type == REQ_TYPE_FS &&
> > > +	    scmd->cmnd[0] != ZBC_IN &&
> > > +	    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);
> > 
> > What software generated a ZBC_IN request with type REQ_TYPE_FS? Shouldn't
> > all ZBC_IN requests be assigned type REQ_TYPE_BLOCK_PC?
> 
> Any application that issues a BLKREPORTZONE ioctl (e.g. mkfs.f2fs or
> libzbc tools) or kernel component calling blkdev_report_zones (e.g.
> f2fs) will generate one or more REQ_OP_ZONE_REPORT BIO which translate
> into a REQ_TYPE_FS requests for the command ZBC_IN. That command does
> not operate on LBAs. The transfer length of the request can be any
> length larger than 64B and the reply length will be at least 64B and
> aligned on a multiple of 64.
> 
> The patch "mpt3sas: Force request partial completion alignment" applied
> last week forgot this case, and frankly, I did too despite having looked
> at it.

Hello Damien,

How about modifying the block layer core such that it sets REQ_TYPE_BLOCK_PC
for REQ_OP_ZONE_REPORT requests? Would that be a valid alternative to the
above patch?

Thanks,

Bart.

Re: [PATCH 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[Damien Le Moal]

Bart,

On 2/14/17 12:59, Bart Van Assche wrote:
>>> What software generated a ZBC_IN request with type REQ_TYPE_FS? Shouldn't
>>> all ZBC_IN requests be assigned type REQ_TYPE_BLOCK_PC?
>>
>> Any application that issues a BLKREPORTZONE ioctl (e.g. mkfs.f2fs or
>> libzbc tools) or kernel component calling blkdev_report_zones (e.g.
>> f2fs) will generate one or more REQ_OP_ZONE_REPORT BIO which translate
>> into a REQ_TYPE_FS requests for the command ZBC_IN. That command does
>> not operate on LBAs. The transfer length of the request can be any
>> length larger than 64B and the reply length will be at least 64B and
>> aligned on a multiple of 64.
>>
>> The patch "mpt3sas: Force request partial completion alignment" applied
>> last week forgot this case, and frankly, I did too despite having looked
>> at it.
> 
> Hello Damien,
> 
> How about modifying the block layer core such that it sets REQ_TYPE_BLOCK_PC
> for REQ_OP_ZONE_REPORT requests? Would that be a valid alternative to the
> above patch?

I think so. But my understanding of REQ_TYPE_BLOCK_PC is that it is the
type for requests issued internally (scsi_execute) of from things like
the SG driver, so in essence, all requests not derived from a BIO... Is
this correct ? If yes, then setting the BLOCK_TYPE_PC for
REQ_OP_ZONE_REPORT (and REQ_OP_ZONE_RESET while at it) would break this
model, wouldn't it ?

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 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[Bart Van Assche]

On Tue, 2017-02-14 at 13:42 +0900, Damien Le Moal wrote:
> I think so. But my understanding of REQ_TYPE_BLOCK_PC is that it is the
> type for requests issued internally (scsi_execute) of from things like
> the SG driver, so in essence, all requests not derived from a BIO... Is
> this correct ? If yes, then setting the BLOCK_TYPE_PC for
> REQ_OP_ZONE_REPORT (and REQ_OP_ZONE_RESET while at it) would break this
> model, wouldn't it ?

Hello Damien,

I think we want to keep the knowledge of which requests have a request size
that should be a multiple of the logical block size in the block layer core
or in the SCSI core but not in the mpt3sas driver. But I'm not sure what the
best approach is to do that. Should we use the request type, should we add a
new request attribute or should we add a new function?

Bart.

Re: [PATCH 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[Damien Le Moal]

Bart,

On 2/14/17 14:11, Bart Van Assche wrote:
> On Tue, 2017-02-14 at 13:42 +0900, Damien Le Moal wrote:
>> I think so. But my understanding of REQ_TYPE_BLOCK_PC is that it is the
>> type for requests issued internally (scsi_execute) of from things like
>> the SG driver, so in essence, all requests not derived from a BIO... Is
>> this correct ? If yes, then setting the BLOCK_TYPE_PC for
>> REQ_OP_ZONE_REPORT (and REQ_OP_ZONE_RESET while at it) would break this
>> model, wouldn't it ?
> 
> Hello Damien,
> 
> I think we want to keep the knowledge of which requests have a request size
> that should be a multiple of the logical block size in the block layer core
> or in the SCSI core but not in the mpt3sas driver. But I'm not sure what the
> best approach is to do that. Should we use the request type, should we add a
> new request attribute or should we add a new function?

I agree. But the mpt3sas patch that introduced the problem is to solve
problems with a buggy hardware in the first place... A quirck of some
sort. So do we really need such a big change just the report zones
exception ? (all other REQ_TYPE_FS commands either have no payload or
the payload size is LBA aligned)

Martin, James, Hannes,

Please advise ! 4.10 is introducing zoned block device support, and from
the first release that will be broken with mpt3sas HBAs if we do not
patch this. Any preferred approach ?

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 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[hch]

On Tue, Feb 14, 2017 at 02:21:37PM +0900, Damien Le Moal wrote:
> > I think we want to keep the knowledge of which requests have a request size
> > that should be a multiple of the logical block size in the block layer core
> > or in the SCSI core but not in the mpt3sas driver. But I'm not sure what the
> > best approach is to do that. Should we use the request type, should we add a
> > new request attribute or should we add a new function?
> 
> I agree. But the mpt3sas patch that introduced the problem is to solve
> problems with a buggy hardware in the first place... A quirck of some
> sort. So do we really need such a big change just the report zones
> exception ? (all other REQ_TYPE_FS commands either have no payload or
> the payload size is LBA aligned)
> 
> Martin, James, Hannes,
> 
> Please advise ! 4.10 is introducing zoned block device support, and from
> the first release that will be broken with mpt3sas HBAs if we do not
> patch this. Any preferred approach ?

I suspect we'll need to move the workaround to the SD driver.  While it's
an mpt bug, sd is the party that knows which kind of requests were
generated, so it's where we can fix up the length in the done callback.

Re: [PATCH 1/1] mpt3sas: Ignore unaligned completion length for ZBC_IN

[Damien Le Moal]

Christoph,

On 2/14/17 15:18, hch@lst.de wrote:
> On Tue, Feb 14, 2017 at 02:21:37PM +0900, Damien Le Moal wrote:
>>> I think we want to keep the knowledge of which requests have a request size
>>> that should be a multiple of the logical block size in the block layer core
>>> or in the SCSI core but not in the mpt3sas driver. But I'm not sure what the
>>> best approach is to do that. Should we use the request type, should we add a
>>> new request attribute or should we add a new function?
>>
>> I agree. But the mpt3sas patch that introduced the problem is to solve
>> problems with a buggy hardware in the first place... A quirck of some
>> sort. So do we really need such a big change just the report zones
>> exception ? (all other REQ_TYPE_FS commands either have no payload or
>> the payload size is LBA aligned)
>>
>> Martin, James, Hannes,
>>
>> Please advise ! 4.10 is introducing zoned block device support, and from
>> the first release that will be broken with mpt3sas HBAs if we do not
>> patch this. Any preferred approach ?
> 
> I suspect we'll need to move the workaround to the SD driver.  While it's
> an mpt bug, sd is the party that knows which kind of requests were
> generated, so it's where we can fix up the length in the done callback.

This makes sense. sd_done already sets report zone command resid to 0,
always. So it make sense to have the mpt3sas check after that, written
in a generic manner. I will send a patch shortly.

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