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

[Yunkai Zhang]

On Sun, Apr 15, 2012 at 11:04 PM, Liu Yuan <namei.unix at gmail.com> wrote:
> 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()

good catch.

>> -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;
>>  }



-- 
Yunkai Zhang
Work at Taobao