On 05/20/2013 05:46 PM, Glauber Costa wrote:
> On 05/20/2013 12:25 PM, Steven Whitehouse wrote:
>> Hi,
>>
>> On Mon, 2013-05-20 at 00:07 +0400, Glauber Costa wrote:
>>> From: Dave Chinner <dchinner@xxxxxxxxxx>
>>>
>>> Convert the filesystem shrinkers to use the new API, and standardise
>>> some of the behaviours of the shrinkers at the same time. For
>>> example, nr_to_scan means the number of objects to scan, not the
>>> number of objects to free.
>>>
>>> I refactored the CIFS idmap shrinker a little - it really needs to
>>> be broken up into a shrinker per tree and keep an item count with
>>> the tree root so that we don't need to walk the tree every time the
>>> shrinker needs to count the number of objects in the tree (i.e.
>>> all the time under memory pressure).
>>>
>>> [ glommer: fixes for ext4, ubifs, nfs, cifs and glock. Fixes are
>>> needed mainly due to new code merged in the tree ]
>>> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
>>> Signed-off-by: Glauber Costa <glommer@xxxxxxxxxx>
>>> Acked-by: Mel Gorman <mgorman@xxxxxxx>
>>> Acked-by: Artem Bityutskiy <artem.bityutskiy@xxxxxxxxxxxxxxx>
>>> Acked-by: Jan Kara <jack@xxxxxxx>
>>> CC: Steven Whitehouse <swhiteho@xxxxxxxxxx>
>>> CC: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>>> ---
>>> fs/ext4/extents_status.c | 30 ++++++++++++++++------------
>>> fs/gfs2/glock.c | 28 +++++++++++++++-----------
>>> fs/gfs2/main.c | 3 ++-
>>> fs/gfs2/quota.c | 12 +++++++-----
>>> fs/gfs2/quota.h | 4 +++-
>>> fs/mbcache.c | 51 ++++++++++++++++++++++++++++--------------------
>>> fs/nfs/dir.c | 18 ++++++++++++++---
>>> fs/nfs/internal.h | 4 +++-
>>> fs/nfs/super.c | 3 ++-
>>> fs/nfsd/nfscache.c | 31 ++++++++++++++++++++---------
>>> fs/quota/dquot.c | 34 +++++++++++++++-----------------
>>> fs/ubifs/shrinker.c | 20 +++++++++++--------
>>> fs/ubifs/super.c | 3 ++-
>>> fs/ubifs/ubifs.h | 3 ++-
>>> 14 files changed, 151 insertions(+), 93 deletions(-)
>> [snip]
>>> return 0;
>>> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
>>> index 3bd2748..4ddbccb 100644
>>> --- a/fs/gfs2/glock.c
>>> +++ b/fs/gfs2/glock.c
>>> @@ -1428,21 +1428,22 @@ __acquires(&lru_lock)
>>> * gfs2_dispose_glock_lru() above.
>>> */
>>>
>>> -static void gfs2_scan_glock_lru(int nr)
>>> +static long gfs2_scan_glock_lru(int nr)
>>> {
>>> struct gfs2_glock *gl;
>>> LIST_HEAD(skipped);
>>> LIST_HEAD(dispose);
>>> + long freed = 0;
>>>
>>> spin_lock(&lru_lock);
>>> - while(nr && !list_empty(&lru_list)) {
>>> + while ((nr-- >= 0) && !list_empty(&lru_list)) {
>>> gl = list_entry(lru_list.next, struct gfs2_glock, gl_lru);
>>>
>>> /* Test for being demotable */
>>> if (!test_and_set_bit(GLF_LOCK, &gl->gl_flags)) {
>>> list_move(&gl->gl_lru, &dispose);
>>> atomic_dec(&lru_count);
>>> - nr--;
>>> + freed++;
>>> continue;
>>> }
>>>
>>
>> This seems to change behaviour so that nr is no longer the number of
>> items to be demoted, but instead the max number of items to scan in
>> order to look for items to be demoted. Does that mean that nr has
>> changed its meaning now?
>>
>> Steve.
>>
> No, this should be the max number to be demoted, no change.
> This test above should then be freed < nr.
>
> I will update, thanks for spotting.
>
Dave,
I am auditing the other conversions now for patterns like this.
In the ashmem driver, you wrote:
- * 'nr_to_scan' is the number of objects (pages) to prune, or 0 to
query how
- * many objects (pages) we have in total.
+ * 'nr_to_scan' is the number of objects to scan for freeing.
Can you please clarify what is your intention here? For me, nr_to_scan
is still the amount we should try to free - and it has been this way for
a while - even if we have to *scan* more objects than this, because
some of them cannot be freed.
In the shrinkers you have been converting, I actually found both kinds
of behaviors: In some of them you test for freed >= nr_to_scan, and in
others, --nr_to_scan > 0
My assumption is that scanning the objects is cheap comparing to the
other cache operations we do, filling or unfilling, so whoever could,
should incur the extra couple of scans to make sure that we free as many
objects as requested *if we can*.
I am changing all shrinkers now to behave consistently like this, i.e.
bailing out in freed > nr_to_scan instead of nr_to_scan == 0 (for most
of them is thankfully obvious, those two things being the same).
If you for any reason wanted nr_to_scan to mean # of objects *scanned*,
not freed, IOW, if this is not a mistake, please say so and justify.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
