| Message ID | 508B16B8.1030706@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
A couple notes below, but looks good. Reviewed-by: Josh Durgin <josh.durgin@inktank.com> 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@inktank.com> > --- > 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. > +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. > + > + 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. > > 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; > > @@ -669,14 +683,14 @@ static int rbd_dev_set_mapping(struct rbd_device > *rbd_dev) > { > int ret; > > - if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME, > + if (!memcmp(rbd_dev->spec->snap_name, RBD_SNAP_HEAD_NAME, > sizeof (RBD_SNAP_HEAD_NAME))) { > - rbd_dev->snap_id = CEPH_NOSNAP; > + rbd_dev->spec->snap_id = CEPH_NOSNAP; > rbd_dev->mapping.size = rbd_dev->header.image_size; > rbd_dev->mapping.features = rbd_dev->header.features; > ret = 0; > } else { > - ret = snap_by_name(rbd_dev, rbd_dev->snap_name); > + ret = snap_by_name(rbd_dev, rbd_dev->spec->snap_name); > if (ret < 0) > goto done; > rbd_dev->mapping.read_only = true; > @@ -1096,7 +1110,7 @@ static int rbd_do_request(struct request *rq, > layout->fl_stripe_unit = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER); > layout->fl_stripe_count = cpu_to_le32(1); > layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER); > - layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->pool_id); > + layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->spec->pool_id); > ret = ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno, > req, ops); > rbd_assert(ret == 0); > @@ -1261,7 +1275,7 @@ static int rbd_do_op(struct request *rq, > opcode = CEPH_OSD_OP_READ; > flags = CEPH_OSD_FLAG_READ; > snapc = NULL; > - snapid = rbd_dev->snap_id; > + snapid = rbd_dev->spec->snap_id; > payload_len = 0; > } > > @@ -1545,7 +1559,7 @@ static void rbd_rq_fn(struct request_queue *q) > down_read(&rbd_dev->header_rwsem); > > if (!rbd_dev->exists) { > - rbd_assert(rbd_dev->snap_id != CEPH_NOSNAP); > + rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP); > up_read(&rbd_dev->header_rwsem); > dout("request for non-existent snapshot"); > spin_lock_irq(q->queue_lock); > @@ -1726,13 +1740,13 @@ rbd_dev_v1_header_read(struct rbd_device > *rbd_dev, u64 *version) > ret = -ENXIO; > pr_warning("short header read for image %s" > " (want %zd got %d)\n", > - rbd_dev->image_name, size, ret); > + rbd_dev->spec->image_name, size, ret); > goto out_err; > } > if (!rbd_dev_ondisk_valid(ondisk)) { > ret = -ENXIO; > pr_warning("invalid header for image %s\n", > - rbd_dev->image_name); > + rbd_dev->spec->image_name); > goto out_err; > } > > @@ -1783,7 +1797,7 @@ static void rbd_update_mapping_size(struct > rbd_device *rbd_dev) > { > sector_t size; > > - if (rbd_dev->snap_id != CEPH_NOSNAP) > + if (rbd_dev->spec->snap_id != CEPH_NOSNAP) > return; > > size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE; > @@ -1957,7 +1971,7 @@ static ssize_t rbd_pool_show(struct device *dev, > { > struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); > > - return sprintf(buf, "%s\n", rbd_dev->pool_name); > + return sprintf(buf, "%s\n", rbd_dev->spec->pool_name); > } > > static ssize_t rbd_pool_id_show(struct device *dev, > @@ -1965,7 +1979,8 @@ static ssize_t rbd_pool_id_show(struct device *dev, > { > struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); > > - return sprintf(buf, "%llu\n", (unsigned long long) rbd_dev->pool_id); > + return sprintf(buf, "%llu\n", > + (unsigned long long) rbd_dev->spec->pool_id); > } > > static ssize_t rbd_name_show(struct device *dev, > @@ -1973,7 +1988,7 @@ static ssize_t rbd_name_show(struct device *dev, > { > struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); > > - return sprintf(buf, "%s\n", rbd_dev->image_name); > + return sprintf(buf, "%s\n", rbd_dev->spec->image_name); > } > > static ssize_t rbd_image_id_show(struct device *dev, > @@ -1981,7 +1996,7 @@ static ssize_t rbd_image_id_show(struct device *dev, > { > struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); > > - return sprintf(buf, "%s\n", rbd_dev->image_id); > + return sprintf(buf, "%s\n", rbd_dev->spec->image_id); > } > > /* > @@ -1994,7 +2009,7 @@ static ssize_t rbd_snap_show(struct device *dev, > { > struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); > > - return sprintf(buf, "%s\n", rbd_dev->snap_name); > + return sprintf(buf, "%s\n", rbd_dev->spec->snap_name); > } > > static ssize_t rbd_image_refresh(struct device *dev, > @@ -2547,11 +2562,12 @@ static int rbd_dev_snaps_update(struct > rbd_device *rbd_dev) > > /* Existing snapshot not in the new snap context */ > > - if (rbd_dev->snap_id == snap->id) > + if (rbd_dev->spec->snap_id == snap->id) > rbd_dev->exists = false; > __rbd_remove_snap_dev(snap); > dout("%ssnap id %llu has been removed\n", > - rbd_dev->snap_id == snap->id ? "mapped " : "", > + rbd_dev->spec->snap_id == snap->id ? > + "mapped " : "", > (unsigned long long) snap->id); > > /* Done with this list entry; advance */ > @@ -2869,16 +2885,17 @@ static int rbd_add_parse_args(struct rbd_device > *rbd_dev, > if (!*options) > goto out_err; /* Missing options */ > > - rbd_dev->pool_name = dup_token(&buf, NULL); > - if (!rbd_dev->pool_name) > + rbd_dev->spec->pool_name = dup_token(&buf, NULL); > + if (!rbd_dev->spec->pool_name) > goto out_mem; > - if (!*rbd_dev->pool_name) > + if (!*rbd_dev->spec->pool_name) > goto out_err; /* Missing pool name */ > > - rbd_dev->image_name = dup_token(&buf, &rbd_dev->image_name_len); > - if (!rbd_dev->image_name) > + rbd_dev->spec->image_name = > + dup_token(&buf, &rbd_dev->spec->image_name_len); > + if (!rbd_dev->spec->image_name) > goto out_mem; > - if (!*rbd_dev->image_name) > + if (!*rbd_dev->spec->image_name) > goto out_err; /* Missing image name */ > > /* > @@ -2893,11 +2910,11 @@ static int rbd_add_parse_args(struct rbd_device > *rbd_dev, > ret = -ENAMETOOLONG; > goto out_err; > } > - rbd_dev->snap_name = kmalloc(len + 1, GFP_KERNEL); > - if (!rbd_dev->snap_name) > + rbd_dev->spec->snap_name = kmalloc(len + 1, GFP_KERNEL); > + if (!rbd_dev->spec->snap_name) > goto out_mem; > - memcpy(rbd_dev->snap_name, buf, len); > - *(rbd_dev->snap_name + len) = '\0'; > + memcpy(rbd_dev->spec->snap_name, buf, len); > + *(rbd_dev->spec->snap_name + len) = '\0'; > > /* Initialize all rbd options to the defaults */ > > @@ -2921,11 +2938,11 @@ static int rbd_add_parse_args(struct rbd_device > *rbd_dev, > out_mem: > ret = -ENOMEM; > out_err: > - kfree(rbd_dev->image_name); > - rbd_dev->image_name = NULL; > - rbd_dev->image_name_len = 0; > - kfree(rbd_dev->pool_name); > - rbd_dev->pool_name = NULL; > + kfree(rbd_dev->spec->image_name); > + rbd_dev->spec->image_name = NULL; > + rbd_dev->spec->image_name_len = 0; > + kfree(rbd_dev->spec->pool_name); > + rbd_dev->spec->pool_name = NULL; > kfree(options); > > return ret; > @@ -2957,11 +2974,11 @@ static int rbd_dev_image_id(struct rbd_device > *rbd_dev) > * First, see if the format 2 image id file exists, and if > * so, get the image's persistent id from it. > */ > - size = sizeof (RBD_ID_PREFIX) + rbd_dev->image_name_len; > + size = sizeof (RBD_ID_PREFIX) + rbd_dev->spec->image_name_len; > object_name = kmalloc(size, GFP_NOIO); > if (!object_name) > return -ENOMEM; > - sprintf(object_name, "%s%s", RBD_ID_PREFIX, rbd_dev->image_name); > + sprintf(object_name, "%s%s", RBD_ID_PREFIX, rbd_dev->spec->image_name); > dout("rbd id object name is %s\n", object_name); > > /* Response will be an encoded string, which includes a length */ > @@ -2984,15 +3001,15 @@ static int rbd_dev_image_id(struct rbd_device > *rbd_dev) > ret = 0; /* rbd_req_sync_exec() can return positive */ > > p = response; > - rbd_dev->image_id = ceph_extract_encoded_string(&p, > + rbd_dev->spec->image_id = ceph_extract_encoded_string(&p, > p + RBD_IMAGE_ID_LEN_MAX, > - &rbd_dev->image_id_len, > + &rbd_dev->spec->image_id_len, > GFP_NOIO); > - if (IS_ERR(rbd_dev->image_id)) { > - ret = PTR_ERR(rbd_dev->image_id); > - rbd_dev->image_id = NULL; > + if (IS_ERR(rbd_dev->spec->image_id)) { > + ret = PTR_ERR(rbd_dev->spec->image_id); > + rbd_dev->spec->image_id = NULL; > } else { > - dout("image_id is %s\n", rbd_dev->image_id); > + dout("image_id is %s\n", rbd_dev->spec->image_id); > } > out: > kfree(response); > @@ -3008,20 +3025,21 @@ static int rbd_dev_v1_probe(struct rbd_device > *rbd_dev) > > /* Version 1 images have no id; empty string is used */ > > - rbd_dev->image_id = kstrdup("", GFP_KERNEL); > - if (!rbd_dev->image_id) > + rbd_dev->spec->image_id = kstrdup("", GFP_KERNEL); > + if (!rbd_dev->spec->image_id) > return -ENOMEM; > - rbd_dev->image_id_len = 0; > + rbd_dev->spec->image_id_len = 0; > > /* Record the header object name for this rbd image. */ > > - size = rbd_dev->image_name_len + sizeof (RBD_SUFFIX); > + size = rbd_dev->spec->image_name_len + 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->image_name, RBD_SUFFIX); > + sprintf(rbd_dev->header_name, "%s%s", > + rbd_dev->spec->image_name, RBD_SUFFIX); > > /* Populate rbd image metadata */ > > @@ -3038,8 +3056,8 @@ static int rbd_dev_v1_probe(struct rbd_device > *rbd_dev) > out_err: > kfree(rbd_dev->header_name); > rbd_dev->header_name = NULL; > - kfree(rbd_dev->image_id); > - rbd_dev->image_id = NULL; > + kfree(rbd_dev->spec->image_id); > + rbd_dev->spec->image_id = NULL; > > return ret; > } > @@ -3054,12 +3072,12 @@ static int rbd_dev_v2_probe(struct rbd_device > *rbd_dev) > * Image id was filled in by the caller. Record the header > * object name for this rbd image. > */ > - size = sizeof (RBD_HEADER_PREFIX) + rbd_dev->image_id_len; > + size = sizeof (RBD_HEADER_PREFIX) + rbd_dev->spec->image_id_len; > 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->image_id); > + RBD_HEADER_PREFIX, rbd_dev->spec->image_id); > > /* Get the size and object order for the image */ > > @@ -3147,6 +3165,9 @@ static ssize_t rbd_add(struct bus_type *bus, > rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL); > if (!rbd_dev) > return -ENOMEM; > + rbd_dev->spec = kzalloc(sizeof (*rbd_dev->spec), GFP_KERNEL); > + if (!rbd_dev->spec) > + goto err_out_mem; > > /* static rbd_device initialization */ > spin_lock_init(&rbd_dev->lock); > @@ -3167,10 +3188,10 @@ static ssize_t rbd_add(struct bus_type *bus, > > /* pick the pool */ > osdc = &rbd_dev->rbd_client->client->osdc; > - rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name); > + rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->spec->pool_name); > if (rc < 0) > goto err_out_client; > - rbd_dev->pool_id = (u64) rc; > + rbd_dev->spec->pool_id = (u64) rc; > > rc = rbd_dev_probe(rbd_dev); > if (rc < 0) > @@ -3257,15 +3278,16 @@ err_out_probe: > err_out_client: > kfree(rbd_dev->header_name); > rbd_put_client(rbd_dev); > - kfree(rbd_dev->image_id); > + kfree(rbd_dev->spec->image_id); > err_out_args: > if (ceph_opts) > ceph_destroy_options(ceph_opts); > - kfree(rbd_dev->snap_name); > - kfree(rbd_dev->image_name); > - kfree(rbd_dev->pool_name); > + kfree(rbd_dev->spec->snap_name); > + kfree(rbd_dev->spec->image_name); > + kfree(rbd_dev->spec->pool_name); > kfree(rbd_opts); > err_out_mem: > + kfree(rbd_dev->spec); > kfree(rbd_dev); > > dout("Error adding device %s\n", buf); > @@ -3314,11 +3336,11 @@ static void rbd_dev_release(struct device *dev) > rbd_header_free(&rbd_dev->header); > > /* done with the id, and with the rbd_dev */ > - kfree(rbd_dev->snap_name); > - kfree(rbd_dev->image_id); > + kfree(rbd_dev->spec->snap_name); > + kfree(rbd_dev->spec->image_id); > kfree(rbd_dev->header_name); > - kfree(rbd_dev->pool_name); > - kfree(rbd_dev->image_name); > + kfree(rbd_dev->spec->pool_name); > + kfree(rbd_dev->spec->image_name); > rbd_dev_id_put(rbd_dev); > kfree(rbd_dev); > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@inktank.com> > > 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@inktank.com> >> --- >> 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. >> +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. >> + >> + 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. >> >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@inktank.com> >> >> 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@inktank.com> >>> --- >>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. + */ +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; + + 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; 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;
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@inktank.com> --- drivers/block/rbd.c | 158 +++++++++++++++++++++++++++++---------------------- 1 file changed, 90 insertions(+), 68 deletions(-) @@ -669,14 +683,14 @@ static int rbd_dev_set_mapping(struct rbd_device *rbd_dev) { int ret; - if (!memcmp(rbd_dev->snap_name, RBD_SNAP_HEAD_NAME, + if (!memcmp(rbd_dev->spec->snap_name, RBD_SNAP_HEAD_NAME, sizeof (RBD_SNAP_HEAD_NAME))) { - rbd_dev->snap_id = CEPH_NOSNAP; + rbd_dev->spec->snap_id = CEPH_NOSNAP; rbd_dev->mapping.size = rbd_dev->header.image_size; rbd_dev->mapping.features = rbd_dev->header.features; ret = 0; } else { - ret = snap_by_name(rbd_dev, rbd_dev->snap_name); + ret = snap_by_name(rbd_dev, rbd_dev->spec->snap_name); if (ret < 0) goto done; rbd_dev->mapping.read_only = true; @@ -1096,7 +1110,7 @@ static int rbd_do_request(struct request *rq, layout->fl_stripe_unit = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER); layout->fl_stripe_count = cpu_to_le32(1); layout->fl_object_size = cpu_to_le32(1 << RBD_MAX_OBJ_ORDER); - layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->pool_id); + layout->fl_pg_pool = cpu_to_le32((int) rbd_dev->spec->pool_id); ret = ceph_calc_raw_layout(osdc, layout, snapid, ofs, &len, &bno, req, ops); rbd_assert(ret == 0); @@ -1261,7 +1275,7 @@ static int rbd_do_op(struct request *rq, opcode = CEPH_OSD_OP_READ; flags = CEPH_OSD_FLAG_READ; snapc = NULL; - snapid = rbd_dev->snap_id; + snapid = rbd_dev->spec->snap_id; payload_len = 0; } @@ -1545,7 +1559,7 @@ static void rbd_rq_fn(struct request_queue *q) down_read(&rbd_dev->header_rwsem); if (!rbd_dev->exists) { - rbd_assert(rbd_dev->snap_id != CEPH_NOSNAP); + rbd_assert(rbd_dev->spec->snap_id != CEPH_NOSNAP); up_read(&rbd_dev->header_rwsem); dout("request for non-existent snapshot"); spin_lock_irq(q->queue_lock); @@ -1726,13 +1740,13 @@ rbd_dev_v1_header_read(struct rbd_device *rbd_dev, u64 *version) ret = -ENXIO; pr_warning("short header read for image %s" " (want %zd got %d)\n", - rbd_dev->image_name, size, ret); + rbd_dev->spec->image_name, size, ret); goto out_err; } if (!rbd_dev_ondisk_valid(ondisk)) { ret = -ENXIO; pr_warning("invalid header for image %s\n", - rbd_dev->image_name); + rbd_dev->spec->image_name); goto out_err; } @@ -1783,7 +1797,7 @@ static void rbd_update_mapping_size(struct rbd_device *rbd_dev) { sector_t size; - if (rbd_dev->snap_id != CEPH_NOSNAP) + if (rbd_dev->spec->snap_id != CEPH_NOSNAP) return; size = (sector_t) rbd_dev->header.image_size / SECTOR_SIZE; @@ -1957,7 +1971,7 @@ static ssize_t rbd_pool_show(struct device *dev, { struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); - return sprintf(buf, "%s\n", rbd_dev->pool_name); + return sprintf(buf, "%s\n", rbd_dev->spec->pool_name); } static ssize_t rbd_pool_id_show(struct device *dev, @@ -1965,7 +1979,8 @@ static ssize_t rbd_pool_id_show(struct device *dev, { struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); - return sprintf(buf, "%llu\n", (unsigned long long) rbd_dev->pool_id); + return sprintf(buf, "%llu\n", + (unsigned long long) rbd_dev->spec->pool_id); } static ssize_t rbd_name_show(struct device *dev, @@ -1973,7 +1988,7 @@ static ssize_t rbd_name_show(struct device *dev, { struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); - return sprintf(buf, "%s\n", rbd_dev->image_name); + return sprintf(buf, "%s\n", rbd_dev->spec->image_name); } static ssize_t rbd_image_id_show(struct device *dev, @@ -1981,7 +1996,7 @@ static ssize_t rbd_image_id_show(struct device *dev, { struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); - return sprintf(buf, "%s\n", rbd_dev->image_id); + return sprintf(buf, "%s\n", rbd_dev->spec->image_id); } /* @@ -1994,7 +2009,7 @@ static ssize_t rbd_snap_show(struct device *dev, { struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); - return sprintf(buf, "%s\n", rbd_dev->snap_name); + return sprintf(buf, "%s\n", rbd_dev->spec->snap_name); } static ssize_t rbd_image_refresh(struct device *dev, @@ -2547,11 +2562,12 @@ static int rbd_dev_snaps_update(struct rbd_device *rbd_dev) /* Existing snapshot not in the new snap context */ - if (rbd_dev->snap_id == snap->id) + if (rbd_dev->spec->snap_id == snap->id) rbd_dev->exists = false; __rbd_remove_snap_dev(snap); dout("%ssnap id %llu has been removed\n", - rbd_dev->snap_id == snap->id ? "mapped " : "", + rbd_dev->spec->snap_id == snap->id ? + "mapped " : "", (unsigned long long) snap->id); /* Done with this list entry; advance */ @@ -2869,16 +2885,17 @@ static int rbd_add_parse_args(struct rbd_device *rbd_dev, if (!*options) goto out_err; /* Missing options */ - rbd_dev->pool_name = dup_token(&buf, NULL); - if (!rbd_dev->pool_name) + rbd_dev->spec->pool_name = dup_token(&buf, NULL); + if (!rbd_dev->spec->pool_name) goto out_mem; - if (!*rbd_dev->pool_name) + if (!*rbd_dev->spec->pool_name) goto out_err; /* Missing pool name */ - rbd_dev->image_name = dup_token(&buf, &rbd_dev->image_name_len); - if (!rbd_dev->image_name) + rbd_dev->spec->image_name = + dup_token(&buf, &rbd_dev->spec->image_name_len); + if (!rbd_dev->spec->image_name) goto out_mem; - if (!*rbd_dev->image_name) + if (!*rbd_dev->spec->image_name) goto out_err; /* Missing image name */ /* @@ -2893,11 +2910,11 @@ static int rbd_add_parse_args(struct rbd_device *rbd_dev, ret = -ENAMETOOLONG; goto out_err; } - rbd_dev->snap_name = kmalloc(len + 1, GFP_KERNEL); - if (!rbd_dev->snap_name) + rbd_dev->spec->snap_name = kmalloc(len + 1, GFP_KERNEL); + if (!rbd_dev->spec->snap_name) goto out_mem; - memcpy(rbd_dev->snap_name, buf, len); - *(rbd_dev->snap_name + len) = '\0'; + memcpy(rbd_dev->spec->snap_name, buf, len); + *(rbd_dev->spec->snap_name + len) = '\0'; /* Initialize all rbd options to the defaults */ @@ -2921,11 +2938,11 @@ static int rbd_add_parse_args(struct rbd_device *rbd_dev, out_mem: ret = -ENOMEM; out_err: - kfree(rbd_dev->image_name); - rbd_dev->image_name = NULL; - rbd_dev->image_name_len = 0; - kfree(rbd_dev->pool_name); - rbd_dev->pool_name = NULL; + kfree(rbd_dev->spec->image_name); + rbd_dev->spec->image_name = NULL; + rbd_dev->spec->image_name_len = 0; + kfree(rbd_dev->spec->pool_name); + rbd_dev->spec->pool_name = NULL; kfree(options); return ret; @@ -2957,11 +2974,11 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) * First, see if the format 2 image id file exists, and if * so, get the image's persistent id from it. */ - size = sizeof (RBD_ID_PREFIX) + rbd_dev->image_name_len; + size = sizeof (RBD_ID_PREFIX) + rbd_dev->spec->image_name_len; object_name = kmalloc(size, GFP_NOIO); if (!object_name) return -ENOMEM; - sprintf(object_name, "%s%s", RBD_ID_PREFIX, rbd_dev->image_name); + sprintf(object_name, "%s%s", RBD_ID_PREFIX, rbd_dev->spec->image_name); dout("rbd id object name is %s\n", object_name); /* Response will be an encoded string, which includes a length */ @@ -2984,15 +3001,15 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) ret = 0; /* rbd_req_sync_exec() can return positive */ p = response; - rbd_dev->image_id = ceph_extract_encoded_string(&p, + rbd_dev->spec->image_id = ceph_extract_encoded_string(&p, p + RBD_IMAGE_ID_LEN_MAX, - &rbd_dev->image_id_len, + &rbd_dev->spec->image_id_len, GFP_NOIO); - if (IS_ERR(rbd_dev->image_id)) { - ret = PTR_ERR(rbd_dev->image_id); - rbd_dev->image_id = NULL; + if (IS_ERR(rbd_dev->spec->image_id)) { + ret = PTR_ERR(rbd_dev->spec->image_id); + rbd_dev->spec->image_id = NULL; } else { - dout("image_id is %s\n", rbd_dev->image_id); + dout("image_id is %s\n", rbd_dev->spec->image_id); } out: kfree(response); @@ -3008,20 +3025,21 @@ static int rbd_dev_v1_probe(struct rbd_device *rbd_dev) /* Version 1 images have no id; empty string is used */ - rbd_dev->image_id = kstrdup("", GFP_KERNEL); - if (!rbd_dev->image_id) + rbd_dev->spec->image_id = kstrdup("", GFP_KERNEL); + if (!rbd_dev->spec->image_id) return -ENOMEM; - rbd_dev->image_id_len = 0; + rbd_dev->spec->image_id_len = 0; /* Record the header object name for this rbd image. */ - size = rbd_dev->image_name_len + sizeof (RBD_SUFFIX); + size = rbd_dev->spec->image_name_len + 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->image_name, RBD_SUFFIX); + sprintf(rbd_dev->header_name, "%s%s", + rbd_dev->spec->image_name, RBD_SUFFIX); /* Populate rbd image metadata */ @@ -3038,8 +3056,8 @@ static int rbd_dev_v1_probe(struct rbd_device *rbd_dev) out_err: kfree(rbd_dev->header_name); rbd_dev->header_name = NULL; - kfree(rbd_dev->image_id); - rbd_dev->image_id = NULL; + kfree(rbd_dev->spec->image_id); + rbd_dev->spec->image_id = NULL; return ret; } @@ -3054,12 +3072,12 @@ static int rbd_dev_v2_probe(struct rbd_device *rbd_dev) * Image id was filled in by the caller. Record the header * object name for this rbd image. */ - size = sizeof (RBD_HEADER_PREFIX) + rbd_dev->image_id_len; + size = sizeof (RBD_HEADER_PREFIX) + rbd_dev->spec->image_id_len; 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->image_id); + RBD_HEADER_PREFIX, rbd_dev->spec->image_id); /* Get the size and object order for the image */ @@ -3147,6 +3165,9 @@ static ssize_t rbd_add(struct bus_type *bus, rbd_dev = kzalloc(sizeof(*rbd_dev), GFP_KERNEL); if (!rbd_dev) return -ENOMEM; + rbd_dev->spec = kzalloc(sizeof (*rbd_dev->spec), GFP_KERNEL); + if (!rbd_dev->spec) + goto err_out_mem; /* static rbd_device initialization */ spin_lock_init(&rbd_dev->lock); @@ -3167,10 +3188,10 @@ static ssize_t rbd_add(struct bus_type *bus, /* pick the pool */ osdc = &rbd_dev->rbd_client->client->osdc; - rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->pool_name); + rc = ceph_pg_poolid_by_name(osdc->osdmap, rbd_dev->spec->pool_name); if (rc < 0) goto err_out_client; - rbd_dev->pool_id = (u64) rc; + rbd_dev->spec->pool_id = (u64) rc; rc = rbd_dev_probe(rbd_dev); if (rc < 0) @@ -3257,15 +3278,16 @@ err_out_probe: err_out_client: kfree(rbd_dev->header_name); rbd_put_client(rbd_dev); - kfree(rbd_dev->image_id); + kfree(rbd_dev->spec->image_id); err_out_args: if (ceph_opts) ceph_destroy_options(ceph_opts); - kfree(rbd_dev->snap_name); - kfree(rbd_dev->image_name); - kfree(rbd_dev->pool_name); + kfree(rbd_dev->spec->snap_name); + kfree(rbd_dev->spec->image_name); + kfree(rbd_dev->spec->pool_name); kfree(rbd_opts); err_out_mem: + kfree(rbd_dev->spec); kfree(rbd_dev); dout("Error adding device %s\n", buf); @@ -3314,11 +3336,11 @@ static void rbd_dev_release(struct device *dev) rbd_header_free(&rbd_dev->header); /* done with the id, and with the rbd_dev */ - kfree(rbd_dev->snap_name); - kfree(rbd_dev->image_id); + kfree(rbd_dev->spec->snap_name); + kfree(rbd_dev->spec->image_id); kfree(rbd_dev->header_name); - kfree(rbd_dev->pool_name); - kfree(rbd_dev->image_name); + kfree(rbd_dev->spec->pool_name); + kfree(rbd_dev->spec->image_name); rbd_dev_id_put(rbd_dev); kfree(rbd_dev);