Skip to contentSkip to conversation

bio-integrity: add "bip_max_vcnt" into struct bio_integrity_payload

RFC PATCH In review Gu Zheng 2 participants 4 messages Jun 27, 2014 – Jun 30, 2014 Block layer

lore
Share & tools
lore

Gu Zheng

author
#0 lore raw

RFC PATCH 1/1 bio-integrity: add "bip_max_vcnt" into struct bio_integrity_payload

Commit "08778795"(block: Fix nr_vecs for inline integrity vectors) from Martin introduces the function bip_integrity_vecs(get the useful vectors) to fix the issue about nr_vecs for inline integrity vectors that reported by David Milburn.
But it seems that bip_integrity_vecs() will return the wrong number if the bio is not based on any bio_set for some reason(bio->bi_pool == NULL), because in that case, the bip_inline_vecs[0] is malloced directly. So here we add the bip_max_vcnt to record the count of vector slots, and cleanup the function bip_integrity_vecs().

---
 block/bio-integrity.c |   12 +++---------
 include/linux/bio.h   |    1 +
 2 files changed, 4 insertions(+), 9 deletions(-)
block/bio-integrity.c +3 -9
index 9e24106..bc423f7b 100644
@@ -70,8 +70,10 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
 					  bs->bvec_integrity_pool);
 		if (!bip->bip_vec)
 			goto err;
+		bip->bip_max_vcnt = bvec_nr_vecs(idx);
 	} else {
 		bip->bip_vec = bip->bip_inline_vecs;
+		bip->bip_max_vcnt = inline_vecs;
 	}
 
 	bip->bip_slab = idx;
@@ -114,14 +116,6 @@ void bio_integrity_free(struct bio *bio)
 }
 EXPORT_SYMBOL(bio_integrity_free);
 
-static inline unsigned int bip_integrity_vecs(struct bio_integrity_payload *bip)
-{
-	if (bip->bip_slab == BIO_POOL_NONE)
-		return BIP_INLINE_VECS;
-
-	return bvec_nr_vecs(bip->bip_slab);
-}
-
 /**
  * bio_integrity_add_page - Attach integrity metadata
  * @bio:	bio to update
@@ -137,7 +131,7 @@ int bio_integrity_add_page(struct bio *bio, struct page *page,
 	struct bio_integrity_payload *bip = bio->bi_integrity;
 	struct bio_vec *iv;
 
-	if (bip->bip_vcnt >= bip_integrity_vecs(bip)) {
+	if (bip->bip_vcnt >= bip->bip_max_vcnt) {
 		printk(KERN_ERR "%s: bip_vec full\n", __func__);
 		return 0;
 	}
include/linux/bio.h +1 -0
index 5a64576..1946d72 100644
@@ -299,6 +299,7 @@ struct bio_integrity_payload {
 
 	unsigned short		bip_slab;	/* slab the bip came from */
 	unsigned short		bip_vcnt;	/* # of integrity bio_vecs */
+	unsigned short		bip_max_vcnt;	/* integrity bio_vec slots */
 	unsigned		bip_owns_buf:1;	/* should free bip_buf */
 
 	struct work_struct	bip_work;	/* I/O completion */
Signature · 1 line
1.7.7
#1 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Gu" == Gu Zheng <[email protected]> writes:

[Sorry about the delay. I'm on vacation right now.]

Gu> But it seems that bip_integrity_vecs() will return the wrong number Gu> if the bio is not based on any bio_set for some reason(bio->bi_pool Gu> == NULL), because in that case, the bip_inline_vecs[0] is malloced Gu> directly. So here we add the bip_max_vcnt to record the count of Gu> vector slots, and cleanup the function bip_integrity_vecs().

I'm in agreement with your fix.

However, I'm still not sure what the use case is for bios without an associated bioset. I do not see any callers that pass in a NULL bioset.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
author
#2 lore raw

Hi Martin,

Quoted 11 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Gu" == Gu Zheng <[email protected]> writes:

[Sorry about the delay. I'm on vacation right now.]

Gu> But it seems that bip_integrity_vecs() will return the wrong number Gu> if the bio is not based on any bio_set for some reason(bio->bi_pool Gu> == NULL), because in that case, the bip_inline_vecs[0] is malloced Gu> directly. So here we add the bip_max_vcnt to record the count of Gu> vector slots, and cleanup the function bip_integrity_vecs().

I'm in agreement with your fix.

Thanks.

Quoted 3 lines

However, I'm still not sure what the use case is for bios without an associated bioset. I do not see any callers that pass in a NULL bioset.

Please refer to bio_kmalloc()--alloc bio via kmalloc.
And IMO, the API(e.g. bio_alloc_bioset) is EXPORT, any guys(including some out of mainline code, some special drivers) can alloc a bio via kmalloc without based on associated bioset.

Regards,
Gu

Quoted 1 line
#3 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Gu" == Gu Zheng <[email protected]> writes:

Gu> Please refer to bio_kmalloc()--alloc bio via kmalloc.

OK, missed that one because it was in bio.h.

In that case we absolutely need your patch.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering