On Thu, Jan 16, 2014 at 04:18:34PM +0900, MORITA Kazutaka wrote:
> At Wed, 15 Jan 2014 14:37:15 +0800,
> Liu Yuan wrote:
> >
> > This will reduce the unecessary recovery procedures if multiple node events
> > happens.
> >
> > Signed-off-by: Liu Yuan <namei.u...@gmail.com>
> > ---
> > sheep/recovery.c | 28 ++++++++++++++++++++--------
> > 1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/sheep/recovery.c b/sheep/recovery.c
> > index b641dcb..6bc8b5e 100644
> > --- a/sheep/recovery.c
> > +++ b/sheep/recovery.c
> > @@ -156,16 +156,18 @@ static int search_erasure_object(uint64_t oid,
> > uint8_t idx,
> > }
> >
> > static void *read_erasure_object(uint64_t oid, uint8_t idx,
> > - struct recovery_work *rw)
> > + struct recovery_obj_work *row)
> > {
> > struct sd_req hdr;
> > unsigned rlen = get_store_objsize(oid);
> > void *buf = xvalloc(rlen);
> > + struct recovery_work *rw = &row->base;
> > struct vnode_info *old = grab_vnode_info(rw->old_vinfo), *new_old;
> > uint32_t epoch = rw->epoch, tgt_epoch = rw->tgt_epoch;
> > const struct sd_node *node;
> > uint8_t policy = get_vdi_copy_policy(oid_to_vid(oid));
> > int edp = ec_policy_to_dp(policy, NULL, NULL);
> > + int ret;
> > again:
> > if (unlikely(old->nr_zones < edp)) {
> > if (search_erasure_object(oid, idx, &old->nroot, rw,
> > @@ -188,7 +190,14 @@ again:
> > hdr.obj.tgt_epoch = tgt_epoch;
> > hdr.obj.ec_index = idx;
> >
> > - if (sheep_exec_req(&node->nid, &hdr, buf) != SD_RES_SUCCESS) {
> > + ret = sheep_exec_req(&node->nid, &hdr, buf);
> > + switch (ret) {
> > + case SD_RES_SUCCESS:
> > + goto done;
>
> I think 'break' is enough. We are abusing goto expressions in this
> function. Refactoring would be appreciated.
>
> > + case SD_RES_OLD_NODE_VER:
> > + row->stop = true;
>
> I think we need "free(buf);" and "buf = NULL;".
>
> > + break;
> > + default:
> > rollback:
> > new_old = rollback_vnode_info(&tgt_epoch, rw->cur_vinfo);
> > if (!new_old) {
> > @@ -390,7 +399,7 @@ out:
> > }
> >
> > static void *rebuild_erasure_object(uint64_t oid, uint8_t idx,
> > - struct recovery_work *rw)
> > + struct recovery_obj_work *row)
> > {
> > int len = get_store_objsize(oid);
> > char *lost = xvalloc(len);
> > @@ -411,7 +420,9 @@ static void *rebuild_erasure_object(uint64_t oid,
> > uint8_t idx,
> > for (i = 0, j = 0; i < edp && j < ed; i++) {
> > if (i == idx)
> > continue;
> > - bufs[j] = read_erasure_object(oid, i, rw);
> > + bufs[j] = read_erasure_object(oid, i, row);
> > + if (row->stop)
> > + break;
> > if (!bufs[j])
> > continue;
> > idxs[j++] = i;
> > @@ -470,11 +481,12 @@ static int recover_erasure_object(struct
> > recovery_obj_work *row)
> > int ret = -1;
> >
> > idx = local_node_copy_index(cur, oid);
> > - buf = read_erasure_object(oid, idx, rw);
> > - if (!buf)
> > - buf = rebuild_erasure_object(oid, idx, rw);
> > + buf = read_erasure_object(oid, idx, row);
> > + if (!buf && !row->stop)
>
> read_erasure_object() had better return NULL when row->stop is true.
> Then we can simplify this if condition a bit.
Ah, acutually I intended it make return NULL when row->stop = true;
Thanks,
Yuan
--
sheepdog mailing list
sheepdog@lists.wpkg.org
http://lists.wpkg.org/mailman/listinfo/sheepdog
