[sheepdog] [PATCH] sheep: no need to set SD_FLAG_CMD_WRITE for local request

[MORITA Kazutaka]

At Tue, 28 Aug 2012 18:35:46 +0800,
Liu Yuan wrote:
> 
> On 08/28/2012 05:41 PM, MORITA Kazutaka wrote:
> > At Tue, 28 Aug 2012 16:43:50 +0800,
> > levin li wrote:
> >>
> >> On 08/28/2012 04:38 PM, Liu Yuan wrote:
> >>> On 08/28/2012 04:14 PM, levin li wrote:
> >>>> -	hdr.flags = SD_FLAG_CMD_WRITE;
> >>>>  	hdr.data_length = sizeof(sys->this_node);
> >>>
> >>> Why not? We do write &sys->this_node as data to transfer.
> >>>
> >>> Thanks,
> >>> Yuan
> >>>
> >>
> >> For cluster requests we send data through cluster driver (corosync or zookeeper),
> >> in which case SD_FLAG_CMD_WRITE is never used.
> > 
> > This change looks really strange to me.  If SD_FLAG_CMD_WRITE is set,
> > hdr.data_length means the size of the sent data, and if not, it means
> > the size of receive data buffer.
> > 
> > I think we shouldn't do multicast the data when SD_FLAG_CMD_WRITE is
> > not set.
> > 
> 
> This is twofold. Strictly checking sometimes cause trouble for
> programmers, at least for me, I had several gloomy times debugging when
> I forgot to assign SD_FLAG_CMD_WRITE. It is easy to forget especially we
> set opcode with WRITE. Even for now, I am considering if we can remove
> this flag, because we might get data direction from opcode.
> 
> So IIUC, SD_FLAG_CMD_WRITE is used for header handling, indicating if
> there is any data to read from the payload. For local request, we don't
> need data transfer at all, the request is made up locally.

When sheep receives a requests without SD_FLAG_CMD_WRITE, begin_rx()
doesn't set req->data, so in such case prepare_cluster_msg() shouldn't
set the invalid data to msg->data, no?  I believe multicasting invalid
data makes it difficult for us to debug the code when we forget to add
SD_FLAG_CMD_WRITE.  If it is true, we need to add SD_FLAG_CMD_WRITE
even when the request is a local one because prepare_cluster_msg()
cannot know whether the request is from local or not.

I think it is worth considering to remove SD_FLAG_CMD_WRITE
completely, but it is another issue to be discussed.  With the current
design, we should add the flag when sending data strictly, at least
for now.  With this patch, notify_recovery_completion_work() looks
like trying to receive some data with SD_OP_COMPLETE_RECOVERY to me...

Thanks,

Kazutaka