On 10/30/2012 05:40 AM, Alex Elder wrote:
On 10/29/2012 05:13 PM, Josh Durgin wrote:
A couple notes below, but looks good.
I responded to all of your notes below. And I will update
the code/comments as appropriate after we have a chance
to talk about it but for the time being I'm going to
commit what I posted as-is.
-Alex
Reviewed-by: Josh Durgin <josh.durgin@xxxxxxxxxxx>
On 10/26/2012 04:03 PM, Alex Elder wrote:
Group the fields that uniquely specify an rbd image into a new
reference-counted rbd_spec structure. This structure will be used
to describe the desired image when mapping an image, and when
probing parent images in layered rbd devices. Replace the set of
fields in the rbd device structure with a pointer to a dynamically
allocated rbd_spec.
Signed-off-by: Alex Elder <elder@xxxxxxxxxxx>
---
drivers/block/rbd.c | 158
+++++++++++++++++++++++++++++----------------------
1 file changed, 90 insertions(+), 68 deletions(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 388dd47..c39e238 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -112,6 +112,27 @@ struct rbd_image_header {
u64 obj_version;
};
+/*
+ * An rbd image specification.
+ *
+ * The tuple (pool_id, image_id, snap_id) is sufficient to uniquely
+ * identify an image.
+ */
This looks like it's meant to be immutable. If so, it'd be nice
to have that in the comment.
I want to be sure we agree on what that term means before saying
either way.
My aim in encapsulating this was to have a single thing represent
the identity of an image. I started with just the pool, image,
and snapshot id's, but after a bit decided the names really
belonged there too. I think the name<->id association can in
some cases change.
Also, for a given image, the id never changes, but a parent
will be specified using this, and that can change.
When I was asking about immutability, I was wondering whether access to
the fields inside an rbd_spec would need to be synchronized. If those
fields won't change, it'd be good to document that fact. Otherwise
document how they'll be accessed.
+struct rbd_spec {
+ u64 pool_id;
+ char *pool_name;
+
+ char *image_id;
+ size_t image_id_len;
+ char *image_name;
+ size_t image_name_len;
I realize you're just rearranging things in this patch, but it's a bit
odd that only image_id and image_name have corresponding lengths stored.
Those fields don't seem necessary.
That's somewhat of an artifact, carrying along what was
already there, and grouping them like this makes it more
obvious those two were different.
Both the image id and image name lengths are now used one
place and it's basically one time only--in the probe routine.
So it is not valuable to keep them around. I will remove
them in a future patch.
Sounds good.
+
+ u64 snap_id;
+ char *snap_name;
+
+ struct kref kref;
+};
+
struct rbd_options {
bool read_only;
};
@@ -189,16 +210,9 @@ struct rbd_device {
struct rbd_image_header header;
bool exists;
- char *image_id;
- size_t image_id_len;
- char *image_name;
- size_t image_name_len;
- char *header_name;
- char *pool_name;
- u64 pool_id;
+ struct rbd_spec *spec;
- char *snap_name;
- u64 snap_id;
+ char *header_name;
Are you planning to split out more stuff into a common
struct rbd_image, like header_name, exists, etc?
There's a bunch of stuff that's not specific to a particular rbd_device
in here.
Possibly. Do you mean to distinguish the Linux device side
from the rados storage side? Let's talk about this today.
Yes that's what I meant. We talked about this, and decided
this separation isn't needed yet, and may not be needed to
implement layering.
struct ceph_osd_event *watch_event;
struct ceph_osd_request *watch_request;
@@ -654,7 +668,7 @@ static int snap_by_name(struct rbd_device *rbd_dev,
const char *snap_name)
list_for_each_entry(snap, &rbd_dev->snaps, node) {
if (!strcmp(snap_name, snap->name)) {
- rbd_dev->snap_id = snap->id;
+ rbd_dev->spec->snap_id = snap->id;
rbd_dev->mapping.size = snap->size;
rbd_dev->mapping.features = snap->features;
. . .
--
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
