| Message ID | 20181205023110.20162-1-martin.petersen@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 60a89a3ce0cce515dc663bc1b45ac89202ad6c79 |
| Headers | show |
| Series | scsi: t10-pi: Return correct ref tag when queue has no integrity profile | expand |
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.
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.
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.
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.
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
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 > > > . >
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;
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(-)