[sheepdog] [Sheepdog] [PATCH] sheep: remove unregister_event from process_event_queue()

[Yunkai Zhang]

On Wed, May 16, 2012 at 3:55 AM, Christoph Hellwig <hch at infradead.org> wrote:
> On Wed, May 16, 2012 at 01:54:06AM +0800, Yunkai Zhang wrote:
>> +static void get_vdi_bitmap_from_sd_list(struct sd_node *nodes, size_t nr_nodes)
>>  {
>>       int i;
>>       /* fixme: we need this until starting up. */
>>
>> -     for (i = 0; i < sys->nr_nodes; i++)
>> -             get_vdi_bitmap_from(sys->nodes + i);
>> +     for (i = 0; i < nr_nodes; i++)
>> +             get_vdi_bitmap_from(nodes + i);
>>  }
>
> I'd suggest to simply kill get_vdi_bitmap_from_sd_list and opencode it
> in __sd_join to make the code cleaner.

I'm also confused by this code. Obviously, the for loop after
get_vdi_bitmap_from_sd_list() do a lot of duplicated works, Maybe this
code is outdated, but I need more testing to clean it, I'll have a
try.

>
>>       /*
>> +      * w->member_list contains joining node, we should
>> +      * exclude it in following operation.
>> +      */
>> +     for (i = 0, nr_nodes = 0; i < w->member_list_entries; i++) {
>> +             if (node_eq(w->member_list + i, &w->joined))
>> +                     continue;
>> +             nodes[nr_nodes++] = w->member_list[i];
>> +     }
>> +
>> +     /*
>>       * If a new comer try to join the running cluster, it only need read
>>       * one copy of bitmap from the first member.
>>       */
>>       if (sys_stat_wait_format())
>> -             get_vdi_bitmap_from(w->member_list);
>> +             get_vdi_bitmap_from(nodes);
>>       else {
>> -             get_vdi_bitmap_from_sd_list();
>> +             get_vdi_bitmap_from_sd_list(nodes, nr_nodes);
>>               for (i = 0; i < w->member_list_entries; i++)
>>                       get_vdi_bitmap_from(w->member_list + i);
>
> The else clause here iterates over member_list_entries twice, also
> why create the local nodes array if we could simplify skip it in
> the for loop.  E.g. why not:

Same as previous comments.

>
>        for (i = 0; i < w->member_list_entries; i++) {
>                if (node_eq(&w->member_list[i], &w->joined))
>                        continue;
>                get_vdi_bitmap_from(&w->member_list[i]);
>
>                /*
>                 * A newly joining node only needs to read a copy of the
>                 * bitmap from one single member node.
>                 */
>                if (sys_stat_wait_format())
>                        break;
>        }
>
>> +     if (result == CJ_RES_SUCCESS || result == CJ_RES_MASTER_TRANSFER)
>> +             update_cluster_info(jm, joined, members, nr_members);
>> +
>
> Previously we would only queue up an event for the CJ_RES_SUCCESS case,
> while the CJ_RES_MASTER_TRANSFER case handles the updates itself already,
> I think with your patch CJ_RES_MASTER_TRANSFER is handled twice now.
>
Good catch. I will give a V3 later.


-- 
Yunkai Zhang
Work at Taobao