Re: [PATCH 5/5] block: Inline blk_integrity in struct gendisk

Re: [PATCH 5/5] block: Inline blk_integrity in struct gendisk

[Mike Snitzer]

On Thu, Aug 20 2015 at  4:41pm -0400,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> Up until now the_integrity profile has been dynamically allocated and
> attached to struct gendisk after the disk has been made active.
> 
> This causes problems because NVMe devices need to register the profile
> prior to the partition table being read due to a mandatory metadata
> buffer requirement. In addition, DM goes through hoops to deal with
> preallocating, but not initializing integrity profiles.

Yes, but only if the underlying device(s) actually support bip.  This
change to inlining blk_integrity (purely to make NVMe happy) comes at
the cost of _always_ allocating the memory 'struct blk_integrity' (via
gendisk inline) even if there is absolutely no need for it.

That said, with your changes the blk_integrity structure is no longer
large (previously was 96 bytes).. so I can let that part go.

> Since the integrity profile is small (4 bytes + a pointer), Christoph
> suggested moving it to struct gendisk proper. This requires several
> changes:
> 
>  - Moving the blk_integrity definition to genhd.h.
> 
>  - Inlining blk_integrity in struct gendisk.
> 
>  - Removing the dynamic allocation code.
> 
>  - Adding helper functions which allow gendisk to set up and tear down
>    the integrity sysfs dir when a disk is added/deleted.
> 
>  - Adding a blk_integrity_revalidate() callback for updating the stable
>    pages bdi setting.
> 
>  - The calls that depend on whether a device has an integrity profile or
>    not now key off of the bi->profile pointer.
> 
>  - Simplifying the integrity support routines in DM.

But I cannot let this DM "simplifying" go (vast majority of it breaks
bip for DM).  You missed the fact that DM has inactive and active
tables.  And that the top-level DM device can only be changed once an
inactive table is made active via DM's resume.  The bip for the DM
device cannot be changed during table load.  Such a change can only be
done during table resume.  Reason is an inactive DM table can get thrown
away at any time; so changing the top-level DM device during the load of
an inactive table isn't right.

Please review the commit header from commit a63a5cf84 ("dm: improve
block integrity support").

Long story short, DM changes that eliminate the checks/code for
allocating the blk_integrity structure should be all that is needed for
this patch.

I'll work through this further.  Hope to send an incremental patch that
fixes things up in the next day or so.

Re: [PATCH 5/5] block: Inline blk_integrity in struct gendisk

[Mike Snitzer]

On Tue, Sep 15 2015 at  9:07P -0400,
Mike Snitzer <snitzer@redhat.com> wrote:
 
> Long story short, DM changes that eliminate the checks/code for
> allocating the blk_integrity structure should be all that is needed for
> this patch.
> 
> I'll work through this further.  Hope to send an incremental patch that
> fixes things up in the next day or so.

Here is what I came up with.  Leaves something to be desired given the
integrity profile is being established on first table load (as opposed
to during resume like the old DM and block integrity code did).. but we
can iterate on this as needed.

Feel free to fold it into your last patch and add my Signed-off-by.

From: Mike Snitzer <snitzer@redhat.com>
Date: Mon, 21 Sep 2015 14:58:44 -0400
Subject: [PATCH] dm table: fixup block integrity profile processing

Not very different from what Martin originally proposed but subtle
changes include:

. only register the integrity profile on first table load; subsequent
  loads must have a matching integrity profile
  - if profile is already registered, verify new table's profile matches

. resume only verifies that the DM device's integrity profile matches
  all the underlying devices' profiles.
  - if they don't match the DM device's integrity profile is unregistered

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-table.c | 77 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 63 insertions(+), 14 deletions(-)

diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index e2f98fc..061152a 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1014,6 +1014,11 @@ static int dm_table_build_index(struct dm_table *t)
 	return r;
 }
 
+static bool integrity_profile_exists(struct gendisk *disk)
+{
+	return !!blk_get_integrity(disk);
+}
+
 /*
  * Get a disk whose integrity profile reflects the table's profile.
  * Returns NULL if integrity support was inconsistent or unavailable.
@@ -1026,10 +1031,10 @@ static struct gendisk * dm_table_get_integrity_disk(struct dm_table *t)
 
 	list_for_each_entry(dd, devices, list) {
 		template_disk = dd->dm_dev->bdev->bd_disk;
-		if (!blk_get_integrity(template_disk))
+		if (!integrity_profile_exists(template_disk))
 			goto no_integrity;
-		if (prev_disk &&
-		    blk_integrity_compare(prev_disk, template_disk) < 0)
+		else if (prev_disk &&
+			 blk_integrity_compare(prev_disk, template_disk) < 0)
 			goto no_integrity;
 		prev_disk = template_disk;
 	}
@@ -1055,23 +1060,40 @@ no_integrity:
  * profile validation: First pass during table load, final pass during
  * resume.
  */
-static int dm_table_set_integrity(struct dm_table *t)
+static int dm_table_register_integrity(struct dm_table *t)
 {
+	struct mapped_device *md = t->md;
 	struct gendisk *template_disk = NULL;
-	bool existing_profile = blk_get_integrity(dm_disk(t->md));
 
 	template_disk = dm_table_get_integrity_disk(t);
-	if (template_disk) {
-		blk_integrity_register(dm_disk(t->md),
-				       blk_get_integrity(template_disk));
+	if (!template_disk)
+		return 0;
+
+	if (!integrity_profile_exists(dm_disk(md))) {
 		t->integrity_supported = 1;
-	} else if (existing_profile) {
-		blk_integrity_unregister(dm_disk(t->md));
-		DMWARN("%s: device no longer has a valid integrity profile",
-		       dm_device_name(t->md));
+		/*
+		 * Register integrity profile during table load; we can do
+		 * this because the final profile must match during resume.
+		 */
+		blk_integrity_register(dm_disk(md),
+				       blk_get_integrity(template_disk));
+		return 0;
+	}
+
+	/*
+	 * If DM device already has an initialized integrity
+	 * profile the new profile should not conflict.
+	 */
+	if (blk_integrity_compare(dm_disk(md), template_disk) < 0) {
+		DMWARN("%s: conflict with existing integrity profile: "
+		       "%s profile mismatch",
+		       dm_device_name(t->md),
+		       template_disk->disk_name);
 		return 1;
 	}
 
+	/* Preserve existing integrity profile */
+	t->integrity_supported = 1;
 	return 0;
 }
 
@@ -1095,7 +1117,7 @@ int dm_table_complete(struct dm_table *t)
 		return r;
 	}
 
-	r = dm_table_set_integrity(t);
+	r = dm_table_register_integrity(t);
 	if (r) {
 		DMERR("could not register integrity profile.");
 		return r;
@@ -1260,6 +1282,33 @@ combine_limits:
 	return validate_hardware_logical_block_alignment(table, limits);
 }
 
+/*
+ * Verify that all devices have an integrity profile that matches the
+ * DM device's registered integrity profile.  If the profiles don't
+ * match then unregister the DM device's integrity profile.
+ */
+static void dm_table_verify_integrity(struct dm_table *t)
+{
+	struct gendisk *template_disk = NULL;
+
+	if (t->integrity_supported) {
+		/*
+		 * Verify that the original integrity profile
+		 * matches all the devices in this table.
+		 */
+		template_disk = dm_table_get_integrity_disk(t);
+		if (template_disk &&
+		    blk_integrity_compare(dm_disk(t->md), template_disk) >= 0)
+			return;
+	}
+
+	if (integrity_profile_exists(dm_disk(t->md))) {
+		DMWARN("%s: unable to establish an integrity profile",
+		       dm_device_name(t->md));
+		blk_integrity_unregister(dm_disk(t->md));
+	}
+}
+
 static int device_flush_capable(struct dm_target *ti, struct dm_dev *dev,
 				sector_t start, sector_t len, void *data)
 {
@@ -1457,7 +1506,7 @@ void dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 	else
 		queue_flag_set_unlocked(QUEUE_FLAG_NO_SG_MERGE, q);
 
-	dm_table_set_integrity(t);
+	dm_table_verify_integrity(t);
 
 	/*
 	 * Determine whether or not this queue's I/O timings contribute
-- 
2.3.8 (Apple Git-58)

Re: [PATCH 5/5] block: Inline blk_integrity in struct gendisk

[Christoph Hellwig]

On Mon, Sep 21, 2015 at 04:45:07PM -0400, Mike Snitzer wrote:
> Here is what I came up with.  Leaves something to be desired given the
> integrity profile is being established on first table load (as opposed
> to during resume like the old DM and block integrity code did).. but we
> can iterate on this as needed.
> 
> Feel free to fold it into your last patch and add my Signed-off-by.

Martin, any chance to repost with Mike's changes folded in?  I'd really
hate to miss 4.4 for the series.

Re: [PATCH 5/5] block: Inline blk_integrity in struct gendisk

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph> any chance to repost with Mike's changes folded in?  I'd
Christoph> really hate to miss 4.4 for the series.

Will do first thing in the morning.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/5] block: Move integrity kobject to struct gendisk

[Mike Snitzer]

On Thu, Aug 20 2015 at  4:41pm -0400,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> The integrity kobject purely exists to support the integrity
> subdirectory in sysfs and doesn't really have anything to do with the
> blk_integrity data structure. Move the kobject to struct gendisk where
> it belongs.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Reported-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

[I understand both Martin and Keith are on vacation but I'm putting the
time to this now with the understanding that a proper exchange may be
delayed.]

Thinking about this series further: I don't really agree with it.
Maybe in the end the benefit of embedding in gendisk outweighs the
complexity of dynamic allocation... but I need to see it for myself.

Christoph, if you _know_ this to be the right way forward I can accept
it but please elaborate further.

What we currently have (before this patchset) has the very real benefit
of not wasting any memory if the block device doesn't have integrity
(DIF/DIX) support.  Especially given how rare bip supporting devices
still are.  Is there reason to expect they won't be so rare in the
future?  Every NVMe and nvdimm device will have integrity support?

This patch moves thes 64 byte 'struct kobject' out into 'struct
gendisk'.  I'm not seeing why (on x86_64 anyway) we'd what to always
allocate an extra 76 bytes (blk_integrity + kobject) for _every_ gendisk
regardless of whether a bip is needed..

76 bytes may not sound like a lot but in the context of DM-mpath that
adds up when you have systems with 1000s of devices with multiple paths
and then a DM mpath device (with its own gendisk) ontop -- 1000 devices
with 4 paths can waste 5000 * 76bytes = ~372K (even 372K isn't much...)

If DM-core could support existing dynamic bip (albeit with more involved
code born out of DM-specific quirks of device stacking and inactive and
active tables) then I have to believe NVMe can too.

As such I'm now going to shift my focus to revisiting what is so hard
for NVMe that it cannot cope with the existing dynamic allocation of
struct blk_integrity.

[If that turns out to be a waste of time (and NVMe/nvdimm/whatever would
be needlessly inconvenienced) then I'll switch back to fixing DM to cope
with this change.  But to be clear the current proposed DM changes
_cannot_ go upstream.]