| Message ID | 508B16C9.8040708@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/26/2012 04:03 PM, Alex Elder wrote: > With layered images we'll share rbd_spec structures, so add a > reference count to it. It neatens up some code also. Could you explain your plan for these data structures? What will the structs and their relationships look like with clones mapped? > A silly get/put pair is added to the alloc routine just to avoid > "defined but not used" warnings. It will go away soon. > > Signed-off-by: Alex Elder <elder@inktank.com> > --- > drivers/block/rbd.c | 52 > +++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 42 insertions(+), 10 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index c39e238..19c7c6b 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -2134,6 +2134,45 @@ static struct device_type rbd_snap_device_type = { > .release = rbd_snap_dev_release, > }; > > +static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec) > +{ > + kref_get(&spec->kref); > + > + return spec; > +} > + > +static void rbd_spec_free(struct kref *kref); > +static void rbd_spec_put(struct rbd_spec *spec) > +{ > + if (spec) > + kref_put(&spec->kref, rbd_spec_free); > +} > + > +static struct rbd_spec *rbd_spec_alloc(void) > +{ > + struct rbd_spec *spec; > + > + spec = kzalloc(sizeof (*spec), GFP_KERNEL); > + if (!spec) > + return NULL; > + kref_init(&spec->kref); > + > + rbd_spec_put(rbd_spec_get(spec)); /* TEMPORARY */ > + > + return spec; > +} > + > +static void rbd_spec_free(struct kref *kref) > +{ > + struct rbd_spec *spec = container_of(kref, struct rbd_spec, kref); > + > + kfree(spec->pool_name); > + kfree(spec->image_id); > + kfree(spec->image_name); > + kfree(spec->snap_name); > + kfree(spec); > +} > + > static bool rbd_snap_registered(struct rbd_snap *snap) > { > bool ret = snap->dev.type == &rbd_snap_device_type; > @@ -3165,7 +3204,7 @@ 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); > + rbd_dev->spec = rbd_spec_alloc(); > if (!rbd_dev->spec) > goto err_out_mem; > > @@ -3278,16 +3317,12 @@ err_out_probe: > err_out_client: > kfree(rbd_dev->header_name); > rbd_put_client(rbd_dev); > - kfree(rbd_dev->spec->image_id); > err_out_args: > if (ceph_opts) > ceph_destroy_options(ceph_opts); > - 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); > + rbd_spec_put(rbd_dev->spec); > kfree(rbd_dev); > > dout("Error adding device %s\n", buf); > @@ -3336,12 +3371,9 @@ 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->spec->snap_name); > - kfree(rbd_dev->spec->image_id); > kfree(rbd_dev->header_name); > - kfree(rbd_dev->spec->pool_name); > - kfree(rbd_dev->spec->image_name); > rbd_dev_id_put(rbd_dev); > + rbd_spec_put(rbd_dev->spec); > kfree(rbd_dev); > > /* release module ref */ > -- 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:20 PM, Josh Durgin wrote: > On 10/26/2012 04:03 PM, Alex Elder wrote: >> With layered images we'll share rbd_spec structures, so add a >> reference count to it. It neatens up some code also. > > Could you explain your plan for these data structures? What > will the structs and their relationships look like with > clones mapped? Like I mentioned in the previous message, this structure encapsulates the information that identifies an rbd image. In the next patch, the information defining the image to be mapped is extracted from "command line" arguments to populate the content of one of these in the argument parsing routine. In an upcoming patch, some create/destroy helper routines will be defined for an rbd_device structure. To create an rbd device, you provide an initialized (activated?) rbd_client (which includes a ceph client) and the identity of an image you want that rbd_device to map using that client. So in response to a map request via /sys/bus/rbd/add, a new rbd_client will be created in that way. Later, a mapped rbd image (v2) will continue its probing activity by probing its parent, and if it has one, an rbd_device structure will be created for that purpose, using the same client as the original/child image and the image specification fetched from the child for the parent. I suppose it's not obvious at this point why reference counting is needed. It's one of those things that is needed later on (in work that's well underway but which I have not posted yet), and this turns out to be a good time to put that in place so it's there when it's needed, shortly. In any case, the reference counting here arises because the image spec for a parent will be created and filled in while probing the child. But I want to provide that image spec for creating the parent rbd_device structure, and keeping track of who owned what got a little confusing so I decided that just sharing a reference counted structure was the best way to go. -Alex >> A silly get/put pair is added to the alloc routine just to avoid >> "defined but not used" warnings. It will go away soon. >> >> Signed-off-by: Alex Elder <elder@inktank.com> >> --- >> drivers/block/rbd.c | 52 >> +++++++++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 42 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index c39e238..19c7c6b 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -2134,6 +2134,45 @@ static struct device_type rbd_snap_device_type = { >> .release = rbd_snap_dev_release, >> }; >> >> +static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec) >> +{ >> + kref_get(&spec->kref); >> + >> + return spec; >> +} >> + >> +static void rbd_spec_free(struct kref *kref); >> +static void rbd_spec_put(struct rbd_spec *spec) >> +{ >> + if (spec) >> + kref_put(&spec->kref, rbd_spec_free); >> +} >> + >> +static struct rbd_spec *rbd_spec_alloc(void) >> +{ >> + struct rbd_spec *spec; >> + >> + spec = kzalloc(sizeof (*spec), GFP_KERNEL); >> + if (!spec) >> + return NULL; >> + kref_init(&spec->kref); >> + >> + rbd_spec_put(rbd_spec_get(spec)); /* TEMPORARY */ >> + >> + return spec; >> +} >> + >> +static void rbd_spec_free(struct kref *kref) >> +{ >> + struct rbd_spec *spec = container_of(kref, struct rbd_spec, kref); >> + >> + kfree(spec->pool_name); >> + kfree(spec->image_id); >> + kfree(spec->image_name); >> + kfree(spec->snap_name); >> + kfree(spec); >> +} >> + >> static bool rbd_snap_registered(struct rbd_snap *snap) >> { >> bool ret = snap->dev.type == &rbd_snap_device_type; >> @@ -3165,7 +3204,7 @@ 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); >> + rbd_dev->spec = rbd_spec_alloc(); >> if (!rbd_dev->spec) >> goto err_out_mem; >> >> @@ -3278,16 +3317,12 @@ err_out_probe: >> err_out_client: >> kfree(rbd_dev->header_name); >> rbd_put_client(rbd_dev); >> - kfree(rbd_dev->spec->image_id); >> err_out_args: >> if (ceph_opts) >> ceph_destroy_options(ceph_opts); >> - 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); >> + rbd_spec_put(rbd_dev->spec); >> kfree(rbd_dev); >> >> dout("Error adding device %s\n", buf); >> @@ -3336,12 +3371,9 @@ 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->spec->snap_name); >> - kfree(rbd_dev->spec->image_id); >> kfree(rbd_dev->header_name); >> - kfree(rbd_dev->spec->pool_name); >> - kfree(rbd_dev->spec->image_name); >> rbd_dev_id_put(rbd_dev); >> + rbd_spec_put(rbd_dev->spec); >> kfree(rbd_dev); >> >> /* release module ref */ >> > -- 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:59 AM, Alex Elder wrote: > On 10/29/2012 05:20 PM, Josh Durgin wrote: >> On 10/26/2012 04:03 PM, Alex Elder wrote: >>> With layered images we'll share rbd_spec structures, so add a >>> reference count to it. It neatens up some code also. >> >> Could you explain your plan for these data structures? What >> will the structs and their relationships look like with >> clones mapped? > > Like I mentioned in the previous message, this structure > encapsulates the information that identifies an rbd image. > > In the next patch, the information defining the image to > be mapped is extracted from "command line" arguments to > populate the content of one of these in the argument > parsing routine. > > In an upcoming patch, some create/destroy helper routines > will be defined for an rbd_device structure. To create an > rbd device, you provide an initialized (activated?) > rbd_client (which includes a ceph client) and the identity > of an image you want that rbd_device to map using that client. > > So in response to a map request via /sys/bus/rbd/add, a > new rbd_client will be created in that way. Later, a > mapped rbd image (v2) will continue its probing activity > by probing its parent, and if it has one, an rbd_device > structure will be created for that purpose, using the > same client as the original/child image and the image > specification fetched from the child for the parent. > > I suppose it's not obvious at this point why reference > counting is needed. It's one of those things that is > needed later on (in work that's well underway but which > I have not posted yet), and this turns out to be a > good time to put that in place so it's there when it's > needed, shortly. > > In any case, the reference counting here arises because > the image spec for a parent will be created and filled in > while probing the child. But I want to provide that image > spec for creating the parent rbd_device structure, and > keeping track of who owned what got a little confusing > so I decided that just sharing a reference counted > structure was the best way to go. This makes sense now, thanks. Reviewed-by: Josh Durgin <josh.durgin@inktank.com> > -Alex > >>> A silly get/put pair is added to the alloc routine just to avoid >>> "defined but not used" warnings. It will go away soon. >>> >>> Signed-off-by: Alex Elder <elder@inktank.com> >>> --- >>> drivers/block/rbd.c | 52 >>> +++++++++++++++++++++++++++++++++++++++++---------- >>> 1 file changed, 42 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >>> index c39e238..19c7c6b 100644 >>> --- a/drivers/block/rbd.c >>> +++ b/drivers/block/rbd.c >>> @@ -2134,6 +2134,45 @@ static struct device_type rbd_snap_device_type = { >>> .release = rbd_snap_dev_release, >>> }; >>> >>> +static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec) >>> +{ >>> + kref_get(&spec->kref); >>> + >>> + return spec; >>> +} >>> + >>> +static void rbd_spec_free(struct kref *kref); >>> +static void rbd_spec_put(struct rbd_spec *spec) >>> +{ >>> + if (spec) >>> + kref_put(&spec->kref, rbd_spec_free); >>> +} >>> + >>> +static struct rbd_spec *rbd_spec_alloc(void) >>> +{ >>> + struct rbd_spec *spec; >>> + >>> + spec = kzalloc(sizeof (*spec), GFP_KERNEL); >>> + if (!spec) >>> + return NULL; >>> + kref_init(&spec->kref); >>> + >>> + rbd_spec_put(rbd_spec_get(spec)); /* TEMPORARY */ >>> + >>> + return spec; >>> +} >>> + >>> +static void rbd_spec_free(struct kref *kref) >>> +{ >>> + struct rbd_spec *spec = container_of(kref, struct rbd_spec, kref); >>> + >>> + kfree(spec->pool_name); >>> + kfree(spec->image_id); >>> + kfree(spec->image_name); >>> + kfree(spec->snap_name); >>> + kfree(spec); >>> +} >>> + >>> static bool rbd_snap_registered(struct rbd_snap *snap) >>> { >>> bool ret = snap->dev.type == &rbd_snap_device_type; >>> @@ -3165,7 +3204,7 @@ 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); >>> + rbd_dev->spec = rbd_spec_alloc(); >>> if (!rbd_dev->spec) >>> goto err_out_mem; >>> >>> @@ -3278,16 +3317,12 @@ err_out_probe: >>> err_out_client: >>> kfree(rbd_dev->header_name); >>> rbd_put_client(rbd_dev); >>> - kfree(rbd_dev->spec->image_id); >>> err_out_args: >>> if (ceph_opts) >>> ceph_destroy_options(ceph_opts); >>> - 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); >>> + rbd_spec_put(rbd_dev->spec); >>> kfree(rbd_dev); >>> >>> dout("Error adding device %s\n", buf); >>> @@ -3336,12 +3371,9 @@ 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->spec->snap_name); >>> - kfree(rbd_dev->spec->image_id); >>> kfree(rbd_dev->header_name); >>> - kfree(rbd_dev->spec->pool_name); >>> - kfree(rbd_dev->spec->image_name); >>> rbd_dev_id_put(rbd_dev); >>> + rbd_spec_put(rbd_dev->spec); >>> kfree(rbd_dev); >>> >>> /* release module ref */ >>> >> -- 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 c39e238..19c7c6b 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -2134,6 +2134,45 @@ static struct device_type rbd_snap_device_type = { .release = rbd_snap_dev_release, }; +static struct rbd_spec *rbd_spec_get(struct rbd_spec *spec) +{ + kref_get(&spec->kref); + + return spec; +} + +static void rbd_spec_free(struct kref *kref); +static void rbd_spec_put(struct rbd_spec *spec) +{ + if (spec) + kref_put(&spec->kref, rbd_spec_free); +} + +static struct rbd_spec *rbd_spec_alloc(void) +{ + struct rbd_spec *spec; + + spec = kzalloc(sizeof (*spec), GFP_KERNEL); + if (!spec) + return NULL; + kref_init(&spec->kref); + + rbd_spec_put(rbd_spec_get(spec)); /* TEMPORARY */ + + return spec; +} + +static void rbd_spec_free(struct kref *kref) +{ + struct rbd_spec *spec = container_of(kref, struct rbd_spec, kref); + + kfree(spec->pool_name); + kfree(spec->image_id); + kfree(spec->image_name); + kfree(spec->snap_name); + kfree(spec); +} + static bool rbd_snap_registered(struct rbd_snap *snap) { bool ret = snap->dev.type == &rbd_snap_device_type; @@ -3165,7 +3204,7 @@ 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); + rbd_dev->spec = rbd_spec_alloc(); if (!rbd_dev->spec) goto err_out_mem; @@ -3278,16 +3317,12 @@ err_out_probe: err_out_client: kfree(rbd_dev->header_name); rbd_put_client(rbd_dev); - kfree(rbd_dev->spec->image_id); err_out_args: if (ceph_opts) ceph_destroy_options(ceph_opts); - 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); + rbd_spec_put(rbd_dev->spec); kfree(rbd_dev); dout("Error adding device %s\n", buf); @@ -3336,12 +3371,9 @@ 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->spec->snap_name); - kfree(rbd_dev->spec->image_id); kfree(rbd_dev->header_name); - kfree(rbd_dev->spec->pool_name); - kfree(rbd_dev->spec->image_name); rbd_dev_id_put(rbd_dev); + rbd_spec_put(rbd_dev->spec); kfree(rbd_dev); /* release module ref */
With layered images we'll share rbd_spec structures, so add a reference count to it. It neatens up some code also. A silly get/put pair is added to the alloc routine just to avoid "defined but not used" warnings. It will go away soon. Signed-off-by: Alex Elder <elder@inktank.com> --- drivers/block/rbd.c | 52 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 10 deletions(-)