[PATCH v2] t10-pi: reduce ref tag code duplication

[PATCH v2] t10-pi: reduce ref tag code duplication

[Caleb Sander Mateos]

t10_pi_ref_tag() and ext_pi_ref_tag() are identical except for the final
truncation of the ref tag to 32 or 48 bits. Factor out a helper
full_pi_ref_tag() to return the untruncated ref tag and use it in
t10_pi_ref_tag() and ext_pi_ref_tag().

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
Reviewed-by: Anuj Gupta <anuj20.g@samsung.com>
---
v2:
- Move full_pi_ref_tag() earlier (Christoph)
- Use lower_32_bits() (Christoph)

 include/linux/t10-pi.h | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/include/linux/t10-pi.h b/include/linux/t10-pi.h
index 2c59fe3efcd4..b6c2496866ea 100644
--- a/include/linux/t10-pi.h
+++ b/include/linux/t10-pi.h
@@ -2,10 +2,11 @@
 #ifndef _LINUX_T10_PI_H
 #define _LINUX_T10_PI_H
 
 #include <linux/types.h>
 #include <linux/blk-mq.h>
+#include <linux/wordpart.h>
 
 /*
  * A T10 PI-capable target device can be formatted with different
  * protection schemes.	Currently 0 through 3 are defined:
  *
@@ -23,10 +24,20 @@ enum t10_dif_type {
 	T10_PI_TYPE1_PROTECTION = 0x1,
 	T10_PI_TYPE2_PROTECTION = 0x2,
 	T10_PI_TYPE3_PROTECTION = 0x3,
 };
 
+static inline u64 full_pi_ref_tag(const struct request *rq)
+{
+	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);
+}
+
 /*
  * T10 Protection Information tuple.
  */
 struct t10_pi_tuple {
 	__be16 guard_tag;	/* Checksum */
@@ -37,16 +48,11 @@ struct t10_pi_tuple {
 #define T10_PI_APP_ESCAPE cpu_to_be16(0xffff)
 #define T10_PI_REF_ESCAPE cpu_to_be32(0xffffffff)
 
 static inline u32 t10_pi_ref_tag(struct request *rq)
 {
-	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;
+	return lower_32_bits(full_pi_ref_tag(rq));
 }
 
 struct crc64_pi_tuple {
 	__be64 guard_tag;
 	__be16 app_tag;
@@ -62,14 +68,9 @@ static inline u64 lower_48_bits(u64 n)
 	return n & ((1ull << 48) - 1);
 }
 
 static inline u64 ext_pi_ref_tag(struct request *rq)
 {
-	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));
+	return lower_48_bits(full_pi_ref_tag(rq));
 }
 
 #endif
-- 
2.45.2

Re: [PATCH v2] t10-pi: reduce ref tag code duplication

[Christoph Hellwig]

On Wed, Apr 15, 2026 at 03:08:47PM -0600, Caleb Sander Mateos wrote:
>  #ifndef _LINUX_T10_PI_H
>  #define _LINUX_T10_PI_H
>  
>  #include <linux/types.h>
>  #include <linux/blk-mq.h>
> +#include <linux/wordpart.h>

We must have already gotten this implicitly given that the code
already uses lower_48_bits.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] t10-pi: reduce ref tag code duplication

[Caleb Sander Mateos]

On Wed, Apr 15, 2026 at 10:18 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Apr 15, 2026 at 03:08:47PM -0600, Caleb Sander Mateos wrote:
> >  #ifndef _LINUX_T10_PI_H
> >  #define _LINUX_T10_PI_H
> >
> >  #include <linux/types.h>
> >  #include <linux/blk-mq.h>
> > +#include <linux/wordpart.h>
>
> We must have already gotten this implicitly given that the code
> already uses lower_48_bits.

lower_48_bits() is defined (and only used) in this header. Yes,
wordpart.h is already transitively included by the other headers, but
I think it's good practice for each file to explicitly include the
headers defining all the items it uses. It reduces the risk that
refactoring the other header files in the future will result in a
compilation error here by dropping the transitive include.

Thanks,
Caleb

Re: [PATCH v2] t10-pi: reduce ref tag code duplication

[Christoph Hellwig]

On Thu, Apr 16, 2026 at 08:38:07AM -0700, Caleb Sander Mateos wrote:
> On Wed, Apr 15, 2026 at 10:18 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Wed, Apr 15, 2026 at 03:08:47PM -0600, Caleb Sander Mateos wrote:
> > >  #ifndef _LINUX_T10_PI_H
> > >  #define _LINUX_T10_PI_H
> > >
> > >  #include <linux/types.h>
> > >  #include <linux/blk-mq.h>
> > > +#include <linux/wordpart.h>
> >
> > We must have already gotten this implicitly given that the code
> > already uses lower_48_bits.
> 
> lower_48_bits() is defined (and only used) in this header. Yes,
> wordpart.h is already transitively included by the other headers, but
> I think it's good practice for each file to explicitly include the
> headers defining all the items it uses. It reduces the risk that
> refactoring the other header files in the future will result in a
> compilation error here by dropping the transitive include.

In general pulling in a new header when no new user of it shows up is a
bit weird.  I don't want to hold the patch up on this, but there are
folks out there actually dropping not needed includes from headers as
it can significantly reduce compile time.  Now this is not a heavily
included header so it's unlikely to make a difference anyway.

Re: [PATCH v2] t10-pi: reduce ref tag code duplication

[Caleb Sander Mateos]

On Fri, Apr 17, 2026 at 12:56 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Apr 16, 2026 at 08:38:07AM -0700, Caleb Sander Mateos wrote:
> > On Wed, Apr 15, 2026 at 10:18 PM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > On Wed, Apr 15, 2026 at 03:08:47PM -0600, Caleb Sander Mateos wrote:
> > > >  #ifndef _LINUX_T10_PI_H
> > > >  #define _LINUX_T10_PI_H
> > > >
> > > >  #include <linux/types.h>
> > > >  #include <linux/blk-mq.h>
> > > > +#include <linux/wordpart.h>
> > >
> > > We must have already gotten this implicitly given that the code
> > > already uses lower_48_bits.
> >
> > lower_48_bits() is defined (and only used) in this header. Yes,
> > wordpart.h is already transitively included by the other headers, but
> > I think it's good practice for each file to explicitly include the
> > headers defining all the items it uses. It reduces the risk that
> > refactoring the other header files in the future will result in a
> > compilation error here by dropping the transitive include.
>
> In general pulling in a new header when no new user of it shows up is a
> bit weird.  I don't want to hold the patch up on this, but there are

I'm not sure what you mean by "no new user". There weren't previously
any uses of items from wordpart.h in t10-pi.h, but now it's using
lower_32_bits(). Personally, I think lower_32_bits() seems excessive
when we could just cast to u32, but I was following your suggestion...

> folks out there actually dropping not needed includes from headers as
> it can significantly reduce compile time.  Now this is not a heavily
> included header so it's unlikely to make a difference anyway.

Is a file being included by the preprocessor multiple times really
that expensive? I would have assumed it would be cached the first time
it was included.

Best,
Caleb

Re: [PATCH v2] t10-pi: reduce ref tag code duplication

[Keith Busch]

On Fri, Apr 17, 2026 at 08:34:30AM -0700, Caleb Sander Mateos wrote:
> On Fri, Apr 17, 2026 at 12:56 AM Christoph Hellwig <hch@infradead.org> wrote:
> > folks out there actually dropping not needed includes from headers as
> > it can significantly reduce compile time.  Now this is not a heavily
> > included header so it's unlikely to make a difference anyway.
> 
> Is a file being included by the preprocessor multiple times really
> that expensive? I would have assumed it would be cached the first time
> it was included.

Do modern compilers even incur a cost? They all have "multiple-include
optimization" features as far as i know. Not that we use "#prama once"
in kernel, but compilers look like they're smart enough to recognize the
#ifdef guards for the same purpose.

https://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html#The-Multiple-Include-Optimization

Re: [PATCH v2] t10-pi: reduce ref tag code duplication

[Jens Axboe]

On Wed, 15 Apr 2026 15:08:47 -0600, Caleb Sander Mateos wrote:
> t10_pi_ref_tag() and ext_pi_ref_tag() are identical except for the final
> truncation of the ref tag to 32 or 48 bits. Factor out a helper
> full_pi_ref_tag() to return the untruncated ref tag and use it in
> t10_pi_ref_tag() and ext_pi_ref_tag().

Applied, thanks!

[1/1] t10-pi: reduce ref tag code duplication
      commit: 2f5015461984caa8ebf265a60b22f38c94d9c70a

Best regards,
-- 
Jens Axboe