diff mbox series

block: t10-pi: Return correct ref tag when queue has no integrity profile

Message ID 20240704061515.282343-1-joshi.k@samsung.com (mailing list archive)
State New
Headers show
Series block: t10-pi: Return correct ref tag when queue has no integrity profile | expand

Commit Message

Kanchan Joshi July 4, 2024, 6:15 a.m. UTC 
From: Anuj Gupta <anuj20.g@samsung.com>

Commit <c6e56cf6b2e7> (block: move integrity information into
queue_limits) changed the ref tag calculation logic. It would break if
there is no integrity profile. This in turn causes read/write failures
for such cases.

Fixes: <c6e56cf6b2e7> (block: move integrity information into queue_limits)
Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
---
 include/linux/t10-pi.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig July 4, 2024, 6:26 a.m. UTC | #1 
On Thu, Jul 04, 2024 at 11:45:15AM +0530, Kanchan Joshi wrote:
> From: Anuj Gupta <anuj20.g@samsung.com>
> 
> Commit <c6e56cf6b2e7> (block: move integrity information into
> queue_limits) changed the ref tag calculation logic. It would break if
> there is no integrity profile. This in turn causes read/write failures
> for such cases.

Can you explain the scenario a bit better?  I guess this is for when
the drivers use PRACT to insert/strip PI because BLK_DEV_INTEGRITY
is disabled?

> 
> Fixes: <c6e56cf6b2e7> (block: move integrity information into queue_limits)

This is not the standard formatting for fixes tags.

>  
>  static inline u32 t10_pi_ref_tag(struct request *rq)
>  {
> -	unsigned int shift = rq->q->limits.integrity.interval_exp;
> +	unsigned int shift = ilog2(queue_logical_block_size(rq->q));
>  
> +	if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) &&
> +	    rq->q->limits.integrity.interval_exp)
> +		shift = rq->q->limits.integrity.interval_exp;
>  	return blk_rq_pos(rq) >> (shift - SECTOR_SHIFT) & 0xffffffff;

But this only works when the interval_exp equals the block size.

So I think the proper fix that not only addresses the regression, but
also the long standing buf for larger interval_exp is to make sure
interval_exp is always initialized, including for
!CONFIG_BLK_DEV_INTEGRITY.
Christoph Hellwig July 4, 2024, 6:32 a.m. UTC | #2 
Looking a bit more it seems like never actually merged the SCSI support
for an interval_exp smaller than the block size, although I'm pretty
sure I've seen Martins patches for it.  So i guess we should just go
with this patch (preferably with the fixed Fixes tag and a more detailed
commit log) and then sort this out later.
Jens Axboe July 4, 2024, 8:06 a.m. UTC | #3 
On Thu, 04 Jul 2024 11:45:15 +0530, Kanchan Joshi wrote:
> Commit <c6e56cf6b2e7> (block: move integrity information into
> queue_limits) changed the ref tag calculation logic. It would break if
> there is no integrity profile. This in turn causes read/write failures
> for such cases.
> 
> 

Applied, thanks!

[1/1] block: t10-pi: Return correct ref tag when queue has no integrity profile
      commit: 162e06871e6dcde861ef608e0c00a8b6a2d35d43

Best regards,
Martin K. Petersen July 5, 2024, 3:49 a.m. UTC | #4 
Christoph,

> Looking a bit more it seems like never actually merged the SCSI
> support for an interval_exp smaller than the block size, although I'm
> pretty sure I've seen Martins patches for it. So i guess we should
> just go with this patch (preferably with the fixed Fixes tag and a
> more detailed commit log) and then sort this out later.

The driver for this feature was being able to use disks with 4Kn sectors
and yet provide 8 bytes of PI for every 512 bytes of data for backwards
compatibility reasons.

I had a couple of drives which supported the feature and several OEMs
were requesting it. However, it turned out that using 512e drives was a
much more elegant solution to this particular problem.
Christoph Hellwig July 5, 2024, 5:05 a.m. UTC | #5 
On Thu, Jul 04, 2024 at 11:49:56PM -0400, Martin K. Petersen wrote:
> I had a couple of drives which supported the feature and several OEMs
> were requesting it. However, it turned out that using 512e drives was a
> much more elegant solution to this particular problem.

So should we just drop the internval_exp member for now?  It would
simplify things a bit, but more importantly we wouldn't have to fix
the !BLK_DEV_INTEGRITY case for strip/insert.
Martin K. Petersen July 5, 2024, 11:21 a.m. UTC | #6 
Christoph,

> So should we just drop the internval_exp member for now? It would
> simplify things a bit, but more importantly we wouldn't have to fix
> the !BLK_DEV_INTEGRITY case for strip/insert.

Yeah, I think that's fine. 512e won.
diff mbox series

Patch

diff --git a/include/linux/t10-pi.h b/include/linux/t10-pi.h
index 1773610010eb..2c59fe3efcd4 100644
--- a/include/linux/t10-pi.h
+++ b/include/linux/t10-pi.h
@@ -39,8 +39,11 @@  struct t10_pi_tuple {
 
 static inline u32 t10_pi_ref_tag(struct request *rq)
 {
-	unsigned int shift = rq->q->limits.integrity.interval_exp;
+	unsigned int shift = ilog2(queue_logical_block_size(rq->q));
 
+	if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) &&
+	    rq->q->limits.integrity.interval_exp)
+		shift = rq->q->limits.integrity.interval_exp;
 	return blk_rq_pos(rq) >> (shift - SECTOR_SHIFT) & 0xffffffff;
 }
 
@@ -61,8 +64,11 @@  static inline u64 lower_48_bits(u64 n)
 
 static inline u64 ext_pi_ref_tag(struct request *rq)
 {
-	unsigned int shift = rq->q->limits.integrity.interval_exp;
+	unsigned int shift = ilog2(queue_logical_block_size(rq->q));
 
+	if (IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY) &&
+	    rq->q->limits.integrity.interval_exp)
+		shift = rq->q->limits.integrity.interval_exp;
 	return lower_48_bits(blk_rq_pos(rq) >> (shift - SECTOR_SHIFT));
 }