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

[Yunkai Zhang]

On Sun, Apr 15, 2012 at 10:52 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);
>>       }
>> -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;
>
>
> Would be cleaner to put lock operation inside
> switch_dirty_tree_and_list() and merge_dirty_tree_and_list().

I don't think so.

In fact, I put it out specially for three reasons:

1). It will make the lock strategy more obviously when people read
object_cache_rw/object_cache_push function.
2). As the name implies, the target of these xxx_dirty_tree_and_list
functions is *just* dirty_tree_and_list. Lock logic should not
included in it.
3). Put the lock out of them will make these functions obey the
principle of orthogonality, so we can compose them like this example
in somewhere:
        pthread_mutex_lock(oc->lock);
        switch_dirty_tree_and_list()
        ...
        merge_dirty_tree_and_list()
        ...
        add_to_dirty_tree_and_list()
        ...
        pthread_mutex_unlock(oc->lock)
>
> Thanks,
> Yuan



-- 
Yunkai Zhang
Work at Taobao