Bug 472040 - GFS2: [RFE] Change gfs2_quota_scan() into a shrinker
GFS2: [RFE] Change gfs2_quota_scan() into a shrinker
Status: CLOSED UPSTREAM
Product: Fedora
Classification: Fedora
Component: GFS-kernel (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Abhijith Das
: FutureFeature
Depends On:
Blocks: 519832
  Show dependency treegraph
 
Reported: 2008-11-18 08:21 EST by Steve Whitehouse
Modified: 2009-10-01 05:28 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Enhancement
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-27 06:40:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


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 11:50 EST, Abhijith Das
no flags Details | Diff
gfs2_quota_data shrinker first attempt patch (8.64 KB, patch)
2008-12-18 12:23 EST, Abhijith Das
no flags Details | Diff
removes printk from earlier lock_nolock lvb-related patch (3.07 KB, patch)
2008-12-18 18:13 EST, Abhijith Das
no flags Details | Diff
3 changes from previous shrinker patch (9.30 KB, patch)
2008-12-18 18:23 EST, 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 05:29 EST, Abhijith Das
no flags Details | Diff
Tried to reduce double locking (13.00 KB, patch)
2009-01-07 12:47 EST, Abhijith Das
no flags Details | Diff
Missed out a lock in the previous patch. Fixed here (13.02 KB, patch)
2009-01-07 13:47 EST, Abhijith Das
no flags Details | Diff

  None (edit)
Description Steve Whitehouse 2008-11-18 08:21:13 EST
"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 11:50:48 EST
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 12:12:53 EST
argh, I missed out a stray printk in the previous patch.
Comment 3 Abhijith Das 2008-12-18 12:23:16 EST
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 18:13:53 EST
Created attachment 327388 [details]
removes printk from earlier lock_nolock lvb-related patch
Comment 5 Abhijith Das 2008-12-18 18:23:28 EST
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 05:02:17 EST
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 05:29:37 EST
Created attachment 327425 [details]
one change from previous patch in gfs2_quota_cleanup to avoid deadlock
Comment 8 Abhijith Das 2009-01-07 12:47:58 EST
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 13:47:12 EST
Created attachment 328404 [details]
Missed out a lock in the previous patch. Fixed here
Comment 10 Steve Whitehouse 2009-01-14 09:00:41 EST
This is now upstream in the -nmw git tree.
Comment 11 Steve Whitehouse 2009-03-27 06:40:14 EDT
In 2.6.30

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