Bug 472040 - GFS2: [RFE] Change gfs2_quota_scan() into a shrinker
Summary: GFS2: [RFE] Change gfs2_quota_scan() into a shrinker
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Fedora
Classification: Fedora
Component: GFS-kernel
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Abhijith Das
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks: 519832
TreeView+ depends on / blocked
 
Reported: 2008-11-18 13:21 UTC by Steve Whitehouse
Modified: 2009-10-01 09:28 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Enhancement
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-03-27 10:40:14 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Patch to bring back some of lock_nolock's lvb-related functionality to make broken quotas work (3.21 KB, patch)
2008-12-18 16:50 UTC, Abhijith Das
no flags Details | Diff
gfs2_quota_data shrinker first attempt patch (8.64 KB, patch)
2008-12-18 17:23 UTC, Abhijith Das
no flags Details | Diff
removes printk from earlier lock_nolock lvb-related patch (3.07 KB, patch)
2008-12-18 23:13 UTC, Abhijith Das
no flags Details | Diff
3 changes from previous shrinker patch (9.30 KB, patch)
2008-12-18 23:23 UTC, Abhijith Das
no flags Details | Diff
one change from previous patch in gfs2_quota_cleanup to avoid deadlock (9.56 KB, patch)
2008-12-19 10:29 UTC, Abhijith Das
no flags Details | Diff
Tried to reduce double locking (13.00 KB, patch)
2009-01-07 17:47 UTC, Abhijith Das
no flags Details | Diff
Missed out a lock in the previous patch. Fixed here (13.02 KB, patch)
2009-01-07 18:47 UTC, Abhijith Das
no flags Details | Diff

Description Steve Whitehouse 2008-11-18 13:21:13 UTC
"Exactly what it says in the summary"... it looks like we should have an LRU list of freeable struct gfs2_quota_data elements, together with a shrinker which can manage the cached data.

That should allow us to remove the call to gfs2_quota_scan from gfs2_quotad and also to be more responsive to memory pressure.

This bug is just a reminder that we need to do that at some future stage.

Comment 1 Abhijith Das 2008-12-18 16:50:48 UTC
Created attachment 327338 [details]
Patch to bring back some of lock_nolock's lvb-related functionality to make broken quotas work

Comment 2 Abhijith Das 2008-12-18 17:12:53 UTC
argh, I missed out a stray printk in the previous patch.

Comment 3 Abhijith Das 2008-12-18 17:23:16 UTC
Created attachment 327342 [details]
gfs2_quota_data shrinker first attempt patch

adds reclaimable quota data to another list managed by a shrinker. This patch has not been tested yet.

Comment 4 Abhijith Das 2008-12-18 23:13:53 UTC
Created attachment 327388 [details]
removes printk from earlier lock_nolock lvb-related patch

Comment 5 Abhijith Das 2008-12-18 23:23:28 UTC
Created attachment 327389 [details]
3 changes from previous shrinker patch

1. added a line to atomic_dec(&sdp->sd_quota_count); in gfs2_shrink_qd_memory() that was missing before when we freed a qd from both the reclaim list and the fs-specific sd_quota_list.

2. changed "(atomic_read(&qd_lru_count) / 100) * sysctl_vfs_cache_pressure" to "(atomic_read(&qd_lru_count) * sysctl_vfs_cache_pressure) / 100" to get qds if qd_lru_count < 100;

3. Added SetPageUptodate(page); to stuffed_readpage() when returning a zero-filled page when index > 0; (This fixes the -EIO, I was seeing earlier; not sure if this is the right thing to do though)

Comment 6 Steve Whitehouse 2008-12-19 10:02:17 UTC
Looks qood. One question though, are we using sdp->sd_quota_spin on its own in enough places to justify keeping it? If we just used qd_lru_lock on its own, then it would be possible to reduce qd_put to something like:

if (atomic_dec_and_lock()) {
        list_add();
        atomic_inc();
        spin_unlock();
}

and it would also simplify the other places in the code where we've now got double locking too. I don't think there should be any other issues wrt stuffed_readpage as I think all the other uses would sequentially read a file, stopping at EOF so it shouldn't be an issue.

Comment 7 Abhijith Das 2008-12-19 10:29:37 UTC
Created attachment 327425 [details]
one change from previous patch in gfs2_quota_cleanup to avoid deadlock

Comment 8 Abhijith Das 2009-01-07 17:47:58 UTC
Created attachment 328402 [details]
Tried to reduce double locking

This patch uses the lru_lock for both quota lists and sd_quota_spin for internal quota fields, bitmap and such. It reduces a bit of double locking.

Comment 9 Abhijith Das 2009-01-07 18:47:12 UTC
Created attachment 328404 [details]
Missed out a lock in the previous patch. Fixed here

Comment 10 Steve Whitehouse 2009-01-14 14:00:41 UTC
This is now upstream in the -nmw git tree.

Comment 11 Steve Whitehouse 2009-03-27 10:40:14 UTC
In 2.6.30


Note You need to log in before you can comment on or make changes to this bug.