On Tue, Apr 30, 2013 at 10:53:55PM +0100, Mel Gorman wrote:
> On Sat, Apr 27, 2013 at 03:19:13AM +0400, Glauber Costa wrote:
> > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> > index 03e44c1..8b9c1a6 100644
> > --- a/drivers/md/bcache/btree.c
> > +++ b/drivers/md/bcache/btree.c
> > @@ -599,11 +599,12 @@ static int mca_reap(struct btree *b, struct closure *cl, unsigned min_order)
> > return 0;
> > }
> >
> > -static int bch_mca_shrink(struct shrinker *shrink, struct shrink_control *sc)
> > +static long bch_mca_scan(struct shrinker *shrink, struct shrink_control *sc)
> > {
> > struct cache_set *c = container_of(shrink, struct cache_set, shrink);
> > struct btree *b, *t;
> > unsigned long i, nr = sc->nr_to_scan;
> > + long freed = 0;
> >
> > if (c->shrinker_disabled)
> > return 0;
>
> -1 if shrinker disabled?
>
> Otherwise if the shrinker is disabled we ultimately hit this loop in
> shrink_slab_one()
My memory is very hazy on this stuff, but I recall there being another
loop that'd just spin if we always returned -1.
(It might've been /proc/sys/vm/drop_caches, or maybe that was another
bug..)
But 0 should certainly be safe - if we're always returning 0, then we're
claiming we don't have anything to shrink.
> do {
> ret = shrinker->scan_objects(shrinker, sc);
> if (ret == -1)
> break
> ....
> count_vm_events(SLABS_SCANNED, batch_size);
> total_scan -= batch_size;
>
> cond_resched();
> } while (total_scan >= batch_size);
>
> which won't break as such but we busy loop until total_scan drops and
> account for SLABS_SCANNED incorrectly.
>
> More using of mutex_lock in here which means that multiple direct reclaimers
> will contend on each other. bch_mca_shrink() checks for __GFP_WAIT but an
> atomic caller does not direct reclaim so it'll always try and contend.
>
> > @@ -611,12 +612,6 @@ static int bch_mca_shrink(struct shrinker *shrink, struct shrink_control *sc)
> > if (c->try_harder)
> > return 0;
> >
> > - /*
> > - * If nr == 0, we're supposed to return the number of items we have
> > - * cached. Not allowed to return -1.
> > - */
> > - if (!nr)
> > - return mca_can_free(c) * c->btree_pages;
> >
> > /* Return -1 if we can't do anything right now */
> > if (sc->gfp_mask & __GFP_WAIT)
> > @@ -629,14 +624,14 @@ static int bch_mca_shrink(struct shrinker *shrink, struct shrink_control *sc)
> >
> > i = 0;
> > list_for_each_entry_safe(b, t, &c->btree_cache_freeable, list) {
> > - if (!nr)
> > + if (freed >= nr)
> > break;
> >
> > if (++i > 3 &&
> > !mca_reap(b, NULL, 0)) {
> > mca_data_free(b);
> > rw_unlock(true, b);
> > - --nr;
> > + freed++;
> > }
> > }
> >
> > @@ -647,7 +642,7 @@ static int bch_mca_shrink(struct shrinker *shrink, struct shrink_control *sc)
> > if (list_empty(&c->btree_cache))
> > goto out;
> >
> > - for (i = 0; nr && i < c->bucket_cache_used; i++) {
> > + for (i = 0; i < c->bucket_cache_used; i++) {
> > b = list_first_entry(&c->btree_cache, struct btree, list);
> > list_rotate_left(&c->btree_cache);
> >
> > @@ -656,14 +651,20 @@ static int bch_mca_shrink(struct shrinker *shrink, struct shrink_control *sc)
> > mca_bucket_free(b);
> > mca_data_free(b);
> > rw_unlock(true, b);
> > - --nr;
> > + freed++;
> > } else
> > b->accessed = 0;
> > }
> > out:
> > - nr = mca_can_free(c) * c->btree_pages;
> > mutex_unlock(&c->bucket_lock);
> > - return nr;
> > + return freed;
> > +}
> > +
> > +static long bch_mca_count(struct shrinker *shrink, struct shrink_control *sc)
> > +{
> > + struct cache_set *c = container_of(shrink, struct cache_set, shrink);
> > +
> > + return mca_can_free(c) * c->btree_pages;
> > }
> >
> > void bch_btree_cache_free(struct cache_set *c)
> > @@ -732,7 +733,8 @@ int bch_btree_cache_alloc(struct cache_set *c)
> > c->verify_data = NULL;
> > #endif
> >
> > - c->shrink.shrink = bch_mca_shrink;
> > + c->shrink.count_objects = bch_mca_count;
> > + c->shrink.scan_objects = bch_mca_scan;
> > c->shrink.seeks = 4;
> > c->shrink.batch = c->btree_pages * 2;
> > register_shrinker(&c->shrink);
> > diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> > index 4d9cca4..fa8d048 100644
> > --- a/drivers/md/bcache/sysfs.c
> > +++ b/drivers/md/bcache/sysfs.c
> > @@ -535,7 +535,7 @@ STORE(__bch_cache_set)
> > struct shrink_control sc;
> > sc.gfp_mask = GFP_KERNEL;
> > sc.nr_to_scan = strtoul_or_return(buf);
> > - c->shrink.shrink(&c->shrink, &sc);
> > + c->shrink.scan_objects(&c->shrink, &sc);
> > }
> >
> > sysfs_strtoul(congested_read_threshold_us,
