[PATCH] DM data integrity support

[PATCH] DM data integrity support

[Martin K. Petersen]

Add support for data integrity to DM.

If all subdevices support the same protection format the DM device is
flagged as capable.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

---

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -874,7 +874,43 @@ void dm_table_set_restrictions(struct dm
 		queue_flag_clear_unlocked(QUEUE_FLAG_CLUSTER, q);
 	else
 		queue_flag_set_unlocked(QUEUE_FLAG_CLUSTER, q);
+}
 
+void dm_table_set_integrity(struct dm_table *t, struct mapped_device *md)
+{
+	struct list_head *devices = dm_table_get_devices(t);
+	struct dm_dev_internal *prev, *cur;
+
+	/*
+	 * Run through all devices to ensure they have matching
+	 * integrity profile
+	 */
+	cur = prev = NULL;
+
+	list_for_each_entry(cur, devices, list) {
+
+		if (prev &&
+		    blk_integrity_compare(prev->dm_dev.bdev->bd_disk,
+					  cur->dm_dev.bdev->bd_disk) < 0) {
+			printk(KERN_ERR "%s: %s %s integrity mismatch!\n",
+			       __func__, prev->dm_dev.bdev->bd_disk->disk_name,
+			       cur->dm_dev.bdev->bd_disk->disk_name);
+			return;
+		}
+		prev = cur;
+	}
+
+	if (!prev || !bdev_get_integrity(prev->dm_dev.bdev))
+		return;
+
+	/* Register dm device as being integrity capable */
+	if (blk_integrity_register(dm_disk(md),
+				   bdev_get_integrity(prev->dm_dev.bdev)))
+		printk(KERN_ERR "%s: %s Could not register integrity!\n",
+		       __func__, dm_disk(md)->disk_name);
+	else
+		printk(KERN_INFO "Enabling data integrity on %s\n",
+		       dm_disk(md)->disk_name);
 }
 
 unsigned int dm_table_get_num_targets(struct dm_table *t)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -669,6 +669,13 @@ static struct bio *split_bvec(struct bio
 	clone->bi_size = to_bytes(len);
 	clone->bi_io_vec->bv_offset = offset;
 	clone->bi_io_vec->bv_len = clone->bi_size;
+	clone->bi_flags |= 1 << BIO_CLONED;
+
+	if (bio_integrity(bio)) {
+		bio_integrity_clone(clone, bio, bs);
+		bio_integrity_trim(clone,
+				   bio_sector_offset(bio, idx, offset), len);
+	}
 
 	return clone;
 }
@@ -690,6 +697,14 @@ static struct bio *clone_bio(struct bio 
 	clone->bi_vcnt = idx + bv_count;
 	clone->bi_size = to_bytes(len);
 	clone->bi_flags &= ~(1 << BIO_SEG_VALID);
+
+	if (bio_integrity(bio)) {
+		bio_integrity_clone(clone, bio, bs);
+
+		if (idx != bio->bi_idx || clone->bi_size < bio->bi_size)
+			bio_integrity_trim(clone,
+					   bio_sector_offset(bio, idx, 0), len);
+	}
 
 	return clone;
 }
@@ -1161,6 +1176,7 @@ static void free_dev(struct mapped_devic
 	mempool_destroy(md->tio_pool);
 	mempool_destroy(md->io_pool);
 	bioset_free(md->bs);
+	blk_integrity_unregister(md->disk);
 	del_gendisk(md->disk);
 	free_minor(minor);
 
@@ -1226,6 +1242,7 @@ static int __bind(struct mapped_device *
 	write_lock(&md->map_lock);
 	md->map = t;
 	dm_table_set_restrictions(t, q);
+	dm_table_set_integrity(t, md);
 	write_unlock(&md->map_lock);
 
 	return 0;
diff --git a/drivers/md/dm.h b/drivers/md/dm.h
--- a/drivers/md/dm.h
+++ b/drivers/md/dm.h
@@ -41,6 +41,7 @@ struct dm_target *dm_table_get_target(st
 struct dm_target *dm_table_get_target(struct dm_table *t, unsigned int index);
 struct dm_target *dm_table_find_target(struct dm_table *t, sector_t sector);
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q);
+void dm_table_set_integrity(struct dm_table *t, struct mapped_device *md);
 struct list_head *dm_table_get_devices(struct dm_table *t);
 void dm_table_presuspend_targets(struct dm_table *t);
 void dm_table_postsuspend_targets(struct dm_table *t);

Re: [PATCH] DM data integrity support

[Alasdair G Kergon]

On Fri, Oct 17, 2008 at 10:47:00AM -0400, Martin K. Petersen wrote:
> Add support for data integrity to DM.
> If all subdevices support the same protection format the DM device is
> flagged as capable.
 
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> +void dm_table_set_integrity(struct dm_table *t, struct mapped_device *md)

md looks superfluous here - I've replaced it with t->md.

I've also moved the function inside dm_table_set_restrictions as they fit
together logically and both suffer from the same flaw when properties of
a subdevice get changed and manual intervention is needed for propagation back
up the tree.

> +	list_for_each_entry(cur, devices, list) {
> +		if (prev &&
> +		    blk_integrity_compare(prev->dm_dev.bdev->bd_disk,
> +					  cur->dm_dev.bdev->bd_disk) < 0) {
> +			printk(KERN_ERR "%s: %s %s integrity mismatch!\n",
> +			       __func__, prev->dm_dev.bdev->bd_disk->disk_name,
> +			       cur->dm_dev.bdev->bd_disk->disk_name);

That message adds nothing to the preceding one that blk_integrity_compare()
would have issued - the one bit of data it could add, namely the name of
the mapped device, isn't there, so I've added it.  I've also reduced it to
KERN_WARN, and I'm a bit concerned about those messages issued by
blk_integrity_compare() too i.e. any message like that will generate support
requests asking what it means and how to fix the problem and eliminate the
messages, but won't they sometimes be unavoidable and expected?

> +	/* Register dm device as being integrity capable */
> +	if (blk_integrity_register(dm_disk(md),
> +				   bdev_get_integrity(prev->dm_dev.bdev)))
> +		printk(KERN_ERR "%s: %s Could not register integrity!\n",
> +		       __func__, dm_disk(md)->disk_name);
> +	else
> +		printk(KERN_INFO "Enabling data integrity on %s\n",
> +		       dm_disk(md)->disk_name);

Superfluous?  Changing to DMDEBUG: if you don't get the error message and the
mapped_device is not its own endpoint for I/O, you can assume it did this.  

> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -669,6 +669,13 @@ static struct bio *split_bvec(struct bio
>  	clone->bi_size = to_bytes(len);
>  	clone->bi_io_vec->bv_offset = offset;
>  	clone->bi_io_vec->bv_len = clone->bi_size;
> +	clone->bi_flags |= 1 << BIO_CLONED;

I've separated that into its own patch - it's nothing to do with blk_integrity,
and if it has unforseen side-effects we need people to be able to bisect to it.

> @@ -1226,6 +1242,7 @@ static int __bind(struct mapped_device *
>  	write_lock(&md->map_lock);
>  	md->map = t;
>  	dm_table_set_restrictions(t, q);
> +	dm_table_set_integrity(t, md);

I moved that inside dm_table_set_restrictions, but there's a little problem
here.  The first time it decides to call blk_integrity_register(), there are
memory allocations and kobject manipulation.  That's not desirable: I don't
want the number of sites relying upon PF_MEMALLOC and the amount of code that
has to remain under audit for memory pressure deadlocks to increase if we
can avoid that.  Those functions can be performed safely during alloc_dev() or
dm_table_create(), but with the device-mapper code as it currently is,
__bind() needs to remain as quick and as simple as possible.

Perhaps change the register/unregister interface to make it symmetric:
we always register and unregister when creating the device, then set the
integrity to the appropriate value - which might include NULL - when the new
table is put into place.

Currently how does device-mapper clear the integrity profile of a mapped_device
which used to have one but which no longer does after a table swap?  In other
words I'm suggesting 'not supported' could be a valid integrity profile, rather
than requiring an 'unregister' (which seems to be missing from the appropriate
code path in this patch anyway).

I'd also like to check how:
   kobject_uevent(&bi->kobj, KOBJ_ADD);
is meant to be used, and whether it could move outside __bind() to the end of the
dm_resume (where we already issue a uevent).

Updated version below.

Alasdair


From: Martin K. Petersen <martin.petersen@oracle.com>

When a bio gets split, mark its fragments with the BIO_CLONED flag.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>
---
 drivers/md/dm.c |    1 +
 1 files changed, 1 insertion(+)

Index: linux/drivers/md/dm.c
===================================================================
--- linux.orig/drivers/md/dm.c	2008-10-21 12:12:55.000000000 +0100
+++ linux/drivers/md/dm.c	2008-10-21 12:16:29.000000000 +0100
@@ -669,6 +669,7 @@ static struct bio *split_bvec(struct bio
 	clone->bi_size = to_bytes(len);
 	clone->bi_io_vec->bv_offset = offset;
 	clone->bi_io_vec->bv_len = clone->bi_size;
+	clone->bi_flags |= 1 << BIO_CLONED;
 
 	return clone;
 }
From: Martin K. Petersen <martin.petersen@oracle.com>

Add support for data integrity to DM.

If all subdevices support the same protection format the DM device is
flagged as capable.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>

---
 drivers/md/dm-table.c |   38 ++++++++++++++++++++++++++++++++++++++
 drivers/md/dm.c       |   15 +++++++++++++++
 2 files changed, 53 insertions(+)

Index: linux/drivers/md/dm-table.c
===================================================================
--- linux.orig/drivers/md/dm-table.c	2008-10-21 12:12:55.000000000 +0100
+++ linux/drivers/md/dm-table.c	2008-10-21 12:19:24.000000000 +0100
@@ -855,6 +855,43 @@ struct dm_target *dm_table_find_target(s
 	return &t->targets[(KEYS_PER_NODE * n) + k];
 }
 
+/*
+ * Set the integrity profile for this device if all devices used have
+ * matching profiles.
+ */
+static void dm_table_set_integrity(struct dm_table *t)
+{
+	struct list_head *devices = dm_table_get_devices(t);
+	struct dm_dev_internal *prev = NULL, *dd = NULL;
+	struct blk_integrity *bi;
+
+	list_for_each_entry(dd, devices, list) {
+		if (prev &&
+		    blk_integrity_compare(prev->dm_dev.bdev->bd_disk,
+					  dd->dm_dev.bdev->bd_disk) < 0) {
+			DMWARN("%s: integrity not set: %s and %s mismatch",
+			       dm_device_name(t->md),
+			       prev->dm_dev.bdev->bd_disk->disk_name,
+			       dd->dm_dev.bdev->bd_disk->disk_name);
+			return;
+		}
+		prev = dd;
+	}
+
+	if (!prev)
+		return;
+
+	bi = bdev_get_integrity(prev->dm_dev.bdev);
+	if (!bi)
+		return;
+
+	if (blk_integrity_register(dm_disk(t->md), bi))
+		DMERR("%s: %s: blk_integrity_register failed.",
+		       __func__, dm_device_name(t->md));
+	else
+		DMDEBUG("%s: Enabling data integrity.", dm_device_name(t->md));
+}
+
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q)
 {
 	/*
@@ -875,6 +912,7 @@ void dm_table_set_restrictions(struct dm
 	else
 		queue_flag_set_unlocked(QUEUE_FLAG_CLUSTER, q);
 
+	dm_table_set_integrity(t);
 }
 
 unsigned int dm_table_get_num_targets(struct dm_table *t)
Index: linux/drivers/md/dm.c
===================================================================
--- linux.orig/drivers/md/dm.c	2008-10-21 12:16:29.000000000 +0100
+++ linux/drivers/md/dm.c	2008-10-21 12:19:24.000000000 +0100
@@ -671,6 +671,12 @@ static struct bio *split_bvec(struct bio
 	clone->bi_io_vec->bv_len = clone->bi_size;
 	clone->bi_flags |= 1 << BIO_CLONED;
 
+	if (bio_integrity(bio)) {
+		bio_integrity_clone(clone, bio, bs);
+		bio_integrity_trim(clone,
+				   bio_sector_offset(bio, idx, offset), len);
+	}
+
 	return clone;
 }
 
@@ -692,6 +698,14 @@ static struct bio *clone_bio(struct bio 
 	clone->bi_size = to_bytes(len);
 	clone->bi_flags &= ~(1 << BIO_SEG_VALID);
 
+	if (bio_integrity(bio)) {
+		bio_integrity_clone(clone, bio, bs);
+
+		if (idx != bio->bi_idx || clone->bi_size < bio->bi_size)
+			bio_integrity_trim(clone,
+					   bio_sector_offset(bio, idx, 0), len);
+	}
+
 	return clone;
 }
 
@@ -1162,6 +1176,7 @@ static void free_dev(struct mapped_devic
 	mempool_destroy(md->tio_pool);
 	mempool_destroy(md->io_pool);
 	bioset_free(md->bs);
+	blk_integrity_unregister(md->disk);
 	del_gendisk(md->disk);
 	free_minor(minor);
 

Re: [PATCH] DM data integrity support

[Martin K. Petersen]

>>>>> "agk" == Alasdair G Kergon <agk@redhat.com> writes:

agk> That message adds nothing to the preceding one that
agk> blk_integrity_compare() would have issued - the one bit of data
agk> it could add, namely the name of the mapped device, isn't there,
agk> so I've added it.  

m'kay


agk> I've also reduced it to KERN_WARN, and I'm a bit concerned about
agk> those messages issued by blk_integrity_compare() too i.e. any
agk> message like that will generate support requests asking what it
agk> means and how to fix the problem and eliminate the messages, but
agk> won't they sometimes be unavoidable and expected?

Well, they are there because they are important in the same sense as
"Your RAID is running in degraded mode".  If you've spent $$$$ on
integrity-capable hardware, one should think you'd want to use it
correctly.

Once this technology starts trickling down to commodity hardware we
may want to revisit.  And hopefully by then we'll have better userland
tools to manage this stuff.


[Cloning]

agk> I've separated that into its own patch - it's nothing to do with
agk> blk_integrity, and if it has unforseen side-effects we need
agk> people to be able to bisect to it.

That's fine.


agk> Perhaps change the register/unregister interface to make it
agk> symmetric: we always register and unregister when creating the
agk> device, then set the integrity to the appropriate value - which
agk> might include NULL - when the new table is put into place.

It seemed wasteful to register profiles since 99%+ of all storage
devices out there that don't have a need for them.  That's why I
deferred the allocation.  But I guess mere mortals compile without
BLK_DEV_INTEGRITY anyway so it may not matter that much...

Jens, what do you think?


agk> Currently how does device-mapper clear the integrity profile of a
agk> mapped_device which used to have one but which no longer does
agk> after a table swap?  In other words I'm suggesting 'not
agk> supported' could be a valid integrity profile, rather than
agk> requiring an 'unregister' (which seems to be missing from the
agk> appropriate code path in this patch anyway).

I botched the unregister stuff when I redid the patch after the
previous round of feedback.  Will fix...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] DM data integrity support

[Jens Axboe]

On Fri, Oct 24 2008, Martin K. Petersen wrote:
> agk> Perhaps change the register/unregister interface to make it
> agk> symmetric: we always register and unregister when creating the
> agk> device, then set the integrity to the appropriate value - which
> agk> might include NULL - when the new table is put into place.
> 
> It seemed wasteful to register profiles since 99%+ of all storage
> devices out there that don't have a need for them.  That's why I
> deferred the allocation.  But I guess mere mortals compile without
> BLK_DEV_INTEGRITY anyway so it may not matter that much...
> 
> Jens, what do you think?

I agree, plus there are going to be lots of users running a
BLK_DEV_INTEGRITY enabled kernel but not having the hardware (distros).
So lazy alloc makes sense.

-- 
Jens Axboe

Re: [PATCH] DM data integrity support

[Alasdair G Kergon]

On Fri, Oct 24, 2008 at 12:36:50AM -0400, Martin K. Petersen wrote:
> agk> Perhaps change the register/unregister interface to make it
> agk> symmetric: we always register and unregister when creating the
> agk> device, then set the integrity to the appropriate value - which
> agk> might include NULL - when the new table is put into place.
> It seemed wasteful to register profiles since 99%+ of all storage
> devices out there that don't have a need for them.  That's why I
> deferred the allocation.  But I guess mere mortals compile without
> BLK_DEV_INTEGRITY anyway so it may not matter that much...
 
Then the integrity profile allocation/free should be attached to the
table creation/destruction, not the device itself, and you make the
reasonable assumption that the profiles of the devices in the table
won't change between table creation and device resumption.

i.e. Each dm table holds a reference to its own allocated integrity
profile, and when a table switches from inactivate to live, that profile
becomes the one registered against the block device (without memory
allocation that could deadlock at that point - the allocation might
involve flushing to the suspended device, which waits for the resume
which waits for this allocation).  (I'd prefer not to see another
dependency on PF_MEMALLOC enter the tree, when a slightly different
interface could avoid it and still offer lazy allocation.)

Alasdair
-- 
agk@redhat.com

[PATCH] dm: Data integrity support

[Martin K. Petersen]

This patch provides support for data integrity passthrough in the device
mapper.

 - If one or more component devices support integrity an integrity
   profile is preallocated for the DM device.

 - If all component devices have compatible profiles the DM device is
   flagged as capable.

 - Handle integrity metadata when splitting and cloning bios.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

---

This patch requires a change in Jens' for-linus branch that adds a GFP
mask to bio_integrity_clone() and obsoletes the bio_set argument.


diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1046,6 +1046,19 @@ static int populate_table(struct dm_tabl
 	return dm_table_complete(table);
 }
 
+static int table_prealloc_integrity(struct dm_table *t,
+				    struct mapped_device *md)
+{
+	struct list_head *devices = dm_table_get_devices(t);
+	struct dm_dev_internal *dd;
+
+	list_for_each_entry(dd, devices, list)
+		if (bdev_get_integrity(dd->dm_dev.bdev))
+			return blk_integrity_register(dm_disk(md), NULL);
+
+	return 0;
+}
+
 static int table_load(struct dm_ioctl *param, size_t param_size)
 {
 	int r;
@@ -1067,6 +1080,14 @@ static int table_load(struct dm_ioctl *p
 		goto out;
 	}
 
+	r = table_prealloc_integrity(t, md);
+	if (r) {
+		DMERR("%s: could not register integrity profile.",
+		      dm_device_name(md));
+		dm_table_destroy(t);
+		goto out;
+	}
+
 	down_write(&_hash_lock);
 	hc = dm_get_mdptr(md);
 	if (!hc || hc->md != md) {
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -877,6 +877,45 @@ struct dm_target *dm_table_find_target(s
 	return &t->targets[(KEYS_PER_NODE * n) + k];
 }
 
+/*
+ * Set the integrity profile for this device if all devices used have
+ * matching profiles.
+ */
+static void dm_table_set_integrity(struct dm_table *t)
+{
+	struct list_head *devices = dm_table_get_devices(t);
+	struct dm_dev_internal *prev = NULL, *dd = NULL;
+
+	if (!blk_get_integrity(dm_disk(t->md)))
+		return;
+
+	list_for_each_entry(dd, devices, list) {
+		if (prev &&
+		    blk_integrity_compare(prev->dm_dev.bdev->bd_disk,
+					  dd->dm_dev.bdev->bd_disk) < 0) {
+			DMWARN("%s: integrity not set: %s and %s mismatch",
+			       dm_device_name(t->md),
+			       prev->dm_dev.bdev->bd_disk->disk_name,
+			       dd->dm_dev.bdev->bd_disk->disk_name);
+			goto no_integrity;
+		}
+		prev = dd;
+	}
+
+	if (!prev || !bdev_get_integrity(prev->dm_dev.bdev))
+		goto no_integrity;
+
+	blk_integrity_register(dm_disk(t->md),
+			       bdev_get_integrity(prev->dm_dev.bdev));
+
+	return;
+
+no_integrity:
+	blk_integrity_register(dm_disk(t->md), NULL);
+
+	return;
+}
+
 void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q)
 {
 	/*
@@ -897,6 +936,7 @@ void dm_table_set_restrictions(struct dm
 	else
 		queue_flag_set_unlocked(QUEUE_FLAG_CLUSTER, q);
 
+	dm_table_set_integrity(t);
 }
 
 unsigned int dm_table_get_num_targets(struct dm_table *t)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -703,6 +703,12 @@ static struct bio *split_bvec(struct bio
 	clone->bi_io_vec->bv_len = clone->bi_size;
 	clone->bi_flags |= 1 << BIO_CLONED;
 
+	if (bio_integrity(bio)) {
+		bio_integrity_clone(clone, bio, GFP_NOIO);
+		bio_integrity_trim(clone,
+				   bio_sector_offset(bio, idx, offset), len);
+	}
+
 	return clone;
 }
 
@@ -724,6 +730,14 @@ static struct bio *clone_bio(struct bio 
 	clone->bi_size = to_bytes(len);
 	clone->bi_flags &= ~(1 << BIO_SEG_VALID);
 
+	if (bio_integrity(bio)) {
+		bio_integrity_clone(clone, bio, GFP_NOIO);
+
+		if (idx != bio->bi_idx || clone->bi_size < bio->bi_size)
+			bio_integrity_trim(clone,
+					   bio_sector_offset(bio, idx, 0), len);
+	}
+
 	return clone;
 }
 
@@ -1191,6 +1205,7 @@ static void free_dev(struct mapped_devic
 	mempool_destroy(md->tio_pool);
 	mempool_destroy(md->io_pool);
 	bioset_free(md->bs);
+	blk_integrity_unregister(md->disk);
 	del_gendisk(md->disk);
 	free_minor(minor);
 

Re: [PATCH] dm: Data integrity support

[Alasdair G Kergon]

On Mon, Mar 09, 2009 at 11:46:05PM -0400, Martin K. Petersen wrote:
> This patch requires a change in Jens' for-linus branch that adds a GFP
> mask to bio_integrity_clone() and obsoletes the bio_set argument.
 
I've not had the chance to track down that patch, but if you can note
in this thread when it's hit Linus's tree, then I'll submit this one
behind it.

Alasdair
-- 
agk@redhat.com