[PATCH 0/5] rbd: avoid snapshot update race

[PATCH 0/5] rbd: avoid snapshot update race

[Alex Elder]

This series ends with a patch that avoids a race involving the
initial read of an rbd image header and a change to the snapshot
context.  The problem occurs because the rbd client sets up its
watch request on the header object *after* the initial header
read, and if the snapshot context changes between them the
kernel client snapshot context will not be up-to-date.

The fix is to set up the watch before doing the initial
header read.  The recent patches, along with the patches
in this series, make doing things in this order possible.

					-Alex

[PATCH 1/5] rbd: move more initialization into rbd_dev_probe_image()
[PATCH 2/5] rbd: define rbd_header_name()
[PATCH 3/5] rbd: don't clean up watch in device release function
[PATCH 4/5] rbd: don't bother checking whether order changes
[PATCH 5/5] rbd: set up watch in rbd_dev_probe_image()

[PATCH 1/5] rbd: move more initialization into rbd_dev_probe_image()

[Alex Elder]

Move a block of initialization related to the "ceph-side" of an rbd
image out of rbd_dev_probe_finish() and into rbd_dev_probe_image().

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 5fd923f..8a9ad60 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4742,19 +4742,6 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 {
 	int ret;

-	/* no need to lock here, as rbd_dev is not registered yet */
-	ret = rbd_dev_snaps_update(rbd_dev);
-	if (ret)
-		return ret;
-
-	ret = rbd_dev_spec_update(rbd_dev);
-	if (ret)
-		goto err_out_snaps;
-
-	ret = rbd_dev_probe_parent(rbd_dev);
-	if (ret)
-		goto err_out_snaps;
-
 	/* generate unique id: find highest unique id, add one */
 	rbd_dev_id_get(rbd_dev);

@@ -4817,8 +4804,6 @@ err_out_id:
 	rbd_dev_id_put(rbd_dev);
 	if (rbd_dev->parent);
 		rbd_dev_remove_parent(rbd_dev);
-err_out_snaps:
-	rbd_remove_all_snaps(rbd_dev);

 	return ret;
 }
@@ -4849,11 +4834,29 @@ static int rbd_dev_probe_image(struct rbd_device
*rbd_dev)
 	if (ret)
 		goto out_err;

+	ret = rbd_dev_snaps_update(rbd_dev);
+	if (ret)
+		goto out_err;
+
+	ret = rbd_dev_spec_update(rbd_dev);
+	if (ret)
+		goto err_out_snaps;
+
+	ret = rbd_dev_probe_parent(rbd_dev);
+	if (ret)
+		goto err_out_snaps;
+
 	ret = rbd_dev_probe_finish(rbd_dev);
 	if (ret)
-		rbd_header_free(&rbd_dev->header);
+		goto err_out_parent;

 	return ret;
+err_out_parent:
+	rbd_header_free(&rbd_dev->header);
+	if (rbd_dev->parent);
+		rbd_dev_remove_parent(rbd_dev);
+err_out_snaps:
+	rbd_remove_all_snaps(rbd_dev);
 out_err:
 	kfree(rbd_dev->spec->image_id);
 	rbd_dev->spec->image_id = NULL;
-- 
1.7.9.5

[PATCH 2/5] rbd: define rbd_header_name()

[Alex Elder]

Set an rbd device's image format value in rbd_dev_probe_image(), and
use it in a new function rbd_header_name(), which allocates and
formats the name of the header object for the rbd device.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   77
++++++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 8a9ad60..c72dcdf 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4575,18 +4575,6 @@ out:
 static int rbd_dev_v1_probe(struct rbd_device *rbd_dev)
 {
 	int ret;
-	size_t size;
-
-	/* Record the header object name for this rbd image. */
-
-	size = strlen(rbd_dev->spec->image_name) + sizeof (RBD_SUFFIX);
-	rbd_dev->header_name = kmalloc(size, GFP_KERNEL);
-	if (!rbd_dev->header_name) {
-		ret = -ENOMEM;
-		goto out_err;
-	}
-	sprintf(rbd_dev->header_name, "%s%s",
-		rbd_dev->spec->image_name, RBD_SUFFIX);

 	/* Populate rbd image metadata */

@@ -4599,8 +4587,6 @@ static int rbd_dev_v1_probe(struct rbd_device
*rbd_dev)
 	rbd_dev->parent_spec = NULL;
 	rbd_dev->parent_overlap = 0;

-	rbd_dev->image_format = 1;
-
 	dout("discovered version 1 image, header name is %s\n",
 		rbd_dev->header_name);

@@ -4617,22 +4603,9 @@ out_err:

 static int rbd_dev_v2_probe(struct rbd_device *rbd_dev)
 {
-	size_t size;
 	int ret;
 	u64 ver = 0;

-	/*
-	 * Image id was filled in by the caller.  Record the header
-	 * object name for this rbd image.
-	 */
-	size = sizeof (RBD_HEADER_PREFIX) + strlen(rbd_dev->spec->image_id);
-	rbd_dev->header_name = kmalloc(size, GFP_KERNEL);
-	if (!rbd_dev->header_name)
-		return -ENOMEM;
-	sprintf(rbd_dev->header_name, "%s%s",
-			RBD_HEADER_PREFIX, rbd_dev->spec->image_id);
-
-	/* Get the size and object order for the image */
 	ret = rbd_dev_v2_image_size(rbd_dev);
 	if (ret)
 		goto out_err;
@@ -4679,8 +4652,6 @@ static int rbd_dev_v2_probe(struct rbd_device
*rbd_dev)
 		goto out_err;
 	rbd_dev->header.obj_version = ver;

-	rbd_dev->image_format = 2;
-
 	dout("discovered version 2 image, header name is %s\n",
 		rbd_dev->header_name);

@@ -4808,6 +4779,33 @@ err_out_id:
 	return ret;
 }

+static int rbd_dev_header_name(struct rbd_device *rbd_dev)
+{
+	struct rbd_spec *spec = rbd_dev->spec;
+	size_t size;
+
+	/* Record the header object name for this rbd image. */
+
+	rbd_assert(rbd_image_format_valid(rbd_dev->image_format));
+
+	if (rbd_dev->image_format == 1)
+		size = strlen(spec->image_name) + sizeof (RBD_SUFFIX);
+	else
+		size = sizeof (RBD_HEADER_PREFIX) + strlen(spec->image_id);
+
+	rbd_dev->header_name = kmalloc(size, GFP_KERNEL);
+	if (!rbd_dev->header_name)
+		return -ENOMEM;
+
+	if (rbd_dev->image_format == 1)
+		sprintf(rbd_dev->header_name, "%s%s",
+			spec->image_name, RBD_SUFFIX);
+	else
+		sprintf(rbd_dev->header_name, "%s%s",
+			RBD_HEADER_PREFIX, spec->image_id);
+	return 0;
+}
+
 /*
  * Probe for the existence of the header object for the given rbd
  * device.  For format 2 images this includes determining the image
@@ -4825,18 +4823,24 @@ static int rbd_dev_probe_image(struct rbd_device
*rbd_dev)
 	ret = rbd_dev_image_id(rbd_dev);
 	if (ret)
 		return ret;
-
 	rbd_assert(rbd_dev->spec->image_id);
-	if (*rbd_dev->spec->image_id)
-		ret = rbd_dev_v2_probe(rbd_dev);
-	else
-		ret = rbd_dev_v1_probe(rbd_dev);
+
+	rbd_dev->image_format = *rbd_dev->spec->image_id ? 2 : 1;
+
+	ret = rbd_dev_header_name(rbd_dev);
 	if (ret)
 		goto out_err;

+	if (rbd_dev->image_format == 1)
+		ret = rbd_dev_v1_probe(rbd_dev);
+	else
+		ret = rbd_dev_v2_probe(rbd_dev);
+	if (ret)
+		goto out_header_name;
+
 	ret = rbd_dev_snaps_update(rbd_dev);
 	if (ret)
-		goto out_err;
+		goto out_header_name;

 	ret = rbd_dev_spec_update(rbd_dev);
 	if (ret)
@@ -4857,6 +4861,9 @@ err_out_parent:
 		rbd_dev_remove_parent(rbd_dev);
 err_out_snaps:
 	rbd_remove_all_snaps(rbd_dev);
+out_header_name:
+	kfree(rbd_dev->header_name);
+	rbd_dev->header_name = NULL;
 out_err:
 	kfree(rbd_dev->spec->image_id);
 	rbd_dev->spec->image_id = NULL;
-- 
1.7.9.5

[PATCH 3/5] rbd: don't clean up watch in device release function

[Alex Elder]

Currently, a watch on an rbd device header object gets torn down
when its final Linux device reference gets dropped.  Instead, tear
it down when removing the device.  If an error occurs cleaning up
the watch event, abort the unmap request.

All images (including parents) still get watch requests set up, so
tear these down also, in rbd_dev_remove_parent().  For now, ignore
any errors that occur in this case.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index c72dcdf..d7a36d1 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4713,6 +4713,10 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 {
 	int ret;

+	ret = rbd_dev_header_watch_sync(rbd_dev, 1);
+	if (ret)
+		return ret;
+
 	/* generate unique id: find highest unique id, add one */
 	rbd_dev_id_get(rbd_dev);

@@ -4743,10 +4747,6 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 	 * of the sysfs code (initiated by rbd_bus_del_dev()).
 	 */

-	ret = rbd_dev_header_watch_sync(rbd_dev, 1);
-	if (ret)
-		goto err_out_bus;
-
 	ret = rbd_dev_set_mapping(rbd_dev);
 	if (ret)
 		goto err_out_bus;
@@ -4775,6 +4775,7 @@ err_out_id:
 	rbd_dev_id_put(rbd_dev);
 	if (rbd_dev->parent);
 		rbd_dev_remove_parent(rbd_dev);
+	(void)rbd_dev_header_watch_sync(rbd_dev, 0);

 	return ret;
 }
@@ -4970,9 +4971,6 @@ static void rbd_dev_release(struct device *dev)
 {
 	struct rbd_device *rbd_dev = dev_to_rbd_dev(dev);

-	if (rbd_dev->watch_event)
-		rbd_dev_header_watch_sync(rbd_dev, 0);
-
 	/* clean up and free blkdev */
 	rbd_free_disk(rbd_dev);
 	unregister_blkdev(rbd_dev->major, rbd_dev->name);
@@ -5007,6 +5005,7 @@ static void rbd_dev_remove_parent(struct
rbd_device *rbd_dev)
 			second = third;
 		}
 		rbd_assert(second);
+		(void)rbd_dev_header_watch_sync(second, 0);
 		rbd_remove_all_snaps(second);
 		rbd_bus_del_dev(second);
 		first->parent = NULL;
@@ -5023,13 +5022,13 @@ static ssize_t rbd_remove(struct bus_type *bus,
 			  size_t count)
 {
 	struct rbd_device *rbd_dev = NULL;
-	int target_id, rc;
+	int target_id;
 	unsigned long ul;
-	int ret = count;
+	int ret;

-	rc = strict_strtoul(buf, 10, &ul);
-	if (rc)
-		return rc;
+	ret = strict_strtoul(buf, 10, &ul);
+	if (ret)
+		return ret;

 	/* convert to int; abort if we lost anything in the conversion */
 	target_id = (int) ul;
@@ -5053,6 +5052,15 @@ static ssize_t rbd_remove(struct bus_type *bus,
 	if (ret < 0)
 		goto done;

+	ret = rbd_dev_header_watch_sync(rbd_dev, 0);
+	if (ret) {
+		rbd_warn(rbd_dev, "error dropping header watch (%d)\n", ret);
+		clear_bit(RBD_DEV_FLAG_REMOVING, &rbd_dev->flags);
+		smp_mb();
+		return ret;
+	}
+	ret = count;
+
 	if (rbd_dev->parent)
 		rbd_dev_remove_parent(rbd_dev);

-- 
1.7.9.5

[PATCH 4/5] rbd: don't bother checking whether order changes

[Alex Elder]

When a format 2 image is refreshed, code is in place to verify that
the object order never changes from what it was originally.  This
relies on the fact that the refresh will occur *after* an initial
load of information about the image.

An upcoming patch makes it possible for the refresh to occur first,
so we can no longer make this order check.  The order really can't
ever change anyway--this was just a sanity check.  So get rid of it.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index d7a36d1..1832b6a 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4015,20 +4015,12 @@ static char *rbd_dev_snap_info(struct rbd_device
*rbd_dev, u32 which,
 static int rbd_dev_v2_refresh(struct rbd_device *rbd_dev, u64 *hver)
 {
 	int ret;
-	__u8 obj_order;

 	down_write(&rbd_dev->header_rwsem);

-	/* Grab old order first, to see if it changes */
-
-	obj_order = rbd_dev->header.obj_order,
 	ret = rbd_dev_v2_image_size(rbd_dev);
 	if (ret)
 		goto out;
-	if (rbd_dev->header.obj_order != obj_order) {
-		ret = -EIO;
-		goto out;
-	}
 	rbd_update_mapping_size(rbd_dev);

 	ret = rbd_dev_v2_snap_context(rbd_dev, hver);
-- 
1.7.9.5

[PATCH 5/5] rbd: set up watch in rbd_dev_probe_image()

[Alex Elder]

Move setting up the watch request for an image so it's done in
rbd_dev_probe_image() rather than rbd_dev_probe_finish().  Move
it all the way up to before doing the initial probe.  This avoids
a potential race condition, in which we get (and use) the initial
snapshot context for an image, and it gets changed between that
time and the time we get the watch set up.

This resolves:
    http://tracker.ceph.com/issues/3871

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 1832b6a..5fcf23f 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4705,10 +4705,6 @@ static int rbd_dev_probe_finish(struct rbd_device
*rbd_dev)
 {
 	int ret;

-	ret = rbd_dev_header_watch_sync(rbd_dev, 1);
-	if (ret)
-		return ret;
-
 	/* generate unique id: find highest unique id, add one */
 	rbd_dev_id_get(rbd_dev);

@@ -4767,7 +4763,6 @@ err_out_id:
 	rbd_dev_id_put(rbd_dev);
 	if (rbd_dev->parent);
 		rbd_dev_remove_parent(rbd_dev);
-	(void)rbd_dev_header_watch_sync(rbd_dev, 0);

 	return ret;
 }
@@ -4824,12 +4819,16 @@ static int rbd_dev_probe_image(struct rbd_device
*rbd_dev)
 	if (ret)
 		goto out_err;

+	ret = rbd_dev_header_watch_sync(rbd_dev, 1);
+	if (ret)
+		goto out_header_name;
+
 	if (rbd_dev->image_format == 1)
 		ret = rbd_dev_v1_probe(rbd_dev);
 	else
 		ret = rbd_dev_v2_probe(rbd_dev);
 	if (ret)
-		goto out_header_name;
+		goto err_out_watch;

 	ret = rbd_dev_snaps_update(rbd_dev);
 	if (ret)
@@ -4841,7 +4840,7 @@ static int rbd_dev_probe_image(struct rbd_device
*rbd_dev)

 	ret = rbd_dev_probe_parent(rbd_dev);
 	if (ret)
-		goto err_out_snaps;
+		goto err_out_watch;

 	ret = rbd_dev_probe_finish(rbd_dev);
 	if (ret)
@@ -4857,6 +4856,8 @@ err_out_snaps:
 out_header_name:
 	kfree(rbd_dev->header_name);
 	rbd_dev->header_name = NULL;
+err_out_watch:
+	(void)rbd_dev_header_watch_sync(rbd_dev, 0);
 out_err:
 	kfree(rbd_dev->spec->image_id);
 	rbd_dev->spec->image_id = NULL;
-- 
1.7.9.5

Re: [PATCH 0/5] rbd: avoid snapshot update race

[Alex Elder]

On 04/27/2013 02:37 PM, Alex Elder wrote:
> This series ends with a patch that avoids a race involving the
> initial read of an rbd image header and a change to the snapshot
> context.  The problem occurs because the rbd client sets up its
> watch request on the header object *after* the initial header
> read, and if the snapshot context changes between them the
> kernel client snapshot context will not be up-to-date.

This series is available in the "review/wip-rbd-cleanup-4"
in the ceph-client git repository, which is based on branch
"review/wip-rbd-cleanup-3".

					-Alex

> The fix is to set up the watch before doing the initial
> header read.  The recent patches, along with the patches
> in this series, make doing things in this order possible.
> 
> 					-Alex
> 
> [PATCH 1/5] rbd: move more initialization into rbd_dev_probe_image()
> [PATCH 2/5] rbd: define rbd_header_name()
> [PATCH 3/5] rbd: don't clean up watch in device release function
> [PATCH 4/5] rbd: don't bother checking whether order changes
> [PATCH 5/5] rbd: set up watch in rbd_dev_probe_image()
> 

Re: [PATCH 0/5] rbd: avoid snapshot update race

[Alex Elder]

On 04/27/2013 02:42 PM, Alex Elder wrote:
> On 04/27/2013 02:37 PM, Alex Elder wrote:
>> This series ends with a patch that avoids a race involving the
>> initial read of an rbd image header and a change to the snapshot
>> context.  The problem occurs because the rbd client sets up its
>> watch request on the header object *after* the initial header
>> read, and if the snapshot context changes between them the
>> kernel client snapshot context will not be up-to-date.

I have similarly updated these patches.  The new versions now
replace the old ones in the "review/wip-rbd-cleanup-4" branch
of the ceph-client git repository.

					-Alex
> This series is available in the "review/wip-rbd-cleanup-4"
> in the ceph-client git repository, which is based on branch
> "review/wip-rbd-cleanup-3".
> 
> 					-Alex
> 
>> The fix is to set up the watch before doing the initial
>> header read.  The recent patches, along with the patches
>> in this series, make doing things in this order possible.
>>
>> 					-Alex
>>
>> [PATCH 1/5] rbd: move more initialization into rbd_dev_probe_image()
>> [PATCH 2/5] rbd: define rbd_header_name()
>> [PATCH 3/5] rbd: don't clean up watch in device release function
>> [PATCH 4/5] rbd: don't bother checking whether order changes
>> [PATCH 5/5] rbd: set up watch in rbd_dev_probe_image()
>>
> 

Re: [PATCH 0/5] rbd: avoid snapshot update race

[Josh Durgin]

On 04/29/2013 10:52 AM, Alex Elder wrote:
> On 04/27/2013 02:42 PM, Alex Elder wrote:
>> On 04/27/2013 02:37 PM, Alex Elder wrote:
>>> This series ends with a patch that avoids a race involving the
>>> initial read of an rbd image header and a change to the snapshot
>>> context.  The problem occurs because the rbd client sets up its
>>> watch request on the header object *after* the initial header
>>> read, and if the snapshot context changes between them the
>>> kernel client snapshot context will not be up-to-date.
>
> I have similarly updated these patches.  The new versions now
> replace the old ones in the "review/wip-rbd-cleanup-4" branch
> of the ceph-client git repository.
>
> 					-Alex
>> This series is available in the "review/wip-rbd-cleanup-4"
>> in the ceph-client git repository, which is based on branch
>> "review/wip-rbd-cleanup-3".
>>
>> 					-Alex
>>
>>> The fix is to set up the watch before doing the initial
>>> header read.  The recent patches, along with the patches
>>> in this series, make doing things in this order possible.
>>>
>>> 					-Alex
>>>
>>> [PATCH 1/5] rbd: move more initialization into rbd_dev_probe_image()
>>> [PATCH 2/5] rbd: define rbd_header_name()
>>> [PATCH 3/5] rbd: don't clean up watch in device release function
>>> [PATCH 4/5] rbd: don't bother checking whether order changes
>>> [PATCH 5/5] rbd: set up watch in rbd_dev_probe_image()

This series looks good.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>