Re: [PATCH v3 2/3] block: verify data when endio

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1.1: Type: text/plain, Size: 2573 bytes --]

On Mar 29, 2019, at 8:40 AM, Bob Liu <bob.liu@oracle.com> wrote:
> 
> On 3/29/19 10:28 PM, Jens Axboe wrote:
>> On 3/29/19 8:23 AM, Bob Liu wrote:
>>> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
>>> index d66bf5f32610..e9f25f162138 100644
>>> --- a/include/linux/blk_types.h
>>> +++ b/include/linux/blk_types.h
>>> @@ -18,6 +18,7 @@ struct block_device;
>>> struct io_context;
>>> struct cgroup_subsys_state;
>>> typedef void (bio_end_io_t) (struct bio *);
>>> +typedef int (bio_verifier_t) (struct bio *);
>>> 
>>> /*
>>>  * Block error status values.  See block/blk-core:blk_errors for the details.
>>> @@ -187,6 +188,8 @@ struct bio {
>>> 		struct bio_integrity_payload *bi_integrity; /* data integrity */
>>> #endif
>>> 	};
>>> +	bio_verifier_t		*bi_verifier;	/* verify callback when endio */
>>> +	struct work_struct      bi_work;	/* I/O completion */
>>> 
>>> 	unsigned short		bi_vcnt;	/* how many bio_vec's */
>> 
>> I told you this for the initial posting, and the objection still stands.
>> Adding 40 bytes to struct bio is a no-go.
> 
> Yes, I know that.
> Already list in Todo:
> - union with "struct bio_integrity_payload *bi_integrity" to save bio space.
> 
> This is only a rfc patch, because not sure any other objection of this passdown-verify solution..
> Will consider more about the bio space if there is agreement of the passdown-verify way.

It is best if "rfc patch" is labelled with "RFC" in the subject when there are known
TODO issues with the patch, to avoid this kind of confusion... :-)


I worked on a similar patch that adds generate and verify callback functions to struct
bio_integrity_payload rather than directly in the bio, to avoid increasing bio size.

This was implemented by registering the callback functions via bio_integrity_prep_fn()
and stored them in bio_integrity_payload before the bio_submit() was called, rather
than passing them down the call stack as would be needed here.

This would allow having a per-bio callback without the changing the whole callchain.
Instead of XFS calling the new bio_submit_verify(), it calls bio_integrity_prep_fn(),
which doesn't seem materially different in terms of code complexity (i.e. there will
still need to be some changes to XFS but it doesn't affect other code).

I've attached the patch below.  It was based on an older kernel and I've just done a
quick port to the latest kernel (untested), so it is entirely possible that there is
some breakage but it shows the general approach.

Cheers, Andreas



[-- Attachment #1.2: 0001-block-add-optional-integrity-callbacks-for-bio.patch --]
[-- Type: application/octet-stream, Size: 9422 bytes --]

From 1d999a8597f93ff36d4fbe08ba3a68f81baa5f10 Mon Sep 17 00:00:00 2001
From: Andreas Dilger <adilger@dilger.ca>
Date: Fri, 29 Mar 2019 17:58:56 -0600
Subject: [RFC PATCH] block: add optional integrity callbacks for bio

Add optional integrity generate and verify callback functions for
a submitted bio.  The callbacks are passed to bio_integrity_prep()
and bio_integrity_alloc() and stored in struct bio_integrity_payload
rather than the bio to avoid increasing the size of struct bio when
they are not needed.

The per-bio integrity generate and verify functions take priority
over the ones registered on the block device.

Signed-off-by: Li Xi <lixi@ddn.com>
Signed-off-by: Li Dongyang <dongyangli@ddn.com>
Signed-off-by: Andreas Dilger <adilger@dilger.ca>
---
 block/bio-integrity.c  | 85 ++++++++++++++++++++++++++++++++++++--------------
 include/linux/bio.h    |  4 +++
 include/linux/blkdev.h |  2 ++
 3 files changed, 68 insertions(+), 23 deletions(-)

diff --git a/block/bio-integrity.c b/block/bio-integrity.c
index 1b633a3..5e670ad 100644
--- a/block/bio-integrity.c
+++ b/block/bio-integrity.c
@@ -39,18 +39,23 @@ void blk_flush_integrity(void)
 }
 
 /**
- * bio_integrity_alloc - Allocate integrity payload and attach it to bio
+ * bio_integrity_alloc_fn - Allocate integrity payload and attach it to bio
  * @bio:	bio to attach integrity metadata to
  * @gfp_mask:	Memory allocation mask
  * @nr_vecs:	Number of integrity metadata scatter-gather elements
+ * @generate_fn: Optional callback function to generate integrity data on write
+ * @verify_fn	Optional callback function to verify integrity data on read
  *
  * Description: This function prepares a bio for attaching integrity
- * metadata.  nr_vecs specifies the maximum number of pages containing
- * integrity metadata that can be attached.
+ * metadata.  @nr_vecs specifies the maximum number of pages containing
+ * integrity metadata that can be attached.  If the @generate_fn and/or
+ * @verify_fn are not NULL, then they will be stored in bio_integrity_payload
+ * for use when bio_integrity_prep() and bio_integrity_verify() are called.
  */
-struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
-						  gfp_t gfp_mask,
-						  unsigned int nr_vecs)
+struct bio_integrity_payload *bio_integrity_alloc_fn(struct bio *bio,
+					gfp_t gfp_mask, unsigned int nr_vecs,
+					integrity_processing_fn *generate_fn,
+					integrity_processing_fn *verify_fn)
 {
 	struct bio_integrity_payload *bip;
 	struct bio_set *bs = bio->bi_pool;
@@ -85,6 +90,8 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
 	}
 
 	bip->bip_bio = bio;
+	bip->bip_generate_fn = generate_fn;
+	bip->bip_verify_fn = verify_fn;
 	bio->bi_integrity = bip;
 	bio->bi_opf |= REQ_INTEGRITY;
 
@@ -93,6 +100,14 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
 	mempool_free(bip, &bs->bio_integrity_pool);
 	return ERR_PTR(-ENOMEM);
 }
+EXPORT_SYMBOL(bio_integrity_allocfn);
+
+struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
+						  gfp_t gfp_mask,
+						  unsigned int nr_vecs)
+{
+	return bio_integrity_alloc_fn(bio, gfp_mask, nr_vecs, NULL, NULL);
+}
 EXPORT_SYMBOL(bio_integrity_alloc);
 
 /**
@@ -187,6 +202,8 @@ static blk_status_t bio_integrity_process(struct bio *bio,
 
 		iter.data_buf = kaddr + bv.bv_offset;
 		iter.data_size = bv.bv_len;
+		iter.bio = bio;
+		iter.bi_idx = proc_iter->bi_idx;
 
 		ret = proc_fn(&iter);
 		if (ret) {
@@ -200,18 +217,23 @@ static blk_status_t bio_integrity_process(struct bio *bio,
 }
 
 /**
- * bio_integrity_prep - Prepare bio for integrity I/O
- * @bio:	bio to prepare
+ * bio_integrity_prep_fn - Prepare bio for integrity I/O
+ * @bio:		bio to prepare
+ * @generate_fn:	optional function called on write to generate integrity
+ * @verify_fn:		optional function called on read to verify integrity
  *
  * Description:  Checks if the bio already has an integrity payload attached.
  * If it does, the payload has been generated by another kernel subsystem,
  * and we just pass it through. Otherwise allocates integrity payload.
  * The bio must have data direction, target device and start sector set priot
  * to calling.  In the WRITE case, integrity metadata will be generated using
- * the block device's integrity function.  In the READ case, the buffer
- * will be prepared for DMA and a suitable end_io handler set up.
+ * either the passed-in @generate_fn (if set), the payload generate_fn (if
+ * set), or the block device's integrity function.  In the READ case, the
+ * buffer will be prepared for DMA and a suitable end_io handler set up.
  */
-bool bio_integrity_prep(struct bio *bio)
+bool bio_integrity_prep_fn(struct bio *bio,
+			   integrity_processing_fn *generate_fn,
+			   integrity_processing_fn *verify_fn)
 {
 	struct bio_integrity_payload *bip;
 	struct blk_integrity *bi = blk_get_integrity(bio->bi_disk);
@@ -236,13 +258,21 @@ bool bio_integrity_prep(struct bio *bio)
 	if (bio_integrity(bio))
 		return true;
 
-	if (bio_data_dir(bio) == READ) {
-		if (!bi->profile->verify_fn ||
-		    !(bi->flags & BLK_INTEGRITY_VERIFY))
+	if (bio_data_dir(bio) == WRITE) {
+		if (!generate_fn) {
+			generate_fn = bip->bip_generate_fn;
+			if (!generate_fn)
+				generate_fn = bi->profile->generate_fn;
+		}
+		if (!generate_fn || !(bi->flags & BLK_INTEGRITY_GENERATE))
 			return true;
 	} else {
-		if (!bi->profile->generate_fn ||
-		    !(bi->flags & BLK_INTEGRITY_GENERATE))
+		if (!verify_fn) {
+			verify_fn = bip->bip_verify_fn;
+			if (!verify_fn)
+				verify_fn = bi->profile->verify_fn;
+		}
+		if (!verify_fn || !(bi->flags & BLK_INTEGRITY_VERIFY))
 			return true;
 	}
 	intervals = bio_integrity_intervals(bi, bio_sectors(bio));
@@ -261,7 +291,8 @@ bool bio_integrity_prep(struct bio *bio)
 	nr_pages = end - start;
 
 	/* Allocate bio integrity payload and integrity vectors */
-	bip = bio_integrity_alloc(bio, GFP_NOIO, nr_pages);
+	bip = bio_integrity_alloc_fn(bio, GFP_NOIO, nr_pages, generate_fn,
+				     verify_fn);
 	if (IS_ERR(bip)) {
 		printk(KERN_ERR "could not allocate data integrity bioset\n");
 		kfree(buf);
@@ -303,12 +334,11 @@ bool bio_integrity_prep(struct bio *bio)
 	}
 
 	/* Auto-generate integrity metadata if this is a write */
-	if (bio_data_dir(bio) == WRITE) {
-		bio_integrity_process(bio, &bio->bi_iter,
-				      bi->profile->generate_fn);
-	} else {
+	if (bio_data_dir(bio) == WRITE)
+		bio_integrity_process(bio, &bio->bi_iter, generate_fn);
+	else
 		bip->bio_iter = bio->bi_iter;
-	}
+
 	return true;
 
 err_end_io:
@@ -317,6 +347,12 @@ bool bio_integrity_prep(struct bio *bio)
 	return false;
 
 }
+EXPORT_SYMBOL(bio_integrity_prep_fn);
+
+bool bio_integrity_prep(struct bio *bio)
+{
+	return bio_integrity_prep_fn(bio, NULL, NULL);
+}
 EXPORT_SYMBOL(bio_integrity_prep);
 
 /**
@@ -340,7 +376,8 @@ static void bio_integrity_verify_fn(struct work_struct *work)
 	 * it's original position.
 	 */
 	bio->bi_status = bio_integrity_process(bio, &bip->bio_iter,
-						bi->profile->verify_fn);
+					       bip->bip_verify_fn ?:
+					       bi->profile->verify_fn);
 	bio_integrity_free(bio);
 	bio_endio(bio);
 }
@@ -431,6 +468,8 @@ int bio_integrity_clone(struct bio *bio, struct bio *bio_src,
 
 	bip->bip_vcnt = bip_src->bip_vcnt;
 	bip->bip_iter = bip_src->bip_iter;
+	bip->bip_generate_fn = bip_src->bip_generate_fn;
+	bip->bip_verify_fn = bip_src->bip_verify_fn;
 
 	return 0;
 }
diff --git a/include/linux/bio.h b/include/linux/bio.h
index bb6090a..79e0c2f 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -315,6 +315,8 @@ struct bio_integrity_payload {
 	struct work_struct	bip_work;	/* I/O completion */
 
 	struct bio_vec		*bip_vec;
+	integrity_processing_fn	*bip_generate_fn;  /* called on write */
+	integrity_processing_fn	*bip_verify_fn;	   /* called on read */
 	struct bio_vec		bip_inline_vecs[0];/* embedded bvec array */
 };
 
@@ -754,8 +756,10 @@ static inline bool bioset_initialized(struct bio_set *bs)
 		bip_for_each_vec(_bvl, _bio->bi_integrity, _iter)
 
 extern struct bio_integrity_payload *bio_integrity_alloc(struct bio *, gfp_t, unsigned int);
+extern struct bio_integrity_payload *bio_integrity_alloc_fn(struct bio *, gfp_t, unsigned int, integrity_processing_fn *generate_fn, integrity_processing_fn *verify_fn);
 extern int bio_integrity_add_page(struct bio *, struct page *, unsigned int, unsigned int);
 extern bool bio_integrity_prep(struct bio *);
+extern bool bio_integrity_prep_fn(struct bio *, integrity_processing_fn *generate_fn, integrity_processing_fn *verify_fn);
 extern void bio_integrity_advance(struct bio *, unsigned int);
 extern void bio_integrity_trim(struct bio *);
 extern int bio_integrity_clone(struct bio *, struct bio *, gfp_t);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5c58a3b..be9a833 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1471,7 +1471,9 @@ struct blk_integrity_iter {
 	sector_t		seed;
 	unsigned int		data_size;
 	unsigned short		interval;
+	unsigned short		bi_idx;
 	const char		*disk_name;
+	struct bio		*bio;
 };
 
 typedef blk_status_t (integrity_processing_fn) (struct blk_integrity_iter *);
-- 
1.8.0


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]