On Sun, Sep 25, 2016 at 6:44 PM, Alex Elder <elder@xxxxxxxxxx> wrote:
> On 09/19/2016 12:03 PM, Ilya Dryomov wrote:
>> Accessing obj_request->img_request union field is only valid for object
>> requests associated with an image (i.e. if obj_request_img_data_test()
>> returns true). rbd_osd_req_format_read() used to do more, but now it
>> just sets osd_req->snap_id. Standalone and stat object requests always
>> go to the HEAD revision and are fine with CEPH_NOSNAP set by libceph,
>> so get around the invalid union field use by simply not calling
>> rbd_osd_req_format_read() in those places.
>
> Since rbd_osd_req_format_{read,write} are now only called
> from rbd_img_obj_request_fill(), and they're really simple,
> maybe they could be eliminated entirely and open-coded
> instead. We already have the img_request and osd_req pointers
> in the caller's context.
>
> Stat requests go to the HEAD because they are only involved for
> a layered write, which *cannot* be a snapshot. I suppose that
> is something that could have been (or be) asserted.
The only reason it's working is img_request->snap_id has the same
offset as obj_request->img_request and we end up setting stat snap_id
to a kernel pointer. A snapshot with an id like 0xffff... can't
practically exist, so we get the HEAD response from the OSDs.
>
> Is it guaranteed/required that method calls go to the HEAD?
> I'm sure they all do now, but my question is whether there's
> a good reason that it will *always* be true. Maybe it is.
>
> Similarly, rbd_obj_read_sync() is currently used only for
> standalone object requests. But it's conceivable it could
> be useful for image objects. (I'm not too concerned about
> that though...)
>
> Anwyay, this looks fine. My reservations are all about
> ensuring these assumptions are clear in the code. Maybe
> a comment or two is warranted.
libceph initializes snap_id to CEPH_NOSNAP - that's the API. If the
client isn't setting it, the assumption is clear - HEAD.
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
