[sheepdog] [PATCH] sheep/recovery: fall out if get SD_RES_OLD_NODE_VER

[Liu Yuan]

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.unix at 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