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

[Alex Elder <elder@ieee.org>]

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
>