From: Josh Durgin <josh.durgin@inktank.com>
To: Alex Elder <elder@inktank.com>
Cc: ceph-devel@vger.kernel.org
Subject: Re: [PATCH 6/8] rbd: define image specification structure
Date: Mon, 29 Oct 2012 15:13:51 -0700 [thread overview]
Message-ID: <508EFF9F.2010009@inktank.com> (raw)
In-Reply-To: <508B16B8.1030706@inktank.com>
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);
>
next prev parent reply other threads:[~2012-10-29 22:14 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-26 22:42 Another pile of patches Alex Elder
2012-10-26 22:44 ` [PATCH, resend] rbd: simplify rbd_rq_fn() Alex Elder
2012-10-29 20:29 ` Josh Durgin
2012-10-30 12:01 ` Alex Elder
2012-10-26 22:45 ` [PATCH] rbd: remove snapshots on error in rbd_add() Alex Elder
2012-10-29 20:36 ` Josh Durgin
2012-10-26 22:45 ` [PATCH] rbd: make pool_id a 64 bit value Alex Elder
2012-10-29 20:40 ` Josh Durgin
2012-10-26 22:48 ` [PATCH 0/2] rbd: mapping structure changes Alex Elder
2012-10-26 22:51 ` [PATCH 1/2] rbd: move snap info out of rbd_mapping struct Alex Elder
2012-10-29 20:43 ` Josh Durgin
2012-10-26 22:51 ` [PATCH 2/2] rbd: rename snap_exists field Alex Elder
2012-10-29 20:46 ` Josh Durgin
2012-10-26 22:52 ` [PATCH 0/2] rbd: consolidate argument parsing Alex Elder
2012-10-26 22:55 ` [PATCH 1/2] rbd: move ceph_parse_options() call up Alex Elder
2012-10-29 21:08 ` Josh Durgin
2012-10-26 22:55 ` [PATCH 2/2] rbd: do all argument parsing in one place Alex Elder
2012-10-29 21:13 ` Josh Durgin
2012-10-26 22:56 ` [PATCH 0/8] rbd: have rbd_add_parse_args() only parse args Alex Elder
2012-10-26 23:00 ` [PATCH 1/8] rbd: get rid of snap_name_len Alex Elder
2012-10-29 21:14 ` Josh Durgin
2012-10-26 23:00 ` [PATCH 2/8] rbd: remove options args from rbd_add_parse_args() Alex Elder
2012-10-29 21:17 ` Josh Durgin
2012-10-26 23:01 ` [PATCH 3/8] rbd: remove snap_name arg " Alex Elder
2012-10-29 21:26 ` Josh Durgin
2012-10-26 23:02 ` [PATCH 4/8] rbd: pass and populate rbd_options structure Alex Elder
2012-10-29 21:27 ` Josh Durgin
2012-10-26 23:02 ` [PATCH 5/8] rbd: have rbd_add_parse_args() return error Alex Elder
2012-10-29 21:28 ` Josh Durgin
2012-10-26 23:03 ` [PATCH 6/8] rbd: define image specification structure Alex Elder
2012-10-29 22:13 ` Josh Durgin [this message]
2012-10-30 12:40 ` Alex Elder
2012-10-30 20:13 ` Josh Durgin
2012-10-26 23:03 ` [PATCH 7/8] rbd: add reference counting to rbd_spec Alex Elder
2012-10-29 22:20 ` Josh Durgin
2012-10-30 12:59 ` Alex Elder
2012-10-30 20:17 ` Josh Durgin
2012-10-26 23:03 ` [PATCH 8/8] rbd: fill rbd_spec in rbd_add_parse_args() Alex Elder
2012-10-29 22:30 ` Josh Durgin
2012-10-30 13:09 ` Alex Elder
2012-10-30 20:18 ` Josh Durgin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=508EFF9F.2010009@inktank.com \
--to=josh.durgin@inktank.com \
--cc=ceph-devel@vger.kernel.org \
--cc=elder@inktank.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox