[PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile

[PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile

[Martin K. Petersen]

Commit ddd0bc756983 ("block: move ref_tag calculation func to the block
layer") moved ref tag calculation from SCSI to a library function. However,
this change broke returning the correct ref tag for devices operating in
DIF mode since these do not have an associated block integrity profile.
This in turn caused read/write failures on PI-formatted disks attached to
an mpt3sas controller.

Fixes: ddd0bc756983 ("block: move ref_tag calculation func to the block layer")
Cc: stable@vger.kernel.org # 4.19+
Reported-by: John Garry <john.garry@huawei.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 include/linux/t10-pi.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/t10-pi.h b/include/linux/t10-pi.h
index b9626aa7e90c..3e2a80cc7b56 100644
--- a/include/linux/t10-pi.h
+++ b/include/linux/t10-pi.h
@@ -39,12 +39,13 @@ struct t10_pi_tuple {
 
 static inline u32 t10_pi_ref_tag(struct request *rq)
 {
+	unsigned int shift = ilog2(queue_logical_block_size(rq->q));
+
 #ifdef CONFIG_BLK_DEV_INTEGRITY
-	return blk_rq_pos(rq) >>
-		(rq->q->integrity.interval_exp - 9) & 0xffffffff;
-#else
-	return -1U;
+	if (rq->q->integrity.interval_exp)
+		shift = rq->q->integrity.interval_exp;
 #endif
+	return blk_rq_pos(rq) >> (shift - SECTOR_SHIFT) & 0xffffffff;
 }
 
 extern const struct blk_integrity_profile t10_pi_type1_crc;
-- 
2.19.2

Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile

[Bart Van Assche]

On 12/4/18 6:31 PM, Martin K. Petersen wrote:
> Commit ddd0bc756983 ("block: move ref_tag calculation func to the block
> layer") moved ref tag calculation from SCSI to a library function. However,
> this change broke returning the correct ref tag for devices operating in
> DIF mode since these do not have an associated block integrity profile.
> This in turn caused read/write failures on PI-formatted disks attached to
> an mpt3sas controller.
> 
> Fixes: ddd0bc756983 ("block: move ref_tag calculation func to the block layer")
> Cc: stable@vger.kernel.org # 4.19+
> Reported-by: John Garry <john.garry@huawei.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>   include/linux/t10-pi.h | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/t10-pi.h b/include/linux/t10-pi.h
> index b9626aa7e90c..3e2a80cc7b56 100644
> --- a/include/linux/t10-pi.h
> +++ b/include/linux/t10-pi.h
> @@ -39,12 +39,13 @@ struct t10_pi_tuple {
>   
>   static inline u32 t10_pi_ref_tag(struct request *rq)
>   {
> +	unsigned int shift = ilog2(queue_logical_block_size(rq->q));
> +
>   #ifdef CONFIG_BLK_DEV_INTEGRITY
> -	return blk_rq_pos(rq) >>
> -		(rq->q->integrity.interval_exp - 9) & 0xffffffff;
> -#else
> -	return -1U;
> +	if (rq->q->integrity.interval_exp)
> +		shift = rq->q->integrity.interval_exp;
>   #endif
> +	return blk_rq_pos(rq) >> (shift - SECTOR_SHIFT) & 0xffffffff;
>   }

Since the return value of this function is 'u32', can the ' & 
0xffffffff' be left out?

Thanks,

Bart.

Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile

[Martin K. Petersen]

Hi Bart,

> Since the return value of this function is 'u32', can the ' &
> 0xffffffff' be left out?

Absolutely, and I almost zapped it. However, I decided to leave it to
emphasize the point that the reference tag is truncated to a 32-bit
value. To me, this is more obvious than having to backtrack and spot the
u32 in the function definition. I generally appreciate some sort of
commentary around a return statement if the value deviates from the
ordinary.

The parentheses around the shift value irk me but had to leave those in
place to silence gcc.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile

[Bart Van Assche]

On 12/5/18 6:04 AM, Martin K. Petersen wrote:
>> Since the return value of this function is 'u32', can the ' &
>> 0xffffffff' be left out?
> 
> Absolutely, and I almost zapped it. However, I decided to leave it to
> emphasize the point that the reference tag is truncated to a 32-bit
> value. To me, this is more obvious than having to backtrack and spot the
> u32 in the function definition. I generally appreciate some sort of
> commentary around a return statement if the value deviates from the
> ordinary.
> 
> The parentheses around the shift value irk me but had to leave those in
> place to silence gcc.

Hi Martin,

Had you considered to use lower_32_bits() instead of "0xffffffff"? That 
would to avoid that reviewers have to count the 'f'-s to verify 
correctness of t10_pi_ref_tag().

Thanks,

Bart.

Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile

[Martin K. Petersen]

Bart,

> Had you considered to use lower_32_bits() instead of "0xffffffff"?
> That would to avoid that reviewers have to count the 'f'-s to verify
> correctness of t10_pi_ref_tag().

I hadn't. I guess I tend to think of lower_32_bits() as something you do
to pointers, not to block numbers.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile

[John Garry]

On 06/12/2018 04:17, Martin K. Petersen wrote:

+

>
> Bart,
>
>> Had you considered to use lower_32_bits() instead of "0xffffffff"?
>> That would to avoid that reviewers have to count the 'f'-s to verify
>> correctness of t10_pi_ref_tag().
>
> I hadn't. I guess I tend to think of lower_32_bits() as something you do
> to pointers, not to block numbers.
>

Xiang Chen tested the patch successfully. I'll let him provide tested-by 
tag.

Thanks,
John

Re: [PATCH] scsi: t10-pi: Return correct ref tag when queue has no integrity profile

[chenxiang (M)]

Hi,
在 2018/12/6 20:04, John Garry 写道:
> On 06/12/2018 04:17, Martin K. Petersen wrote:
>
> +
>
>>
>> Bart,
>>
>>> Had you considered to use lower_32_bits() instead of "0xffffffff"?
>>> That would to avoid that reviewers have to count the 'f'-s to verify
>>> correctness of t10_pi_ref_tag().
>>
>> I hadn't. I guess I tend to think of lower_32_bits() as something you do
>> to pointers, not to block numbers.
>>
>
> Xiang Chen tested the patch successfully. I'll let him provide 
> tested-by tag.

I have tested the patch with running fio on 3008 (on our ARM64 board 
with 4 DIF disks and 8 non-DIF disks), and now it works well.
Tested-by: Xiang Chen <chenxiang66@hisilicon.com>

>
> Thanks,
> John
>
>
> .
>