[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.
> 
> function would be better renamed as 'is_valid_vnode(...)' and document
> what kind of vnode isn't considered invalid.
> 

I thought of binary search first, but the vnodes are sorted by the vnode ID,
which is recalculated every time the nodes change, it's not a constant value
for a specified address and port, so we can not use binary search here.

thanks,

levin

>>  /*
>>   * 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;
> 
>