* RFC: merging the quota source files @ 2011-08-30 12:11 Christoph Hellwig 2011-08-31 0:22 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2011-08-30 12:11 UTC (permalink / raw) To: xfs Hi, I've started a bit work on the quota code, and the split between the implementation files is rather annoying. I'd suggest we merge at least xfs_dquot.c and xfs_qm.c into a single file, as there is no visible split. Given how small and useless xfs_qm_bhv.c is I'd throw it in. As a second pass I'd also like to kill xfs_qm_syscalls.c and merge it into the new xfs_qm.c/xfs_dquot.c for the low-level helpers, and xfs_quotaops.c for high-level code that deals directly with the Linux interface. Does this a) sound good to others, and b) does anyone have a preference for the name of the new quota implementation file? _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: RFC: merging the quota source files 2011-08-30 12:11 RFC: merging the quota source files Christoph Hellwig @ 2011-08-31 0:22 ` Dave Chinner 2011-08-31 5:20 ` Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2011-08-31 0:22 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Tue, Aug 30, 2011 at 08:11:12AM -0400, Christoph Hellwig wrote: > Hi, > > I've started a bit work on the quota code, and the split between the > implementation files is rather annoying. I'd suggest we merge at least > xfs_dquot.c and xfs_qm.c into a single file, as there is no visible > split. It seems to me to be a reasonable separation - xfs_qm.c is mostly global management functions e.g. quota cache initialisation, reclaim, quotacheck, etc, which xfs_dquot.c is all single dquot specific operations which don't have global scope. I'd argue that there are parts of xfs_qm.c that could be moved into xfs_dquot.c (like the xfs_qm_vop_* and xfs_qm_dq*attach* functions that only act on a specific dquots) which would make this separation much clearer. This would also reflect the same sort of separation as we have with inode operations vs cache infrastructure.... > Given how small and useless xfs_qm_bhv.c is I'd throw it in. Agreed, that should be merged into xfs_qm.c at least. > As a second pass I'd also like to kill xfs_qm_syscalls.c and merge > it into the new xfs_qm.c/xfs_dquot.c for the low-level helpers, and > xfs_quotaops.c for high-level code that deals directly with the Linux > interface. That seems like a good idea, too. > Does this a) sound good to others, and b) does anyone have a preference > for the name of the new quota implementation file? If you are going to rename xfs_qm.c, then xfs_quota.c seems like the best name to me. FWIW, this is what I'd like to seen done with the quota infrastructure in the medium term: 1. break up the global quota manager into per-xfs_mount structures (move into the quotainfo structure). 2. remove the lookup hash tables and replace it with something more flexible (btree/radix-tree/rbtree) for different cache sizes. 3. replace the weird, complex freelist and dquot recycling code with a standard LRU implementation. 4. clearly document the locking model and simplify it to remove all the unnecessary nested locking and try-locking that the current deeply nested model requires. This is appears to just be a replication of the locking model pattern already defined by the VFS dentry and inode caches... 4. change the shrinker implementation to be "normal" so that the dquot cache sizes respond to memory pressure correctly The gets rid of roughly ~50% of the infrastructure code that currently exists in xfs_qm.c, and remaining stuff like the sync infrastructure mostly goes away with AIL driven writeback, too. Some of the functionality remaining (like the shrinkers) can be placed in xfs_sync.c with the inode cache shrinkers, which further reduces the code in xfs_qm.c. At that point the delineation between xfs_qm.c and xfs_dquot.c will be much clearer, I think, especially if the changes I suggested above were made... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: RFC: merging the quota source files 2011-08-31 0:22 ` Dave Chinner @ 2011-08-31 5:20 ` Christoph Hellwig 2011-08-31 12:17 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2011-08-31 5:20 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs On Wed, Aug 31, 2011 at 10:22:36AM +1000, Dave Chinner wrote: > > I've started a bit work on the quota code, and the split between the > > implementation files is rather annoying. I'd suggest we merge at least > > xfs_dquot.c and xfs_qm.c into a single file, as there is no visible > > split. > > It seems to me to be a reasonable separation - xfs_qm.c is mostly > global management functions e.g. quota cache initialisation, > reclaim, quotacheck, etc, which xfs_dquot.c is all single dquot > specific operations which don't have global scope. > > I'd argue that there are parts of xfs_qm.c that could be moved into > xfs_dquot.c (like the xfs_qm_vop_* and xfs_qm_dq*attach* functions > that only act on a specific dquots) which would make this separation > much clearer. This would also reflect the same sort of separation as > we have with inode operations vs cache infrastructure.... That and things like quota reclaim which tends to operate on the single dquot, but mostly sit in xfs_qm.c. > > Does this a) sound good to others, and b) does anyone have a preference > > for the name of the new quota implementation file? > > If you are going to rename xfs_qm.c, then xfs_quota.c seems like the > best name to me. Ok. > FWIW, this is what I'd like to seen done with the quota > infrastructure in the medium term: > > > 4. clearly document the locking model and simplify it to > remove all the unnecessary nested locking and try-locking > that the current deeply nested model requires. This is > appears to just be a replication of the locking model pattern > already defined by the VFS dentry and inode caches... This is the queue I plan to submit for Linux 3.2 if we get any progress on getting the current pending queue reviewed and applied. > > 2. remove the lookup hash tables and replace it with > something more flexible (btree/radix-tree/rbtree) for > different cache sizes. I have a prototype to use a radix tree. You said before you didn't like it as the radix tree assumeds the quota ids are clustered, but so are the actual dqouts in the quota file, so I do not think it is a problem. We'll probably need testing at a big quota user site to decided this. > > 4. change the shrinker implementation to be "normal" so that > the dquot cache sizes respond to memory pressure correctly I'm waiting for your shrinker interface changes to start with as there will be too many conflicts otherwise. > 3. replace the weird, complex freelist and dquot recycling > code with a standard LRU implementation. I have ptrototypes / WIP patches for this, but it doesn't make sense before item 3 is done. > The gets rid of roughly ~50% of the infrastructure code that > 1. break up the global quota manager into per-xfs_mount > structures (move into the quotainfo structure). Moving to the radix tree remove the hashtable and some accounting from the global quota manager. After that we only have the freelist and assorted accounting and the slab caches left. The freelist can fairly easily be changed to be per-mount at this point, but the slab caches should remain global. We probably could just create them unconditionally if quota support is compiled in and kill the global quota manager at this point. > currently exists in xfs_qm.c, and remaining stuff like the sync > infrastructure mostly goes away with AIL driven writeback, too. Most of the sync infrastructure is already gone in my series, we only do one non-trylocking but delwri pass after quotacheck, after that all quota updates are logged. > Some > of the functionality remaining (like the shrinkers) can be placed in > xfs_sync.c with the inode cache shrinkers, which further reduces the > code in xfs_qm.c. > > At that point the delineation between xfs_qm.c and xfs_dquot.c will > be much clearer, I think, especially if the changes I suggested > above were made... So that's one vote for keeping them separate. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: RFC: merging the quota source files 2011-08-31 5:20 ` Christoph Hellwig @ 2011-08-31 12:17 ` Dave Chinner 2011-08-31 12:37 ` Christoph Hellwig 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2011-08-31 12:17 UTC (permalink / raw) To: Christoph Hellwig; +Cc: xfs On Wed, Aug 31, 2011 at 01:20:49AM -0400, Christoph Hellwig wrote: > On Wed, Aug 31, 2011 at 10:22:36AM +1000, Dave Chinner wrote: > > FWIW, this is what I'd like to seen done with the quota > > infrastructure in the medium term: > > > > > > 4. clearly document the locking model and simplify it to > > remove all the unnecessary nested locking and try-locking > > that the current deeply nested model requires. This is > > appears to just be a replication of the locking model pattern > > already defined by the VFS dentry and inode caches... > > This is the queue I plan to submit for Linux 3.2 if we get any progress > on getting the current pending queue reviewed and applied. I'm slowly working my way through the queue. I've got plenty of stuff on my plate at the moment, though, and I'll be on holidays for a couple of weeks starting next week, so don't rely on me to get stuff reviewed in the near term.... > > 2. remove the lookup hash tables and replace it with > > something more flexible (btree/radix-tree/rbtree) for > > different cache sizes. > > I have a prototype to use a radix tree. You said before you didn't like > it as the radix tree assumeds the quota ids are clustered, but so are > the actual dqouts in the quota file, so I do not think it is a problem. > We'll probably need testing at a big quota user site to decided this. I'd just use the generic btree code in lib/btree.c. That way we simply don't have to care what quota ids are in use and how sparse they are.... > > 4. change the shrinker implementation to be "normal" so that > > the dquot cache sizes respond to memory pressure correctly > > I'm waiting for your shrinker interface changes to start with as there > will be too many conflicts otherwise. Yeah, that's what I was planning on doing > > 3. replace the weird, complex freelist and dquot recycling > > code with a standard LRU implementation. > > I have ptrototypes / WIP patches for this, but it doesn't make sense > before item 3 is done. Similarly I was planning ondoingthis to use the generic LRU code I've proposed... > > The gets rid of roughly ~50% of the infrastructure code that > > > 1. break up the global quota manager into per-xfs_mount > > structures (move into the quotainfo structure). > > Moving to the radix tree remove the hashtable and some accounting > from the global quota manager. After that we only have the freelist > and assorted accounting and the slab caches left. The freelist can > fairly easily be changed to be per-mount at this point, but the slab > caches should remain global. We probably could just create them > unconditionally if quota support is compiled in and kill the global > quota manager at this point. *nod* > > currently exists in xfs_qm.c, and remaining stuff like the sync > > infrastructure mostly goes away with AIL driven writeback, too. > > Most of the sync infrastructure is already gone in my series, we only > do one non-trylocking but delwri pass after quotacheck, after that > all quota updates are logged. > > > Some > > of the functionality remaining (like the shrinkers) can be placed in > > xfs_sync.c with the inode cache shrinkers, which further reduces the > > code in xfs_qm.c. > > > > At that point the delineation between xfs_qm.c and xfs_dquot.c will > > be much clearer, I think, especially if the changes I suggested > > above were made... > > So that's one vote for keeping them separate. Yeah. the dquot design pattern is based on the pattern that inodes use, so having the code structured in a similar manner makes sense to me. Speaking of which (and going well and truly OT), I don't see much reason for keeping xfs_iget.c separate from xfs_inode.c anymore - there isn't really much code left in xfs_iget.c now, and stuff like all the locking code isn't really related to the inode lookup function of xfs_iget(), anyway... Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: RFC: merging the quota source files 2011-08-31 12:17 ` Dave Chinner @ 2011-08-31 12:37 ` Christoph Hellwig 0 siblings, 0 replies; 5+ messages in thread From: Christoph Hellwig @ 2011-08-31 12:37 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, xfs On Wed, Aug 31, 2011 at 10:17:46PM +1000, Dave Chinner wrote: > > This is the queue I plan to submit for Linux 3.2 if we get any progress > > on getting the current pending queue reviewed and applied. > > I'm slowly working my way through the queue. I've got plenty of > stuff on my plate at the moment, though, and I'll be on holidays > for a couple of weeks starting next week, so don't rely on me to get > stuff reviewed in the near term.... Ok. It would be good to get the bmapi refactor off the plate - the series is huge and has chances to get a major pain to rebase. Been through it once from your original series, and I don't want to do it again. I don't think there is much else that you haven't reviewed yet. > > I have a prototype to use a radix tree. You said before you didn't like > > it as the radix tree assumeds the quota ids are clustered, but so are > > the actual dqouts in the quota file, so I do not think it is a problem. > > We'll probably need testing at a big quota user site to decided this. > > I'd just use the generic btree code in lib/btree.c. That way we > simply don't have to care what quota ids are in use and how sparse > they are.... I do not trust code that is used by a single fringe filesystem as much code used all over the core VM/VFS. Nevermind that the radix tree has a lot of features that do come in handy for quotas. I've also done a survey of large users systems I have access to and all of them absolutely do cluster uids. Less so for GIDs, but none of them has a lot of those. > > I have ptrototypes / WIP patches for this, but it doesn't make sense > > before item 3 is done. > > Similarly I was planning ondoingthis to use the generic LRU code > I've proposed... That is the idea. We'll need a few iterations before getting there, though. > > > > So that's one vote for keeping them separate. > > Yeah. the dquot design pattern is based on the pattern that inodes > use, so having the code structured in a similar manner makes sense > to me. I don't really see a similar split for the inode. What would be the equivalent to xfs_qm.c by your previous split? Btw, if we look at xfs_qm.c in current minaline, lines: 0 - 63: boilerplate 64 - 272: handling of xfs_Gqm (will go away) 273 - 410: mount / unmount handling 411 - 604: dquot flushing / reclaim 605 - 879: dquot attach / detach (belongs into xfs_dquot.c?) 880 - 973: xfs_qm_sync (will go away) 974 - 1540: mount / umount handling 1541 - 1793: quotacheck / quota inode allocation 1794 - 2043: quota reclaim / reuse 2044 - 2072: sb write helper 2073 - 2416: vop helpers So less than half of the code is thing that do belong into xfs_qm.c by your previous defintion. I think we'd be simpler off with a merge. > Speaking of which (and going well and truly OT), I don't see much > reason for keeping xfs_iget.c separate from xfs_inode.c anymore - > there isn't really much code left in xfs_iget.c now, and stuff like > all the locking code isn't really related to the inode lookup > function of xfs_iget(), anyway... No real reason. If we want to resplit these I'd split the about 2/3 of xfs_inode.c that are shared with userspace into a separate file, though. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-08-31 12:37 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-08-30 12:11 RFC: merging the quota source files Christoph Hellwig 2011-08-31 0:22 ` Dave Chinner 2011-08-31 5:20 ` Christoph Hellwig 2011-08-31 12:17 ` Dave Chinner 2011-08-31 12:37 ` Christoph Hellwig
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.