On Mon, Mar 15, 2010 at 12:08:01PM -0400, Christoph Hellwig wrote:
> Some comments on the patches:
> - "xfs: cluster fsync transaction" seems like something that can stand
> on it's own and go into the tree now. Despite the comments it
> currently only clusters in fsync and not write_inode which might
> lead to higher benefits, btw.
As I said, I'll have a bit more of a think about this one...
> - the busy extent tracking might be worth to be reordered before the
> delayed logging series. In itself it might also want some reordering
> as there's a lot churn in the patches there. Making
> "XFS: Simplify transaction busy extent tracking" first in that
> subseries might help quite a bit to reduce that churn
Agreed. They were ordered this way because I only added the extent
tracking after doing the delayed logging and tracking down one of
the sources of log forces that was limiting performance was
overflowing the per-ag busy extent array. I'll rework it....
> - The actual CIL implementation seems to be split into too small
> patches IMHO. E.g. "xfs: extend the log item to support delayed
> logging" and "xfs: Introduce the Committed Item List" are two
> sides of the same coin and splitting it might not make too much
True, it might be a bit fine grained. Really, what I wanted to do is
split the changes that affected the non-delayed logging from those
that are only executed when delayed logging is active. That way I
could confirm that the non-delayed path was still operating
correctly before adding all the delayed logging code that used
it. I can combine them together again if you want.