| Message ID | 1416823190-10246-1-git-send-email-idryomov@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/24/2014 03:59 AM, Ilya Dryomov wrote: > CEPH_OSD_OP_DELETE is not an extent op, stop treating it as such. This > sneaked in with discard patches - it's one of the three osd ops (the > other two are CEPH_OSD_OP_TRUNCATE and CEPH_OSD_OP_ZERO) that discard > is implemented with. > > Signed-off-by: Ilya Dryomov <idryomov@redhat.com> Is the CEPH_OSD_OP_DELETE used in ceph_zero_partial_object() an extent op? If it is not, you should get rid of the BUG_ON() in osd_req_op_extent_init() that allows CEPH_OSD_OP_DELETE. And if that's the case it looks like that function or ceph_osdc_new_request() handle CEPH_OSD_OP_DELETE properly--so it's not treated as an extent op. And: osd_req_encode_op() encodes a CEPH_OSD_OP_DELETE as an extent op as well. If it *can* be an extent op (but just not as used by RBD) then it warrants a comment here that explains why it is not being initialized as an extent op. -Alex > --- > drivers/block/rbd.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 27b71a0b72d0..1df0802bf6cb 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -2370,8 +2370,12 @@ static void rbd_img_obj_request_fill(struct rbd_obj_request *obj_request, > opcode = CEPH_OSD_OP_READ; > } > > - osd_req_op_extent_init(osd_request, num_ops, opcode, offset, length, > - 0, 0); > + if (opcode == CEPH_OSD_OP_DELETE) > + osd_req_op_init(osd_request, num_ops, opcode); > + else > + osd_req_op_extent_init(osd_request, num_ops, opcode, > + offset, length, 0, 0); > + > if (obj_request->type == OBJ_REQUEST_BIO) > osd_req_op_extent_osd_data_bio(osd_request, num_ops, > obj_request->bio_list, length); > -- 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, Nov 24, 2014 at 3:23 PM, Alex Elder <elder@ieee.org> wrote: > On 11/24/2014 03:59 AM, Ilya Dryomov wrote: >> CEPH_OSD_OP_DELETE is not an extent op, stop treating it as such. This >> sneaked in with discard patches - it's one of the three osd ops (the >> other two are CEPH_OSD_OP_TRUNCATE and CEPH_OSD_OP_ZERO) that discard >> is implemented with. >> >> Signed-off-by: Ilya Dryomov <idryomov@redhat.com> > > Is the CEPH_OSD_OP_DELETE used in ceph_zero_partial_object() > an extent op? If it is not, you should get rid of the BUG_ON() > in osd_req_op_extent_init() that allows CEPH_OSD_OP_DELETE. > > And if that's the case it looks like that function or > ceph_osdc_new_request() handle CEPH_OSD_OP_DELETE > properly--so it's not treated as an extent op. > > And: osd_req_encode_op() encodes a CEPH_OSD_OP_DELETE > as an extent op as well. > > If it *can* be an extent op (but just not as used by RBD) > then it warrants a comment here that explains why it is > not being initialized as an extent op. Hi Alex, Clearly I didn't provide enough background. OSDs don't look at extent part of the op union when processing CEPH_OSD_OP_DELETE, so it's not an extent op. Zheng added support for CEPH_OSD_OP_CREATE and in the same commit changed osd_req_op_extent_init(), ceph_osdc_new_request() and osd_req_encode_op() to not allow/encode CEPH_OSD_OP_DELETE, see [1]. This patch was rebased into testing before [1] in order to not break git bisect as Zheng didn't care of rbd, which only recently started issuing CEPH_OSD_OP_DELETE for whole-object discards. [1] https://github.com/ceph/ceph-client/commit/6d23aa137d1861fc48f86ba6532458fcebcdd038 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 11/24/2014 07:30 AM, Ilya Dryomov wrote: > On Mon, Nov 24, 2014 at 3:23 PM, Alex Elder <elder@ieee.org> wrote: >> On 11/24/2014 03:59 AM, Ilya Dryomov wrote: >>> CEPH_OSD_OP_DELETE is not an extent op, stop treating it as such. This >>> sneaked in with discard patches - it's one of the three osd ops (the >>> other two are CEPH_OSD_OP_TRUNCATE and CEPH_OSD_OP_ZERO) that discard >>> is implemented with. >>> >>> Signed-off-by: Ilya Dryomov <idryomov@redhat.com> >> >> Is the CEPH_OSD_OP_DELETE used in ceph_zero_partial_object() >> an extent op? If it is not, you should get rid of the BUG_ON() >> in osd_req_op_extent_init() that allows CEPH_OSD_OP_DELETE. >> >> And if that's the case it looks like that function or >> ceph_osdc_new_request() handle CEPH_OSD_OP_DELETE >> properly--so it's not treated as an extent op. >> >> And: osd_req_encode_op() encodes a CEPH_OSD_OP_DELETE >> as an extent op as well. >> >> If it *can* be an extent op (but just not as used by RBD) >> then it warrants a comment here that explains why it is >> not being initialized as an extent op. > > Hi Alex, > > Clearly I didn't provide enough background. > > OSDs don't look at extent part of the op union when processing > CEPH_OSD_OP_DELETE, so it's not an extent op. > > Zheng added support for CEPH_OSD_OP_CREATE and in the same commit > changed osd_req_op_extent_init(), ceph_osdc_new_request() and > osd_req_encode_op() to not allow/encode CEPH_OSD_OP_DELETE, see [1]. > This patch was rebased into testing before [1] in order to not break > git bisect as Zheng didn't care of rbd, which only recently started > issuing CEPH_OSD_OP_DELETE for whole-object discards. So it sounds like my concerns were addressed in the Zheng's original patch. RBD didn't used to use OP_DELETE, and once it did, the fact that it encoded it as an extent op didn't matter because the OSD ignored anything it sent for the extent. Therefore this is just cleaning up RBD code that was not exhibiting erroneous behavior. I'm sorry I didn't update my tree before reviewing the patch... Reviewed-by: Alex Elder <elder@linaro.org> > [1] https://github.com/ceph/ceph-client/commit/6d23aa137d1861fc48f86ba6532458fcebcdd038 > > 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 27b71a0b72d0..1df0802bf6cb 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -2370,8 +2370,12 @@ static void rbd_img_obj_request_fill(struct rbd_obj_request *obj_request, opcode = CEPH_OSD_OP_READ; } - osd_req_op_extent_init(osd_request, num_ops, opcode, offset, length, - 0, 0); + if (opcode == CEPH_OSD_OP_DELETE) + osd_req_op_init(osd_request, num_ops, opcode); + else + osd_req_op_extent_init(osd_request, num_ops, opcode, + offset, length, 0, 0); + if (obj_request->type == OBJ_REQUEST_BIO) osd_req_op_extent_osd_data_bio(osd_request, num_ops, obj_request->bio_list, length);
CEPH_OSD_OP_DELETE is not an extent op, stop treating it as such. This sneaked in with discard patches - it's one of the three osd ops (the other two are CEPH_OSD_OP_TRUNCATE and CEPH_OSD_OP_ZERO) that discard is implemented with. Signed-off-by: Ilya Dryomov <idryomov@redhat.com> --- drivers/block/rbd.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)