Re: [PATCH 6/8] rbd: define image specification structure

[Josh Durgin <josh.durgin@inktank.com>]

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;
>>>
>
> . . .
>