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

[Wu Fengguang]

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/