[sheepdog] [PATCH 1/2] corosync: fix bug when processing blocked event

[MORITA Kazutaka]

At Wed, 11 Jul 2012 01:30:59 +0800,
Yunkai Zhang wrote:
> 
> From: Yunkai Zhang <qiushu.zyk at taobao.com>
> 
> In old code, corosync driver could not process blocked event
> correctly.
> 
> For example:
> Suppose there was two requests: R1, R2.
> 1) In queue_cluster_request(), R1 sent a BLOCK event(B1) to cluster,
>    R1 was added to sys->pending_list.
> 2) When B1 was received, cdrv_cpg_deliver() was executed, and sd_block_handler()
>    would be called in __dispatch_corosycn_one(). sd_block_handler() will get R1
>    from sys->pending_list(but not delete R1 from it), cluster_op_running was set
>    TRUE, and then queue_work().
> 3) Before cluster_op_done() of R1 was executed, R2 was coming and sent a BLOCK
>    event(B2) to cluster in queue_cluster_request().
> 4) Now, cluster_op_done() of R1 was called, R1 sent an UNBLOCK event(U1) to
>    cluster, and cluster_op_running was set FALSE.
> 5) And then, B2 was received, cdrv_cpg_deliver()->__dispatch_corosycn_one()
>    would be called. Because cluster_op_running was FALSE again, so
>    sd_block_handler() would be executed again, as R1 was also at the head of
>    sys->pending_list, then R1 would be queue_work() again, ..., this bug will
>    lead to so many segment fault.
> 
> Accord has the same problem, I will fix it in next patch. But zookeeper dirver
> works well at this situation.
> 
> Signed-off-by: Yunkai Zhang <qiushu.zyk at taobao.com>
> ---
>  sheep/cluster/corosync.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/sheep/cluster/corosync.c b/sheep/cluster/corosync.c
> index bd955bb..7810a2e 100644
> --- a/sheep/cluster/corosync.c
> +++ b/sheep/cluster/corosync.c
> @@ -38,6 +38,7 @@ static struct cpg_node cpg_nodes[SD_MAX_NODES];
>  static size_t nr_cpg_nodes;
>  static int self_elect;
>  static int join_finished;
> +static int corosync_notify_blocked;
>  
>  /* event types which are dispatched in corosync_dispatch() */
>  enum corosync_event_type {
> @@ -342,7 +343,8 @@ static int __corosync_dispatch_one(struct corosync_event *cevent)
>  		sd_leave_handler(&cevent->sender.ent, entries, nr_cpg_nodes);
>  		break;
>  	case COROSYNC_EVENT_TYPE_BLOCK:
> -		sd_block_handler(&cevent->sender.ent);
> +		if (sd_block_handler(&cevent->sender.ent))
> +			corosync_notify_blocked = 1;
>  
>  		/* block other messages until the unblock message comes */
>  		return 0;
> @@ -368,6 +370,14 @@ static void __corosync_dispatch(void)
>  			cevent = list_first_entry(&corosync_notify_list,
>  						  typeof(*cevent), list);
>  
> +		/*
> +		 * When there is unfinished blocked event, only cfgchg events
> +		 * could continue to be processed(as we have given priotiry to
> +		 * process cfgchg events now).
> +		 */
> +		if (!event_is_confchg(cevent->type) && corosync_notify_blocked)
> +			return;
> +
>  		/* update join status */
>  		if (!join_finished) {
>  			switch (cevent->type) {
> @@ -687,6 +697,8 @@ static void corosync_unblock(void *msg, size_t msg_len)
>  {
>  	send_message(COROSYNC_MSG_TYPE_UNBLOCK, 0, &this_node, NULL, 0,
>  		     msg, msg_len);
> +
> +	corosync_notify_blocked = 0;

Setting corosync_notify_blocked zero here looks wrong.  Because if
__corosync_dispatch is called before the COROSYNC_MSG_TYPE_UNBLOCK
message is arrived, sd_block_handler will be called again.  My patch
looks simpler and correct, doesn't it?

Thanks,

Kazutaka