[sheepdog] [PATCH v2] recovery: avoid recovering object from node left

[levin li]

On 05/24/2012 02:20 PM, Liu Yuan wrote:
> On 05/24/2012 02:00 PM, levin li wrote:
> 
>> From: levin li <xingke.lwp at taobao.com>
>>
>> v1 -- > v2:
>> rebased to the current master branch
>> ------------------------------------------------ >8
>> In the recovery path, sheep may get to old epoch at which
>> some nodes have left the cluster, we shouldn't try to recover
>> objects from these nodes, so I add a check function to check
>> whether the target node is a valid node at current epoch.
>>
>> Signed-off-by: levin li <xingke.lwp at taobao.com>
>> ---
>>  sheep/recovery.c |   20 ++++++++++++++++++--
>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/sheep/recovery.c b/sheep/recovery.c
>> index f341fc6..9d1f010 100644
>> --- a/sheep/recovery.c
>> +++ b/sheep/recovery.c
>> @@ -193,6 +193,20 @@ static void rollback_old_cur(struct sd_vnode *old, int *old_nr, int *old_copies,
>>  	*old_copies = new_old_copies;
>>  }
>>  
>> +static int check_entry_valid(struct sd_vnode *entry, struct sd_node *nodes,
>> +				int nr_nodes)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < nr_nodes; i++) {
>> +		if (!memcmp(entry->addr, nodes[i].addr, sizeof(entry->addr)) &&
>> +			entry->port == nodes[i].port)
>> +			return 0;
>> +	}
>> +
>> +	return -1;
>> +}
>> +
> 
> 
> vnodes is sorted by nodes_to_vnodes(), we'd better use a binary search
> for vnode compare.

The way to use binary search is to sort the node list(not vnode list) firstly
with our defined cmp function, and then to use bsearch with the same cmp function,
I'll update my patch soon.

thanks,

levin

> 
> function would be better renamed as 'is_valid_vnode(...)' and document
> what kind of vnode isn't considered invalid.
> 
>>  /*
>>   * Recover the object from its track in epoch history. That is,
>>   * the routine will try to recovery it from the nodes it has stayed,
>> @@ -223,8 +237,10 @@ again:
>>  		int idx;
>>  		idx = obj_to_sheep(old, old_nr, oid, i);
>>  		tgt_entry = old + idx;
>> -		ret = recover_object_from_replica(oid, tgt_entry,
>> -						  epoch, tgt_epoch);
>> +		ret = check_entry_valid(tgt_entry, rw->cur_nodes, rw->cur_nr_nodes);
>> +		if (!ret)
>> +			ret = recover_object_from_replica(oid, tgt_entry,
>> +							  epoch, tgt_epoch);
> 
> 
> !ret isn't a good practice to mean a success case, would be better coded as
> 		...
> 		ret = check_entry_valid(...)
> 		if (ret < 0)
> 			continue;
> 		....
> 
>>  		if (ret == 0) {
>>  			/* Succeed */
>>  			break;
> 
>