[Sheepdog] [PATCH v2] object cache: incorrect lock may lead to update lost

[Liu Yuan]

On 04/15/2012 09:53 PM, Yunkai Zhang wrote:

>  int object_cache_push(struct object_cache *oc)
>  {
>  	struct object_cache_entry *entry, *t;
> +	struct rb_root *inactive_dirty_tree;
> +	struct list_head *inactive_dirty_list;
>  	int ret = SD_RES_SUCCESS;
>  
>  	if (node_in_recovery())
>  		/* We don't do flushing in recovery */
>  		return SD_RES_SUCCESS;
>  
> -	list_for_each_entry_safe(entry, t, &oc->dirty_list, list) {
> +	pthread_mutex_lock(&oc->lock);
> +	switch_dirty_tree_and_list(oc,
> +			&inactive_dirty_tree,
> +			&inactive_dirty_list);
> +	pthread_mutex_unlock(&oc->lock);
> +
> +	/* 1. for async flush, there is only one worker
> +	 * 2. for sync flush, Guest assure us of that only one sync
> +	 * request is issued in one of gateway worker threads
> +	 * So we need not to protect inactive dirty tree and list */
> +	list_for_each_entry_safe(entry, t, inactive_dirty_list, list) {
>  		ret = push_cache_object(oc->vid, entry->idx, entry->create);
> -		if (ret != SD_RES_SUCCESS)
> -			goto out;
> -		pthread_mutex_lock(&oc->lock);
> -		rb_erase(&entry->rb, &oc->dirty_rb);
> -		list_del(&entry->list);
> -		pthread_mutex_unlock(&oc->lock);
> +		if (ret != SD_RES_SUCCESS) {
> +			goto push_failed;
> +		}
> +		del_from_dirty_tree_and_list(entry,
> +				inactive_dirty_tree,
> +				inactive_dirty_list);
>  		free(entry);
>  	}


right here I think we need return ret; to avoid unnecessary call of
merge_dirty_tree_and_list()

> -out:
> +push_failed:
> +	pthread_mutex_lock(&oc->lock);
> +	merge_dirty_tree_and_list(oc,
> +			inactive_dirty_tree,
> +			inactive_dirty_list);
> +	pthread_mutex_unlock(&oc->lock);
>  	return ret;
>  }