[PATCH 0/3] dm thin metadata: handful of improvements

[PATCH 0/3] dm thin metadata: handful of improvements

[Mike Snitzer]

All are working well in my testing but I'd appreciate further review
from others.

Thanks,
Mike

Mike Snitzer (3):
  dm space map common: zero entire ll_disk
  dm thin metadata: do not write metadata if no changes occurred
  dm thin metadata: check __commit_transaction()'s return

 drivers/md/dm-thin-metadata.c                 | 63 +++++++++++++------
 .../md/persistent-data/dm-space-map-common.c  |  2 +
 2 files changed, 47 insertions(+), 18 deletions(-)

-- 
2.18.0

[PATCH 1/3] dm space map common: zero entire ll_disk

[Mike Snitzer]

Otherwise, memory that is allocated (and potentially not previously
zeroed) will get written to disk as part of the space maps.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/persistent-data/dm-space-map-common.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/md/persistent-data/dm-space-map-common.c b/drivers/md/persistent-data/dm-space-map-common.c
index 0a3b8ae4a29c..b8a62188f6be 100644
--- a/drivers/md/persistent-data/dm-space-map-common.c
+++ b/drivers/md/persistent-data/dm-space-map-common.c
@@ -190,6 +190,8 @@ static int sm_find_free(void *addr, unsigned begin, unsigned end,
 
 static int sm_ll_init(struct ll_disk *ll, struct dm_transaction_manager *tm)
 {
+	memset(ll, 0, sizeof(struct ll_disk));
+
 	ll->tm = tm;
 
 	ll->bitmap_info.tm = tm;
-- 
2.18.0

[PATCH 2/3] dm thin metadata: do not write metadata if no changes occurred

[Mike Snitzer]

Otherwise, just loading a thin-pool and then removing it will cause its
metadata to be changed (e.g. superblock written) -- even without any
thin devices being made active or metadata snapshot being manipulated.

Add 'changed' flag to struct dm_pool_metadata.  Set both td->changed and
pmd->changed using __set_thin_changed() -- this assures pmd->changed is
always set when td->changed is.  For methods that don't involve changing
a td, set pmd->changed directly.

Rename __delete_device() to __delete_thin() -- improves symmetry with
__create_thin().

Update dm_pool_changed_this_transaction() to simply return pmd->changed.

Fix dm_pool_commit_metadata() to open the next transaction if the return
from __commit_transaction() is 0.  Not seeing why the early return ever
made since for a return of 0 given that dm-io's async_io(), as used by
bufio, always returns 0.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-thin-metadata.c | 56 ++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c
index ed3caceaed07..f4e8327abc75 100644
--- a/drivers/md/dm-thin-metadata.c
+++ b/drivers/md/dm-thin-metadata.c
@@ -201,6 +201,12 @@ struct dm_pool_metadata {
 	 */
 	bool fail_io:1;
 
+	/*
+	 * Set if metadata has changed, particularly important for methods that
+	 * alter metadata without the ability to reflect the change in td->changed.
+	 */
+	bool changed:1;
+
 	/*
 	 * Reading the space map roots can fail, so we read it into these
 	 * buffers before the superblock is locked and updated.
@@ -769,7 +775,7 @@ static int __write_changed_details(struct dm_pool_metadata *pmd)
 			return r;
 
 		if (td->open_count)
-			td->changed = 0;
+			td->changed = false;
 		else {
 			list_del(&td->list);
 			kfree(td);
@@ -779,6 +785,14 @@ static int __write_changed_details(struct dm_pool_metadata *pmd)
 	return 0;
 }
 
+static void __set_thin_changed(struct dm_thin_device *td)
+{
+	struct dm_pool_metadata *pmd = td->pmd;
+
+	td->changed = true;
+	pmd->changed = true;
+}
+
 static int __commit_transaction(struct dm_pool_metadata *pmd)
 {
 	int r;
@@ -790,6 +804,9 @@ static int __commit_transaction(struct dm_pool_metadata *pmd)
 	 */
 	BUILD_BUG_ON(sizeof(struct thin_disk_superblock) > 512);
 
+	if (!pmd->changed)
+		return 0;
+
 	r = __write_changed_details(pmd);
 	if (r < 0)
 		return r;
@@ -819,6 +836,8 @@ static int __commit_transaction(struct dm_pool_metadata *pmd)
 
 	copy_sm_roots(pmd, disk_super);
 
+	pmd->changed = false;
+
 	return dm_tm_commit(pmd->tm, sblock);
 }
 
@@ -853,6 +872,7 @@ struct dm_pool_metadata *dm_pool_metadata_open(struct block_device *bdev,
 	pmd->time = 0;
 	INIT_LIST_HEAD(&pmd->thin_devices);
 	pmd->fail_io = false;
+	pmd->changed = false;
 	pmd->bdev = bdev;
 	pmd->data_block_size = data_block_size;
 
@@ -967,7 +987,8 @@ static int __open_device(struct dm_pool_metadata *pmd,
 	(*td)->pmd = pmd;
 	(*td)->id = dev;
 	(*td)->open_count = 1;
-	(*td)->changed = changed;
+	if (changed)
+		__set_thin_changed(*td);
 	(*td)->aborted_with_changes = false;
 	(*td)->mapped_blocks = le64_to_cpu(details_le.mapped_blocks);
 	(*td)->transaction_id = le64_to_cpu(details_le.transaction_id);
@@ -1051,7 +1072,7 @@ static int __set_snapshot_details(struct dm_pool_metadata *pmd,
 	if (r)
 		return r;
 
-	td->changed = 1;
+	__set_thin_changed(td);
 	td->snapshotted_time = time;
 
 	snap->mapped_blocks = td->mapped_blocks;
@@ -1131,7 +1152,7 @@ int dm_pool_create_snap(struct dm_pool_metadata *pmd,
 	return r;
 }
 
-static int __delete_device(struct dm_pool_metadata *pmd, dm_thin_id dev)
+static int __delete_thin(struct dm_pool_metadata *pmd, dm_thin_id dev)
 {
 	int r;
 	uint64_t key = dev;
@@ -1158,6 +1179,8 @@ static int __delete_device(struct dm_pool_metadata *pmd, dm_thin_id dev)
 	if (r)
 		return r;
 
+	pmd->changed = true;
+
 	return 0;
 }
 
@@ -1168,7 +1191,7 @@ int dm_pool_delete_thin_device(struct dm_pool_metadata *pmd,
 
 	down_write(&pmd->root_lock);
 	if (!pmd->fail_io)
-		r = __delete_device(pmd, dev);
+		r = __delete_thin(pmd, dev);
 	up_write(&pmd->root_lock);
 
 	return r;
@@ -1276,6 +1299,9 @@ static int __reserve_metadata_snap(struct dm_pool_metadata *pmd)
 	disk_super = dm_block_data(sblock);
 	disk_super->held_root = cpu_to_le64(held_root);
 	dm_bm_unlock(sblock);
+
+	pmd->changed = true;
+
 	return 0;
 }
 
@@ -1324,6 +1350,8 @@ static int __release_metadata_snap(struct dm_pool_metadata *pmd)
 
 	dm_tm_unlock(pmd->tm, copy);
 
+	pmd->changed = true;
+
 	return 0;
 }
 
@@ -1558,7 +1586,7 @@ static int __insert(struct dm_thin_device *td, dm_block_t block,
 	if (r)
 		return r;
 
-	td->changed = 1;
+	__set_thin_changed(td);
 	if (inserted)
 		td->mapped_blocks++;
 
@@ -1589,7 +1617,7 @@ static int __remove(struct dm_thin_device *td, dm_block_t block)
 		return r;
 
 	td->mapped_blocks--;
-	td->changed = 1;
+	__set_thin_changed(td);
 
 	return 0;
 }
@@ -1643,7 +1671,7 @@ static int __remove_range(struct dm_thin_device *td, dm_block_t begin, dm_block_
 	}
 
 	td->mapped_blocks -= total_count;
-	td->changed = 1;
+	__set_thin_changed(td);
 
 	/*
 	 * Reinsert the mapping tree.
@@ -1735,16 +1763,10 @@ bool dm_thin_changed_this_transaction(struct dm_thin_device *td)
 
 bool dm_pool_changed_this_transaction(struct dm_pool_metadata *pmd)
 {
-	bool r = false;
-	struct dm_thin_device *td, *tmp;
+	bool r;
 
 	down_read(&pmd->root_lock);
-	list_for_each_entry_safe(td, tmp, &pmd->thin_devices, list) {
-		if (td->changed) {
-			r = td->changed;
-			break;
-		}
-	}
+	r = pmd->changed;
 	up_read(&pmd->root_lock);
 
 	return r;
@@ -1782,7 +1804,7 @@ int dm_pool_commit_metadata(struct dm_pool_metadata *pmd)
 		goto out;
 
 	r = __commit_transaction(pmd);
-	if (r <= 0)
+	if (r < 0)
 		goto out;
 
 	/*
-- 
2.18.0

Re: [PATCH 2/3] dm thin metadata: do not write metadata if no changes occurred

[Mike Snitzer]

On Mon, Apr 15 2019 at  5:17pm -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> Otherwise, just loading a thin-pool and then removing it will cause its
> metadata to be changed (e.g. superblock written) -- even without any
> thin devices being made active or metadata snapshot being manipulated.
> 
> Add 'changed' flag to struct dm_pool_metadata.  Set both td->changed and
> pmd->changed using __set_thin_changed() -- this assures pmd->changed is
> always set when td->changed is.  For methods that don't involve changing
> a td, set pmd->changed directly.
> 
> Rename __delete_device() to __delete_thin() -- improves symmetry with
> __create_thin().
> 
> Update dm_pool_changed_this_transaction() to simply return pmd->changed.
> 
> Fix dm_pool_commit_metadata() to open the next transaction if the return
> from __commit_transaction() is 0.  Not seeing why the early return ever
> made since for a return of 0 given that dm-io's async_io(), as used by
> bufio, always returns 0.

This v1 patch was incomplete.  However this approach's need to establish
pmd->changed everywhere that alters metadata, without doing so in terms
of a thin device, is like whack-a-mole.

But I think this could be simplified to only set a 'pmd->accessed'
(never clear it) and only__commit_transaction() if pmd->accessed --
so only pool messages that change metadata would need to set
pmd->accessed.

[PATCH v2 2/3] dm thin metadata: do not write metadata if no changes occurred

[Mike Snitzer]

Otherwise, just loading a thin-pool and then removing it will cause its
metadata to be changed (e.g. superblock written) -- even without any
thin devices being made active or metadata snapshot being manipulated.

Add 'in_service' flag to struct dm_pool_metadata and use a new
dm_pool_set_in_service() interface to set it if either a thin device is
activated or a message is sent to the thin-pool.  Once 'in_service' is
set it is never cleared.  __commit_transaction() will return 0 if
'in_service' is not set.

Also fix dm_pool_commit_metadata() to open the next transaction if the
return from __commit_transaction() is 0.  Not seeing why the early
return ever made since for a return of 0 given that dm-io's async_io(),
as used by bufio, always returns 0.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-thin-metadata.c | 23 ++++++++++++++++++++++-
 drivers/md/dm-thin-metadata.h |  2 ++
 drivers/md/dm-thin.c          |  9 +++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c
index 7381e477a945..fcff30fc07cd 100644
--- a/drivers/md/dm-thin-metadata.c
+++ b/drivers/md/dm-thin-metadata.c
@@ -201,6 +201,13 @@ struct dm_pool_metadata {
 	 */
 	bool fail_io:1;
 
+	/*
+	 * Set once a thin-pool has been accessed through one of the interfaces
+	 * that imply the pool is in-service (e.g. thin devices created/deleted,
+	 * thin-pool message, metadata snapshots, etc).
+	 */
+	bool in_service:1;
+
 	/*
 	 * Reading the space map roots can fail, so we read it into these
 	 * buffers before the superblock is locked and updated.
@@ -790,6 +797,9 @@ static int __commit_transaction(struct dm_pool_metadata *pmd)
 	 */
 	BUILD_BUG_ON(sizeof(struct thin_disk_superblock) > 512);
 
+	if (unlikely(!pmd->in_service))
+		return 0;
+
 	r = __write_changed_details(pmd);
 	if (r < 0)
 		return r;
@@ -853,6 +863,7 @@ struct dm_pool_metadata *dm_pool_metadata_open(struct block_device *bdev,
 	pmd->time = 0;
 	INIT_LIST_HEAD(&pmd->thin_devices);
 	pmd->fail_io = false;
+	pmd->in_service = false;
 	pmd->bdev = bdev;
 	pmd->data_block_size = data_block_size;
 
@@ -1727,6 +1738,16 @@ int dm_pool_dec_data_range(struct dm_pool_metadata *pmd, dm_block_t b, dm_block_
 	return r;
 }
 
+void dm_pool_set_in_service(struct dm_pool_metadata *pmd)
+{
+	if (!pmd->in_service) {
+		down_write(&pmd->root_lock);
+		if (!pmd->in_service)
+			pmd->in_service = true;
+		up_write(&pmd->root_lock);
+	}
+}
+
 bool dm_thin_changed_this_transaction(struct dm_thin_device *td)
 {
 	int r;
@@ -1787,7 +1808,7 @@ int dm_pool_commit_metadata(struct dm_pool_metadata *pmd)
 		goto out;
 
 	r = __commit_transaction(pmd);
-	if (r <= 0)
+	if (r < 0)
 		goto out;
 
 	/*
diff --git a/drivers/md/dm-thin-metadata.h b/drivers/md/dm-thin-metadata.h
index f6be0d733c20..8192530984be 100644
--- a/drivers/md/dm-thin-metadata.h
+++ b/drivers/md/dm-thin-metadata.h
@@ -47,6 +47,8 @@ struct dm_pool_metadata *dm_pool_metadata_open(struct block_device *bdev,
 
 int dm_pool_metadata_close(struct dm_pool_metadata *pmd);
 
+void dm_pool_set_in_service(struct dm_pool_metadata *pmd);
+
 /*
  * Compat feature flags.  Any incompat flags beyond the ones
  * specified below will prevent use of the thin metadata.
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index fcd887703f95..b6dbbdfc9118 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -3822,6 +3822,13 @@ static int pool_message(struct dm_target *ti, unsigned argc, char **argv,
 		return -EOPNOTSUPP;
 	}
 
+	/*
+	 * Any message sent to a thin-pool warrants putting it in-service
+	 * (even if the message is malformed, it speaks to a user
+	 *  attempting to interface with the thin-pool)
+	 */
+	dm_pool_set_in_service(pool->pmd);
+
 	if (!strcasecmp(argv[0], "create_thin"))
 		r = process_create_thin_mesg(argc, argv, pool);
 
@@ -4222,6 +4229,8 @@ static int thin_ctr(struct dm_target *ti, unsigned argc, char **argv)
 		goto bad_pool;
 	}
 
+	dm_pool_set_in_service(tc->pool->pmd);
+
 	r = dm_pool_open_thin_device(tc->pool->pmd, tc->dev_id, &tc->td);
 	if (r) {
 		ti->error = "Couldn't open thin internal device";
-- 
2.18.0

[PATCH 3/3] dm thin metadata: check __commit_transaction()'s return

[Mike Snitzer]

Fix __reserve_metadata_snap() to return early if __commit_transaction()
fails.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-thin-metadata.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c
index f4e8327abc75..3ba27f5f7da9 100644
--- a/drivers/md/dm-thin-metadata.c
+++ b/drivers/md/dm-thin-metadata.c
@@ -1248,7 +1248,12 @@ static int __reserve_metadata_snap(struct dm_pool_metadata *pmd)
 	 * We commit to ensure the btree roots which we increment in a
 	 * moment are up to date.
 	 */
-	__commit_transaction(pmd);
+	r = __commit_transaction(pmd);
+	if (r < 0) {
+		DMWARN("%s: __commit_transaction() failed, error = %d",
+		       __func__, r);
+		return r;
+	}
 
 	/*
 	 * Copy the superblock.
-- 
2.18.0