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

Wu Fengguang
2006-05-25T08:03:11Z

On Thu, May 25, 2006 at 03:26:00PM +1000, Nick Piggin wrote:
> Wu Fengguang wrote:
> >+ 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?)

Nice catch, will fix it.

> >+
> >+ *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).

Yes, it is ugly.
Maybe we can make it a function named radix_tree_scan_hole_coarse().

> 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.

Sorry for not being aware of this problem :)

Wu