messages/MOSDOp: a fixes of encode_payload

[heyingstar]

If we set config ms_dump_on_send,the function encode may run twice.Then data will double of normal data.
the function SimpleMessenger::submit_message or AsyncMessenger::submit_message may run m->encode(-1, MSG_CRC_ALL).This code can run MOSDOp::encode_payload.
when the function of encode run twice.the code OSDOp::merge_osd_op_vector_in_data will run twice.
so we should add data.clear() at begin of encode_payload.

Signed-off-by: Ying He heyingbj@inspur.com

[heyingstar]

@joscollin make check failed?why?please give me some help.Thinks.

[joscollin]

Jenkins retest this please

[joscollin]

@heyingstar I have given a retest.

[heyingstar]

@yuyuyu101 please help me for review this.Thanks.

[heyingstar]

@liewegas can you help me for review this?Thanks.

`

[gregsfortytwo]

I'm concerned about this. MOSDOp has other functions which write directly into the data bufferlist, although I'm not sure they're actually used — can you remove them if they're not? If they are in use, we'll need to do something to prevent them causing issues.
You did catch the only user of get_data() which seems to break this though, so good job there! :)

[heyingstar]

@gregsfortytwo Yes,I know it.we should make sure the data is private.if someone hope to add data to msg,who must add it to ops[0].indata.
For example the code ::encode(map, subop->get_data()); replace of ::encode(map, scrub[0].indata);.
BTW,I don't have better idea of this mistake.Please send me a message when you have another idea.Thank you.

If i could modify more codes,I can do this:
const bufferlist& get_data() { return data; },

[gregsfortytwo]

Sounds like you've identified the basic possibilities. I think I'd prefer making get_data() return a const bufferlist& and making data a private member? Not sure though.

[heyingstar]

@gregsfortytwo If i fix this mistake with const bufferlist& get_data();.I should modify many many code,and modify many many modules. I think this will be a new feature,not a bug fix.
Can I fix this bug with data.clear(); ?
And I will pull a new feature with const bufferlist& get_data(); in next weak or next next weak.
what is your mean?
thanks.

[gregsfortytwo]

My concern is that this is a very high-risk change. If anybody adds direct data bufferlist manipulation, or we missed some obtuse way it happens, then it breaks horribly and it won't be clear why.

[gregsfortytwo]

Couldn't we also just mark a flag for if the op vector has already been merged into the data bl, and not do it repeatedly? That seems like a much simpler, smaller fix!

[heyingstar]

@gregsfortytwo Yes,I see.
I think you are right.If we fix this bug with much simpler,smaller code.Someone else can use this code as before.
I will modify this PR with your mind.Thank you. :)

And I will pull a new feature with const bufferlist& get_data().We can talk about this in new PR.

[amitkumar50]

@heyingstar tab space left

[heyingstar]

Thanks.
I update code now.

[amitkumar50]

@heyingstar Tab space left

[heyingstar]

@gregsfortytwo I update the code now with your mind.
I think your are very right.This is the best choice of the bug fix now.
Thanks.

[gregsfortytwo]

Can you squash this down to a single commit? We don't want both of them and the master merge in patch history. :)

[heyingstar]

Ok.
Wait a moment.

[heyingstar]

@gregsfortytwo I squash this PR to a single commit.

[gregsfortytwo]

Reviewed-by: Greg Farnum gfarnum@redhat.com

This still needs to get run through an integration branch test suite before merge; it may be a few days but will happen eventually.

[yuriw]

ready for merge

http://pulpito.ceph.com/yuriw-2017-08-21_21:16:27-rados-wip-yuri-testing_2017_8_22-distro-basic-smithi/

http://pulpito.ceph.com/yuriw-2017-08-22_14:54:54-rados-wip-yuri-testing_2017_8_22-distro-basic-smithi/

[yuriw]

jenkins test docs

[alfredodeza]

jenkins test docs