| Message ID | 1479417398-2593-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On Thu, 17 Nov 2016, Douglas Anderson wrote: > diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c > index b3ba142e59a4..885ba5482d9f 100644 > --- a/drivers/md/dm-bufio.c > +++ b/drivers/md/dm-bufio.c > @@ -89,6 +89,7 @@ struct dm_bufio_client { > > struct list_head lru[LIST_SIZE]; > unsigned long n_buffers[LIST_SIZE]; > + unsigned long n_all_buffers; > > struct block_device *bdev; > unsigned block_size; > @@ -485,6 +486,7 @@ static void __link_buffer(struct dm_buffer *b, sector_t block, int dirty) > struct dm_bufio_client *c = b->c; > > c->n_buffers[dirty]++; > + c->n_all_buffers++; > b->block = block; > b->list_mode = dirty; > list_add(&b->lru_list, &c->lru[dirty]); > @@ -502,6 +504,7 @@ static void __unlink_buffer(struct dm_buffer *b) > BUG_ON(!c->n_buffers[b->list_mode]); > > c->n_buffers[b->list_mode]--; > + c->n_all_buffers--; > __remove(b->c, b); > list_del(&b->lru_list); > } > @@ -515,6 +518,7 @@ static void __relink_lru(struct dm_buffer *b, int dirty) > > BUG_ON(!c->n_buffers[b->list_mode]); > > + /* NOTE: don't update n_all_buffers: -1 + 1 = 0 */ > c->n_buffers[b->list_mode]--; > c->n_buffers[dirty]++; > b->list_mode = dirty; > @@ -1588,17 +1592,10 @@ static unsigned long > dm_bufio_shrink_count(struct shrinker *shrink, struct shrink_control *sc) > { > struct dm_bufio_client *c; > - unsigned long count; > > c = container_of(shrink, struct dm_bufio_client, shrinker); > - if (sc->gfp_mask & __GFP_FS) > - dm_bufio_lock(c); > - else if (!dm_bufio_trylock(c)) > - return 0; > > - count = c->n_buffers[LIST_CLEAN] + c->n_buffers[LIST_DIRTY]; > - dm_bufio_unlock(c); > - return count; > + return c->n_all_buffers; > } > > /* Would be better to just avoid taking the mutex at all and returning c->n_buffers[LIST_CLEAN] + c->n_buffers[LIST_DIRTY] with a comment that the estimate might be wrong, but the actual count may vary between ->count_objects() and ->scan_objects() anyway, so we don't actually care? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c index b3ba142e59a4..885ba5482d9f 100644 --- a/drivers/md/dm-bufio.c +++ b/drivers/md/dm-bufio.c @@ -89,6 +89,7 @@ struct dm_bufio_client { struct list_head lru[LIST_SIZE]; unsigned long n_buffers[LIST_SIZE]; + unsigned long n_all_buffers; struct block_device *bdev; unsigned block_size; @@ -485,6 +486,7 @@ static void __link_buffer(struct dm_buffer *b, sector_t block, int dirty) struct dm_bufio_client *c = b->c; c->n_buffers[dirty]++; + c->n_all_buffers++; b->block = block; b->list_mode = dirty; list_add(&b->lru_list, &c->lru[dirty]); @@ -502,6 +504,7 @@ static void __unlink_buffer(struct dm_buffer *b) BUG_ON(!c->n_buffers[b->list_mode]); c->n_buffers[b->list_mode]--; + c->n_all_buffers--; __remove(b->c, b); list_del(&b->lru_list); } @@ -515,6 +518,7 @@ static void __relink_lru(struct dm_buffer *b, int dirty) BUG_ON(!c->n_buffers[b->list_mode]); + /* NOTE: don't update n_all_buffers: -1 + 1 = 0 */ c->n_buffers[b->list_mode]--; c->n_buffers[dirty]++; b->list_mode = dirty; @@ -1588,17 +1592,10 @@ static unsigned long dm_bufio_shrink_count(struct shrinker *shrink, struct shrink_control *sc) { struct dm_bufio_client *c; - unsigned long count; c = container_of(shrink, struct dm_bufio_client, shrinker); - if (sc->gfp_mask & __GFP_FS) - dm_bufio_lock(c); - else if (!dm_bufio_trylock(c)) - return 0; - count = c->n_buffers[LIST_CLEAN] + c->n_buffers[LIST_DIRTY]; - dm_bufio_unlock(c); - return count; + return c->n_all_buffers; } /*
As reported in my recent patch ("dm: Avoid sleeping while holding the dm_bufio lock"), we've seen in-field reports of lots of tasks sitting blocked on: mutex_lock+0x4c/0x68 dm_bufio_shrink_count+0x38/0x78 shrink_slab.part.54.constprop.65+0x100/0x464 shrink_zone+0xa8/0x198 Analysis of dm_bufio_shrink_count() shows that it's not much work to avoid grabbing the mutex there. Presumably if we can avoid grabbing this mutex then other tasks (including those handling swap) may sometimes return faster. Note that it's likely that this won't be a huge savings since we'll just need to grab the mutex if dm_bufio_shrink_scan() is called, but it still seems like it would be a sane tradeoff. This does slightly change the behavior of dm_bufio_shrink_count(). If anyone was relying on dm_bufio_shrink_count() to return the total count _after_ some in-progress operation finished then they'll no longer get that behavior. It seems unlikely anyone would be relying on this behavior, though, because: - Someone would have to be certain that the operation was already in progress _before_ dm_bufio_shrink_count() was called. If the operation wasn't already in progress then we'd get the lock first. - There aren't exactly lots of long-running operations where the dm_bufio lock is held the whole time. Most functions using the dm_bufio lock grab and drop it almost constantly. That means that getting the dm_bufio doesn't mean that any in-progress dm_bufio transactions are all done. Maybe the above argument would be obvious to someone wise in the ways of dm_bufio but it's a useful argument to make for those like me who are trying to make a small fix without full comprehension of all of dm_bufio's finer details. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- It's a bit unclear if this is actually useful and I don't have any test cases showing the benefit, but posting it in case someone else has good test cases or thinks it's a clear win. Same caveat that this development was done on Chrome OS Kernel 4.4 drivers/md/dm-bufio.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)