RFC: merging the quota source files

RFC: merging the quota source files

[Christoph Hellwig]

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

Re: RFC: merging the quota source files

[Dave Chinner]

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

Re: RFC: merging the quota source files

[Christoph Hellwig]

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

Re: RFC: merging the quota source files

[Dave Chinner]

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

Re: RFC: merging the quota source files

[Christoph Hellwig]

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