On 07/24/2014 10:10 AM, Ilya Dryomov wrote:
> On Thu, Jul 24, 2014 at 5:46 PM, Ilya Dryomov <ilya.dryomov@xxxxxxxxxxx> wrote:
>> On Thu, Jul 24, 2014 at 5:25 PM, Alex Elder <elder@xxxxxxxx> wrote:
>>> On 07/24/2014 03:42 AM, Ilya Dryomov wrote:
>>>> There is no sense in trying to update the mapping size before it's even
>>>> been set.
>>>
>>> It took me a bit to follow this. But basically there is
>>> no mapping unless it's mapped. So previously this was
>>> updating the mapping information even for unmapped
>>> parent (or other) images. There's no need to update
>>> the mapping size for a snapshot--it'll never change.
>>>
>>> Is that right? If not, please advise; otherwise:
>>
>> No. Previously it was updating the mapping size both on the inital map
>> and on refresh (of the base image). Doing that on the inital map
>> doesn't make sense: not only struct rbd_mapping fields aren't properly
>> initialized at that point - rbd_dev_mapping_set() is called much later
>> in the map sequence, snap_id isn't initialized either (at least in the
>> format 2 case, I haven't looked too closely at the format 1 case). And
>> just in general, trying to update something before it had a chance to
>> change is bogus..
>
> BTW, while we are on the subject of struct rbd_mapping, is there any
> particular reason to keep around both the base image values and actual
> mapping values? I am tempted to change the mapping sequence so that 1)
> snap context is read in immediately after watch setup, before anything
> else is done, 2) supplied snap name is resolved into an id and 3) the
> actual (i.e. based on snap_id) mapping size, features, parent_overlap,
> etc are read in directly into struct rbd_image_header. That would
> allow to rip struct rbd_mapping entirely and make the code more clear.
The rbd_mapping structure started out well-intentioned but
over time it seems clear it hasn't added the value it was
intended to add. Here's where it got created:
f84344f rbd: separate mapping info in rbd_dev
The only original field that survives is read_only. There's
no harm at all in just moving the fields in that structure
out into the rbd_device structure.
Preserving the base image size in the header structure is
an artifact of the version 1 code, where it held the last
version of the on-disk header data. The version 2 code
maintains it for consistency.
You could eliminate that I suppose. I think the result
would require rbd_header_from_disk() to know about the
mapping rather than doing a simple conversion of data
from one form to another.
I say try it, and if you like the result I probably will too...
-Alex
> Thanks,
>
> Ilya
>
--
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
