At Fri, 27 Jul 2012 19:01:03 +0800,
levin li wrote:
>
> >> +static void reclaim_work(struct work *work)
> >> +{
> >> + struct object_cache_entry *entry, *n;
> >> + int ret;
> >> +
> >> + if (node_in_recovery())
> >> + return;
> >
> > Can you comment briefly why we cannot recaim caches while recovering?
> > I think it will help newcomer to understand the code.
> >
>
> I did it because it may makes things more complicated, just as we don't
> do flush while recovering, maybe later we make sure that there's no problem
> to do reclaim in recovery, then we can remove this.
Then, how about adding it as a TODO comment?
>
> >
> >> +
> >> + list_for_each_entry_revert_safe_rcu(entry, n,
> >> + &sys_cache.cache_lru_list, lru_list) {
> >> + unsigned data_length;
> >> + /* Reclaim cache to 80% of max size */
> >> + if (uatomic_read(&sys_cache.cache_size) <=
> >> + sys->cache_size * 8 / 10)
> >> + break;
> >> +
> >> + ret = reclaim_object(entry);
> >> + if (ret != SD_RES_SUCCESS)
> >> + continue;
> >> + if (idx_has_vdi_bit(entry->idx))
> >> + data_length = SD_INODE_SIZE;
> >> + else
> >> + data_length = SD_DATA_OBJ_SIZE;
> >> +
> >> + uatomic_sub(&sys_cache.cache_size, data_length);
> >> + free(entry);
> >> + }
> >> +
> >> + dprintf("cache reclaim complete\n");
> >> +}
> >> +
> >> +static void reclaim_done(struct work *work)
> >> +{
> >> + uatomic_set(&sys_cache.reclaiming, 0);
> >> + free(work);
> >> +}
> >> +
> >> static struct object_cache_entry *
> >> dirty_tree_insert(struct object_cache *oc, uint32_t idx,
> >> uint64_t bmap, int create)
> >> @@ -171,18 +530,21 @@ dirty_tree_insert(struct object_cache *oc, uint32_t
> >> idx,
> >> struct rb_node **p = &oc->dirty_tree.rb_node;
> >> struct rb_node *parent = NULL;
> >> struct object_cache_entry *entry;
> >> + idx = idx_mask(idx);
> >>
> >> while (*p) {
> >> parent = *p;
> >> entry = rb_entry(parent, struct object_cache_entry, dirty_node);
> >>
> >> - if (idx < entry->idx)
> >> + if (idx < idx_mask(entry->idx))
> >> p = &(*p)->rb_left;
> >> - else if (idx > entry->idx)
> >> + else if (idx > idx_mask(entry->idx))
> >> p = &(*p)->rb_right;
> >> else {
> >> /* already has this entry, merge bmap */
> >> entry->bmap |= bmap;
> >> + if (create)
> >> + entry->idx |= CACHE_CREATE_BIT;
> >> return entry;
> >> }
> >> }
> >> @@ -192,7 +554,8 @@ dirty_tree_insert(struct object_cache *oc, uint32_t
> >> idx,
> >> return NULL;
> >>
> >> entry->bmap |= bmap;
> >> - entry->flags |= ENTRY_CREATE_BIT;
> >> + if (create)
> >> + entry->idx |= CACHE_CREATE_BIT;
> >> rb_link_node(&entry->dirty_node, parent, p);
> >> rb_insert_color(&entry->dirty_node, &oc->dirty_tree);
> >> list_add(&entry->list, &oc->dirty_list);
> >> @@ -200,26 +563,6 @@ dirty_tree_insert(struct object_cache *oc, uint32_t
> >> idx,
> >> return entry;
> >> }
> >>
> >> -static struct object_cache_entry *dirty_tree_search(struct rb_root *root,
> >> - uint32_t idx)
> >> -{
> >> - struct rb_node *n = root->rb_node;
> >> - struct object_cache_entry *t;
> >> -
> >> - while (n) {
> >> - t = rb_entry(n, struct object_cache_entry, dirty_node);
> >> -
> >> - if (idx < t->idx)
> >> - n = n->rb_left;
> >> - else if (idx > t->idx)
> >> - n = n->rb_right;
> >> - else
> >> - return t; /* found it */
> >> - }
> >> -
> >> - return NULL;
> >> -}
> >> -
> >> static int create_dir_for(uint32_t vid)
> >> {
> >> int ret = 0;
> >> @@ -257,6 +600,7 @@ not_found:
> >> if (create) {
> >> cache = xzalloc(sizeof(*cache));
> >> cache->vid = vid;
> >> + cache->object_tree = RB_ROOT;
> >> create_dir_for(vid);
> >>
> >> cache->dirty_tree = RB_ROOT;
> >> @@ -271,14 +615,6 @@ out:
> >> return cache;
> >> }
> >>
> >> -static inline void
> >> -del_from_dirty_tree_and_list(struct object_cache_entry *entry,
> >> - struct rb_root *dirty_tree)
> >> -{
> >> - rb_erase(&entry->dirty_node, dirty_tree);
> >> - list_del(&entry->list);
> >> -}
> >> -
> >> /* Caller should hold the oc->lock */
> >> static inline void
> >> add_to_dirty_tree_and_list(struct object_cache *oc, uint32_t idx,
> >> @@ -289,6 +625,9 @@ add_to_dirty_tree_and_list(struct object_cache *oc,
> >> uint32_t idx,
> >> if (!entry)
> >> panic("Can not find object entry %" PRIx32 "\n", idx);
> >>
> >> + if (cache_in_reclaim(0))
> >> + return;
> >> +
> >> /* If cache isn't in reclaiming, move it
> >> * to the head of lru list */
> >> cds_list_del_rcu(&entry->lru_list);
> >> @@ -312,15 +651,18 @@ static void add_to_object_cache(struct object_cache
> >> *oc, uint32_t idx)
> >> uint32_t data_length;
> >>
> >> if (idx_has_vdi_bit(idx))
> >> - data_length = SD_INODE_SIZE / 1024;
> >> + data_length = SD_INODE_SIZE;
> >> else
> >> - data_length = SD_DATA_OBJ_SIZE / 1024;
> >> + data_length = SD_DATA_OBJ_SIZE;
> >
> > What's the intention of this change? And if you want to use the value
> > 1024, please use a macro and specify what it is.
> >
>
> Well, I will not use the magic number 1024 any more, because the 64 bits
> cache_size
> it big enough.
Then, please remove the code from the 2nd patch.
Thanks,
Kazutaka
--
sheepdog mailing list
[email protected]
http://lists.wpkg.org/mailman/listinfo/sheepdog
