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 |