Re: [PATCH 17/33] readahead: context based method

Nick Piggin
2006-05-25T05:26:05Z

Wu Fengguang wrote:

>
>+/*
>+ * Look back and check history pages to estimate thrashing-threshold.
>+ */
>+static unsigned long query_page_cache_segment(struct address_space *mapping,
>+ struct file_ra_state *ra,
>+ unsigned long *remain, pgoff_t offset,
>+ unsigned long ra_min, unsigned long ra_max)
>+{
>+ pgoff_t index;
>+ unsigned long count;
>+ unsigned long nr_lookback;
>+ struct radix_tree_cache cache;
>+
>+ /*
>+ * Scan backward and check the near @ra_max pages.
>+ * The count here determines ra_size.
>+ */
>+ cond_resched();
>+ read_lock_irq(&mapping->tree_lock);
>+ index = radix_tree_scan_hole_backward(&mapping->page_tree,
>+ offset, ra_max);
>+ read_unlock_irq(&mapping->tree_lock);
>

Why do you drop this lock just to pick it up again a few instructions
down the line? (is ra_cache_hit_ok or cound_cache_hit very big or
unable to be called without the lock?)

>+
>+ *remain = offset - index;
>+
>+ if (offset == ra->readahead_index && ra_cache_hit_ok(ra))
>+ count = *remain;
>+ else if (count_cache_hit(mapping, index + 1, offset) *
>+ readahead_hit_rate >= *remain)
>+ count = *remain;
>+ else
>+ count = ra_min;
>+
>+ /*
>+ * Unnecessary to count more?
>+ */
>+ if (count < ra_max)
>+ goto out;
>+
>+ if (unlikely(ra->flags & RA_FLAG_NO_LOOKAHEAD))
>+ goto out;
>+
>+ /*
>+ * Check the far pages coarsely.
>+ * The enlarged count here helps increase la_size.
>+ */
>+ nr_lookback = ra_max * (LOOKAHEAD_RATIO + 1) *
>+ 100 / (readahead_ratio | 1);
>+
>+ cond_resched();
>+ radix_tree_cache_init(&cache);
>+ read_lock_irq(&mapping->tree_lock);
>+ for (count += ra_max; count < nr_lookback; count += ra_max) {
>+ struct radix_tree_node *node;
>+ node = radix_tree_cache_lookup_parent(&mapping->page_tree,
>+ &cache, offset - count, 1);
>+ if (!node)
>+ break;
>+ }
>+ read_unlock_irq(&mapping->tree_lock);
>

Yuck. Apart from not being commented, this depends on internal
implementation of radix-tree. This should just be packaged up in some
radix-tree function to do exactly what you want (eg. is there a hole of
N contiguous pages).

And then again you can be rid of the radix-tree cache.

Yes, it increasingly appears that you're using the cache because you're
using the wrong abstractions. Eg. this is basically half implementing
some data-structure internal detail.

--

Send instant messages to your online friends http://au.messenger.yahoo.com