[PATCH] rbd: don't treat CEPH_OSD_OP_DELETE as extent op

[PATCH] rbd: don't treat CEPH_OSD_OP_DELETE as extent op

[Ilya Dryomov]

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(-)

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);
-- 
2.1.1

Re: [PATCH] rbd: don't treat CEPH_OSD_OP_DELETE as extent op

[Alex Elder]

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

Re: [PATCH] rbd: don't treat CEPH_OSD_OP_DELETE as extent op

[Ilya Dryomov]

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

Re: [PATCH] rbd: don't treat CEPH_OSD_OP_DELETE as extent op

[Alex Elder]

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
>