[PATCH] dm thin metadata: clarify __open_device functionality and fix callers

[PATCH] dm thin metadata: clarify __open_device functionality and fix callers

[Mike Snitzer]

The __open_device() error path of __create_thin() and __create_snap()
will call __close_device() even if td was not initialized by
__open_device().

Clearly define what callers should expect from __open_device()'s
return.

Also, remove redundant 'td->changed = 1' in __create_thin() --
__open_device() with create=1 will have set td->changed on successful
return.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-thin-metadata.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-thin-metadata.c b/drivers/md/dm-thin-metadata.c
index 59c4f04..28f98cb 100644
--- a/drivers/md/dm-thin-metadata.c
+++ b/drivers/md/dm-thin-metadata.c
@@ -789,6 +789,11 @@ int dm_pool_metadata_close(struct dm_pool_metadata *pmd)
 	return 0;
 }
 
+/*
+ * __open_device: lookup @td if already open, allocate @td on first open.
+ * on success, return @td with an incremented reference count.
+ * on failure, @td is not initialized.
+ */
 static int __open_device(struct dm_pool_metadata *pmd,
 			 dm_thin_id dev, int create,
 			 struct dm_thin_device **td)
@@ -803,6 +808,13 @@ static int __open_device(struct dm_pool_metadata *pmd,
 	 */
 	list_for_each_entry(td2, &pmd->thin_devices, list)
 		if (td2->id == dev) {
+			if (create) {
+				/*
+				 * inconsistency if looking to create
+				 * an already open device.
+				 */
+				return -EINVAL;
+			}
 			td2->open_count++;
 			*td = td2;
 			return 0;
@@ -817,6 +829,9 @@ static int __open_device(struct dm_pool_metadata *pmd,
 		if (r != -ENODATA || !create)
 			return r;
 
+		/*
+		 * Open is for device creation.
+		 */
 		changed = 1;
 		details_le.mapped_blocks = 0;
 		details_le.transaction_id = cpu_to_le64(pmd->trans_id);
@@ -882,12 +897,10 @@ static int __create_thin(struct dm_pool_metadata *pmd,
 
 	r = __open_device(pmd, dev, 1, &td);
 	if (r) {
-		__close_device(td);
 		dm_btree_remove(&pmd->tl_info, pmd->root, &key, &pmd->root);
 		dm_btree_del(&pmd->bl_info, dev_root);
 		return r;
 	}
-	td->changed = 1;
 	__close_device(td);
 
 	return r;
@@ -967,14 +980,15 @@ static int __create_snap(struct dm_pool_metadata *pmd,
 		goto bad;
 
 	r = __set_snapshot_details(pmd, td, origin, pmd->time);
-	if (r)
+	if (r) {
+		__close_device(td);
 		goto bad;
+	}
 
 	__close_device(td);
 	return 0;
 
 bad:
-	__close_device(td);
 	dm_btree_remove(&pmd->tl_info, pmd->root, &key, &pmd->root);
 	dm_btree_remove(&pmd->details_info, pmd->details_root,
 			&key, &pmd->details_root);
-- 
1.7.4.4

Re: [PATCH] dm thin metadata: clarify __open_device functionality and fix callers

[Joe Thornber]

On Mon, Jan 09, 2012 at 03:33:08PM -0500, Mike Snitzer wrote:
> The __open_device() error path of __create_thin() and __create_snap()
> will call __close_device() even if td was not initialized by
> __open_device().
>
> Clearly define what callers should expect from __open_device()'s
> return.
>
> Also, remove redundant 'td->changed = 1' in __create_thin() --
> __open_device() with create=1 will have set td->changed on successful
> return.

Hi Mike,

This looks like a code review patch, and as such I ACK it.

I don't think it changes any behaviour though.  Both create_thin and
create_snapshot hold a mutex when they run, and the first thing they
do is check for the existence of the device.

        r = dm_btree_lookup(&pmd->details_info, pmd->details_root,
                            &key, &details_le);
        if (!r)
                return -EEXIST;

I don't think there's a race on creation?

We need to focus on reproducing Kabi's issue.

- Joe

Re: dm thin metadata: clarify __open_device functionality and fix callers

[Mike Snitzer]

On Tue, Jan 10 2012 at  5:31am -0500,
Joe Thornber <thornber@redhat.com> wrote:

> On Mon, Jan 09, 2012 at 03:33:08PM -0500, Mike Snitzer wrote:
> > The __open_device() error path of __create_thin() and __create_snap()
> > will call __close_device() even if td was not initialized by
> > __open_device().
> >
> > Clearly define what callers should expect from __open_device()'s
> > return.
> >
> > Also, remove redundant 'td->changed = 1' in __create_thin() --
> > __open_device() with create=1 will have set td->changed on successful
> > return.
> 
> Hi Mike,
> 
> This looks like a code review patch, and as such I ACK it.

Yes, this was something that Alasdair came across when he inspected the
error path of __create_thin().  He was looking at this because Kabi was
having issues with opencount on a DM device (device held open when he
wasn't aware of any openers).
 
> I don't think it changes any behaviour though.  Both create_thin and
> create_snapshot hold a mutex when they run, and the first thing they
> do is check for the existence of the device.
> 
>         r = dm_btree_lookup(&pmd->details_info, pmd->details_root,
>                             &key, &details_le);
>         if (!r)
>                 return -EEXIST;
> 
> I don't think there's a race on creation?

Right, doesn't change any behavior -- other than fix the error path to
not decrement some random memory (uninitialized @td) in
__close_device().

(and no I wasn't suggesting there was some race on creation).