| Message ID | 1474304608-17958-8-git-send-email-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. In any case... Reviewed-by: Alex Elder <elder@linaro.org> > Reported-by: David Disseldorp <ddiss@suse.de> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > --- > drivers/block/rbd.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 03f171067e08..8907ee6342ac 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -1943,11 +1943,10 @@ static void rbd_osd_req_callback(struct ceph_osd_request *osd_req) > > static void rbd_osd_req_format_read(struct rbd_obj_request *obj_request) > { > - struct rbd_img_request *img_request = obj_request->img_request; > struct ceph_osd_request *osd_req = obj_request->osd_req; > > - if (img_request) > - osd_req->r_snapid = img_request->snap_id; > + rbd_assert(obj_request_img_data_test(obj_request)); > + osd_req->r_snapid = obj_request->img_request->snap_id; > } > > static void rbd_osd_req_format_write(struct rbd_obj_request *obj_request) > @@ -2929,8 +2928,6 @@ static int rbd_img_obj_exists_submit(struct rbd_obj_request *obj_request) > stat_request->page_count = page_count; > stat_request->callback = rbd_img_obj_exists_callback; > > - rbd_osd_req_format_read(stat_request); > - > rbd_obj_request_submit(stat_request); > return 0; > > @@ -4026,7 +4023,6 @@ static int rbd_obj_method_sync(struct rbd_device *rbd_dev, > osd_req_op_cls_response_data_pages(obj_request->osd_req, 0, > obj_request->pages, inbound_size, > 0, false, false); > - rbd_osd_req_format_read(obj_request); > > rbd_obj_request_submit(obj_request); > ret = rbd_obj_request_wait(obj_request); > @@ -4265,7 +4261,6 @@ static int rbd_obj_read_sync(struct rbd_device *rbd_dev, > obj_request->length, > obj_request->offset & ~PAGE_MASK, > false, false); > - rbd_osd_req_format_read(obj_request); > > rbd_obj_request_submit(obj_request); > ret = rbd_obj_request_wait(obj_request); > -- 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 Mon, 19 Sep 2016 19:03:27 +0200, 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. > > Reported-by: David Disseldorp <ddiss@suse.de> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> Looks fine. Reviewed-by: David Disseldorp <ddiss@suse.de> -- 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 Sun, Sep 25, 2016 at 6:44 PM, Alex Elder <elder@linaro.org> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/26/2016 11:37 AM, Ilya Dryomov wrote: > On Sun, Sep 25, 2016 at 6:44 PM, Alex Elder <elder@linaro.org> 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 Oh wow. I wasn't even thinking about that. I was focusing on the fact that you're no longer calling rbd_osd_req_format_read() in those two places. (Looking again, I see the first sentence in your description talks about the invalid use of that field.) So yes, clearly we need to test the IMG_DATA flag before using the image request pointer. Aside from that what I said is still true. Any write *must* be going to a HEAD object. And yes, the standalone object requests go to the HEAD object, but there's nothing that really requires that Thanks for the explanation. This is a good fix. -Alex > 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@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 03f171067e08..8907ee6342ac 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -1943,11 +1943,10 @@ static void rbd_osd_req_callback(struct ceph_osd_request *osd_req) static void rbd_osd_req_format_read(struct rbd_obj_request *obj_request) { - struct rbd_img_request *img_request = obj_request->img_request; struct ceph_osd_request *osd_req = obj_request->osd_req; - if (img_request) - osd_req->r_snapid = img_request->snap_id; + rbd_assert(obj_request_img_data_test(obj_request)); + osd_req->r_snapid = obj_request->img_request->snap_id; } static void rbd_osd_req_format_write(struct rbd_obj_request *obj_request) @@ -2929,8 +2928,6 @@ static int rbd_img_obj_exists_submit(struct rbd_obj_request *obj_request) stat_request->page_count = page_count; stat_request->callback = rbd_img_obj_exists_callback; - rbd_osd_req_format_read(stat_request); - rbd_obj_request_submit(stat_request); return 0; @@ -4026,7 +4023,6 @@ static int rbd_obj_method_sync(struct rbd_device *rbd_dev, osd_req_op_cls_response_data_pages(obj_request->osd_req, 0, obj_request->pages, inbound_size, 0, false, false); - rbd_osd_req_format_read(obj_request); rbd_obj_request_submit(obj_request); ret = rbd_obj_request_wait(obj_request); @@ -4265,7 +4261,6 @@ static int rbd_obj_read_sync(struct rbd_device *rbd_dev, obj_request->length, obj_request->offset & ~PAGE_MASK, false, false); - rbd_osd_req_format_read(obj_request); rbd_obj_request_submit(obj_request); ret = rbd_obj_request_wait(obj_request);
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. Reported-by: David Disseldorp <ddiss@suse.de> Signed-off-by: Ilya Dryomov <idryomov@gmail.com> --- drivers/block/rbd.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)