There are a couple calls that look unnecessary noted below.
Other than that:
Reviewed-by: Josh Durgin <josh.durgin@xxxxxxxxxxx>
On 04/29/2013 11:00 AM, Alex Elder wrote:
Define a new function rbd_dev_unprobe() which undoes state changes
that occur from calling rbd_dev_v1_probe() or rbd_dev_v2_probe().
Note that this is a superset of rbd_header_free(), which is now
getting removed (it seems to have been used improperly anyway).
Flesh out rbd_dev_image_release() so it undoes exactly what
rbd_dev_image_probe() does.
This means that:
- rbd_dev_device_release() gets called when the last device
reference gets dropped;
- that undoes everything done by the rbd_dev_device_setup() call
at the end of rbd_dev_image_probe() (and nothing more), ending
by calling rbd_dev_image_release(); and
- rbd_dev_image_release() undoes everything else done by
rbd_dev_image_probe() (and this includes a call to
rbd_dev_unprobe().
This means the image and device portions of an rbd device are fairly
cleanly separated now, so error paths should be a little easier to
verify than they used to be.
Signed-off-by: Alex Elder <elder@xxxxxxxxxxx>
---
drivers/block/rbd.c | 78
+++++++++++++++++++++++++--------------------------
1 file changed, 39 insertions(+), 39 deletions(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index feaa2e9..595d7b3 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -900,18 +900,6 @@ static void rbd_dev_clear_mapping(struct rbd_device
*rbd_dev)
rbd_dev->mapping.read_only = true;
}
-static void rbd_header_free(struct rbd_image_header *header)
-{
- kfree(header->object_prefix);
- header->object_prefix = NULL;
- kfree(header->snap_sizes);
- header->snap_sizes = NULL;
- kfree(header->snap_names);
- header->snap_names = NULL;
- rbd_snap_context_put(header->snapc);
- header->snapc = NULL;
-}
-
static const char *rbd_segment_name(struct rbd_device *rbd_dev, u64 offset)
{
char *name;
@@ -4588,6 +4576,27 @@ out:
return ret;
}
+/* Undo whatever state changes are made by v1 or v2 image probe */
+
+static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
+{
+ struct rbd_image_header *header;
+
+ rbd_dev_remove_parent(rbd_dev);
+ rbd_spec_put(rbd_dev->parent_spec);
+ rbd_dev->parent_spec = NULL;
+ rbd_dev->parent_overlap = 0;
+
+ /* Free dynamic fields from the header, then zero it out */
+
+ header = &rbd_dev->header;
+ rbd_snap_context_put(header->snapc);
+ kfree(header->snap_sizes);
+ kfree(header->snap_names);
+ kfree(header->object_prefix);
+ memset(header, 0, sizeof (*header));
+}
+
static int rbd_dev_v1_probe(struct rbd_device *rbd_dev)
{
int ret;
@@ -4809,10 +4818,20 @@ static int rbd_dev_header_name(struct rbd_device
*rbd_dev)
static void rbd_dev_image_release(struct rbd_device *rbd_dev)
{
- rbd_header_free(&rbd_dev->header);
- rbd_assert(rbd_dev->rbd_client != NULL);
- rbd_spec_put(rbd_dev->parent_spec);
+ int ret;
+
+ rbd_dev_remove_parent(rbd_dev);
This was just moved to rbd_dev_unprobe(), which is called in two lines.
It seems like this call should go away, since rbd_dev_unprobe() is also
used in an error path.
+ rbd_remove_all_snaps(rbd_dev);
+ rbd_dev_unprobe(rbd_dev);
+ ret = rbd_dev_header_watch_sync(rbd_dev, 0);
+ if (ret)
+ rbd_warn(rbd_dev, "failed to cancel watch event (%d)\n", ret);
kfree(rbd_dev->header_name);
+ rbd_dev->header_name = NULL;
+ rbd_dev->image_format = 0;
+ kfree(rbd_dev->spec->image_id);
+ rbd_dev->spec->image_id = NULL;
+
rbd_dev_destroy(rbd_dev);
}
@@ -4854,7 +4873,7 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev)
ret = rbd_dev_snaps_update(rbd_dev);
if (ret)
- goto err_out_watch;
+ goto err_out_probe;
ret = rbd_dev_spec_update(rbd_dev);
if (ret)
@@ -4865,15 +4884,14 @@ static int rbd_dev_image_probe(struct rbd_device
*rbd_dev)
goto err_out_snaps;
ret = rbd_dev_device_setup(rbd_dev);
- if (ret)
- goto err_out_parent;
+ if (!ret)
+ return 0;
- return ret;
-err_out_parent:
rbd_dev_remove_parent(rbd_dev);
This call is removed in the next patch, but should probably be removed
here, since rbd_dev_unprobe() calls it now.
- rbd_header_free(&rbd_dev->header);
err_out_snaps:
rbd_remove_all_snaps(rbd_dev);
+err_out_probe:
+ rbd_dev_unprobe(rbd_dev);
err_out_watch:
tmp = rbd_dev_header_watch_sync(rbd_dev, 0);
if (tmp)
@@ -5005,7 +5023,6 @@ static void rbd_dev_remove_parent(struct
rbd_device *rbd_dev)
struct rbd_device *first = rbd_dev;
struct rbd_device *second = first->parent;
struct rbd_device *third;
- int ret;
/*
* Follow to the parent with no grandparent and
@@ -5016,11 +5033,6 @@ static void rbd_dev_remove_parent(struct
rbd_device *rbd_dev)
second = third;
}
rbd_assert(second);
- ret = rbd_dev_header_watch_sync(rbd_dev, 0);
- if (ret)
- rbd_warn(rbd_dev,
- "failed to cancel watch event (%d)\n", ret);
- rbd_remove_all_snaps(second);
rbd_bus_del_dev(second);
first->parent = NULL;
first->parent_overlap = 0;
@@ -5065,19 +5077,7 @@ static ssize_t rbd_remove(struct bus_type *bus,
spin_unlock_irq(&rbd_dev->lock);
if (ret < 0)
goto done;
-
- ret = rbd_dev_header_watch_sync(rbd_dev, 0);
- if (ret) {
- rbd_warn(rbd_dev, "failed to cancel watch event (%d)\n", ret);
- clear_bit(RBD_DEV_FLAG_REMOVING, &rbd_dev->flags);
- smp_mb();
- return ret;
- }
ret = count;
-
- rbd_dev_remove_parent(rbd_dev);
-
- rbd_remove_all_snaps(rbd_dev);
rbd_bus_del_dev(rbd_dev);
module_put(THIS_MODULE);
done:
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
