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 |