[PATCH 0/2] block: enable multi-page bvec for bio integrity

[PATCH 0/2] block: enable multi-page bvec for bio integrity

[Ming Lei]

Hi,

The 1st patch enables multi-page bvec for bio integrity meta data.

The 2nd patch simplifies bio_integrity_prep() via multi-page bvec.

Thanks,

Ming Lei (2):
  block: integrity: enable multi-page bvec for bio integrity
  block: integrity: simplify bio_integrity_prep

 block/bio-integrity.c | 55 ++++++++++++++++-----------------------------------
 block/bio.c           | 25 -----------------------
 block/blk.h           | 26 ++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 63 deletions(-)

-- 
2.9.5

[PATCH 1/2] block: integrity: enable multi-page bvec for bio integrity

[Ming Lei]

bio integrity's bvec is same with the normal bvec, and it is pretty
straight-forward to enable multi-page bvec for bio integrity because
bio_for_each_integrity_vec() is based on for_each_bvec() which can
handle multi-page bvec well.

The integrity buffer can't be very big, for example, the max sectors
for one bio is 2560, one sector may take at most 8bytes for integrity
info, so the max size of integrity buffer is just 20k(<=5 pages). So
not like normal multi-page bvec convertion, this patch doesn't switch
blk_rq_map_integrity_sg() to iterate over multi-page bvec.

Cc: Martin K . Petersen <martin.petersen@oracle.com>,
Cc: Christoph Hellwig <hch@lst.de>,
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bio-integrity.c | 18 ++++++++++++------
 block/bio.c           | 25 -------------------------
 block/blk.h           | 26 ++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 1b633a3526d4..ba9d145315ed 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -143,18 +143,24 @@ int bio_integrity_add_page(struct bio *bio, struct page *page,
 		return 0;
 	}
 
-	iv = bip->bip_vec + bip->bip_vcnt;
+	if (bip->bip_vcnt) {
+		iv = bip->bip_vec + bip->bip_vcnt - 1;
 
-	if (bip->bip_vcnt &&
-	    bvec_gap_to_prev(bio->bi_disk->queue,
-			     &bip->bip_vec[bip->bip_vcnt - 1], offset))
-		return 0;
+		if (bvec_gap_to_prev(bio->bi_disk->queue, iv, offset))
+			return 0;
 
+		if (page_is_mergeable(iv, page, len, offset, false)) {
+			iv->bv_len += len;
+			goto done;
+		}
+	}
+
+	iv = bip->bip_vec + bip->bip_vcnt;
 	iv->bv_page = page;
 	iv->bv_len = len;
 	iv->bv_offset = offset;
 	bip->bip_vcnt++;
-
+ done:
 	return len;
 }
 EXPORT_SYMBOL(bio_integrity_add_page);
diff --git a/block/bio.c b/block/bio.c
index c81ed2dfee53..89d1aa119a33 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -647,31 +647,6 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs)
 }
 EXPORT_SYMBOL(bio_clone_fast);
 
-static inline bool page_is_mergeable(const struct bio_vec *bv,
-		struct page *page, unsigned int len, unsigned int off,
-		bool same_page)
-{
-	phys_addr_t vec_end_addr = page_to_phys(bv->bv_page) +
-		bv->bv_offset + bv->bv_len - 1;
-	phys_addr_t page_addr = page_to_phys(page);
-
-	if (vec_end_addr + 1 != page_addr + off)
-		return false;
-	if (xen_domain() && !xen_biovec_phys_mergeable(bv, page))
-		return false;
-
-	if ((vec_end_addr & PAGE_MASK) != page_addr) {
-		if (same_page)
-			return false;
-		if (pfn_to_page(PFN_DOWN(vec_end_addr)) + 1 != page)
-			return false;
-	}
-
-	WARN_ON_ONCE(same_page && (len + off) > PAGE_SIZE);
-
-	return true;
-}
-
 /*
  * Check if the @page can be added to the current segment(@bv), and make
  * sure to call it only if page_is_mergeable(@bv, @page) is true
diff --git a/block/blk.h b/block/blk.h
index e27fd1512e4b..1df54fa04edf 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -324,4 +324,30 @@ void blk_queue_free_zone_bitmaps(struct request_queue *q);
 static inline void blk_queue_free_zone_bitmaps(struct request_queue *q) {}
 #endif
 
+static inline bool page_is_mergeable(const struct bio_vec *bv,
+		struct page *page, unsigned int len, unsigned int off,
+		bool same_page)
+{
+	phys_addr_t vec_end_addr = page_to_phys(bv->bv_page) +
+		bv->bv_offset + bv->bv_len - 1;
+	phys_addr_t page_addr = page_to_phys(page);
+
+	if (vec_end_addr + 1 != page_addr + off)
+		return false;
+	if (xen_domain() && !xen_biovec_phys_mergeable(bv, page))
+		return false;
+
+	if ((vec_end_addr & PAGE_MASK) != page_addr) {
+		if (same_page)
+			return false;
+		if (pfn_to_page(PFN_DOWN(vec_end_addr)) + 1 != page)
+			return false;
+	}
+
+	WARN_ON_ONCE(same_page && (len + off) > PAGE_SIZE);
+
+	return true;
+}
+
+
 #endif /* BLK_INTERNAL_H */
-- 
2.9.5

Re: [PATCH 1/2] block: integrity: enable multi-page bvec for bio integrity

[Christoph Hellwig]

On Tue, Apr 23, 2019 at 03:15:49PM +0800, Ming Lei wrote:
> +	if (bip->bip_vcnt) {
> +		iv = bip->bip_vec + bip->bip_vcnt - 1;
>  
> +		if (bvec_gap_to_prev(bio->bi_disk->queue, iv, offset))
> +			return 0;
>  
> +		if (page_is_mergeable(iv, page, len, offset, false)) {
> +			iv->bv_len += len;
> +			goto done;
> +		}
> +	}
> +
> +	iv = bip->bip_vec + bip->bip_vcnt;
>  	iv->bv_page = page;
>  	iv->bv_len = len;
>  	iv->bv_offset = offset;
>  	bip->bip_vcnt++;
> -
> + done:
>  	return len;

Just do an early return instead of the goto..

Otherwise this looks fine:

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

Re: [PATCH 1/2] block: integrity: enable multi-page bvec for bio integrity

[Martin K. Petersen]

Ming,

I'm traveling today and probably won't be able to take a closer look
until tomorrow. But from a quick glance this looks OK.

> The integrity buffer can't be very big, for example, the max sectors
> for one bio is 2560, one sector may take at most 8bytes for integrity
> info, so the max size of integrity buffer is just 20k(<=5 pages).

Just a comment on your rationale about 5 pages.

buffer_head submissions have traditionally been small, and depending on
your choice of allocator, new allocations would grow backwards in
memory. So there were several common I/O patterns that produced a
single, non-mergeable 8 byte integrity metadata allocation for every 512
bytes of data in the I/O. It's a pathological corner case. Just make
sure it's something you handle when you muck with this. ext[23] and dd
to the block device used to be able to reproduce this scenario easily.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/2] block: integrity: enable multi-page bvec for bio integrity

[Ming Lei]

On Wed, Apr 24, 2019 at 09:14:36AM -0400, Martin K. Petersen wrote:
> 
> Ming,
> 
> I'm traveling today and probably won't be able to take a closer look
> until tomorrow. But from a quick glance this looks OK.
> 
> > The integrity buffer can't be very big, for example, the max sectors
> > for one bio is 2560, one sector may take at most 8bytes for integrity
> > info, so the max size of integrity buffer is just 20k(<=5 pages).
> 
> Just a comment on your rationale about 5 pages.
> 
> buffer_head submissions have traditionally been small, and depending on
> your choice of allocator, new allocations would grow backwards in
> memory. So there were several common I/O patterns that produced a
> single, non-mergeable 8 byte integrity metadata allocation for every 512
> bytes of data in the I/O. It's a pathological corner case. Just make
> sure it's something you handle when you muck with this. ext[23] and dd
> to the block device used to be able to reproduce this scenario easily.

Yeah, you are right.

I have realized that 5 pages aren't correct, and it should be one
protection data segment for each bio usually.

Thanks,
Ming

[PATCH 2/2] block: integrity: simplify bio_integrity_prep

[Ming Lei]

Now bio integrity is capable of dealing with multi-page bvec,
also bio_integrity_add_page() can add physically contiguous pages
into bip once, so avoid to loop over page by page.

Cc: Martin K . Petersen <martin.petersen@oracle.com>,
Cc: Christoph Hellwig <hch@lst.de>,
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/bio-integrity.c | 37 +++++--------------------------------
 1 file changed, 5 insertions(+), 32 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index ba9d145315ed..61edc79737bd 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -223,9 +223,7 @@ bool bio_integrity_prep(struct bio *bio)
 	struct blk_integrity *bi = blk_get_integrity(bio->bi_disk);
 	struct request_queue *q = bio->bi_disk->queue;
 	void *buf;
-	unsigned long start, end;
-	unsigned int len, nr_pages;
-	unsigned int bytes, offset, i;
+	unsigned int len;
 	unsigned int intervals;
 	blk_status_t status;
 
@@ -262,12 +260,8 @@ bool bio_integrity_prep(struct bio *bio)
 		goto err_end_io;
 	}
 
-	end = (((unsigned long) buf) + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	start = ((unsigned long) buf) >> PAGE_SHIFT;
-	nr_pages = end - start;
-
 	/* Allocate bio integrity payload and integrity vectors */
-	bip = bio_integrity_alloc(bio, GFP_NOIO, nr_pages);
+	bip = bio_integrity_alloc(bio, GFP_NOIO, 1);
 	if (IS_ERR(bip)) {
 		printk(KERN_ERR "could not allocate data integrity bioset\n");
 		kfree(buf);
@@ -283,30 +277,9 @@ bool bio_integrity_prep(struct bio *bio)
 		bip->bip_flags |= BIP_IP_CHECKSUM;
 
 	/* Map it */
-	offset = offset_in_page(buf);
-	for (i = 0 ; i < nr_pages ; i++) {
-		int ret;
-		bytes = PAGE_SIZE - offset;
-
-		if (len <= 0)
-			break;
-
-		if (bytes > len)
-			bytes = len;
-
-		ret = bio_integrity_add_page(bio, virt_to_page(buf),
-					     bytes, offset);
-
-		if (ret == 0)
-			return false;
-
-		if (ret < bytes)
-			break;
-
-		buf += bytes;
-		len -= bytes;
-		offset = 0;
-	}
+	if (bio_integrity_add_page(bio, virt_to_page(buf), len,
+				   offset_in_page(buf)) != len)
+		return false;
 
 	/* Auto-generate integrity metadata if this is a write */
 	if (bio_data_dir(bio) == WRITE) {
-- 
2.9.5

Re: [PATCH 2/2] block: integrity: simplify bio_integrity_prep

[Christoph Hellwig]

On Tue, Apr 23, 2019 at 03:15:50PM +0800, Ming Lei wrote:
> Now bio integrity is capable of dealing with multi-page bvec,
> also bio_integrity_add_page() can add physically contiguous pages
> into bip once, so avoid to loop over page by page.

Looks good,

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