David Chinner wrote:
On Wed, Nov 28, 2007 at 11:43:26AM +1100, Lachlan McIlroy wrote:
David Chinner wrote:
On Tue, Nov 27, 2007 at 02:30:25PM +1100, Lachlan McIlroy wrote:
David Chinner wrote:
Sorry, i wasn't particularly clear. What I mean was that i_update_core
might disappear completely with the changes I'm making.
Basically, we have three different methods of marking the inode dirty
at the moment - on the linux inode (mark_inode_dirty[_sync]()), the
i_update_core = 1 for unlogged changes and logged changes are tracked
inode log item in the AIL.
One top of that, we have three different methods of flushing them - one
>from the generic code for inodes dirtied by mark_inode_dirty(), one from
Don't we already call mark_inode_dirty[_sync]() everywhere we dirty the
xfssyncd for inodes that are only dirtied by setting i_update_core = 1
and the other from the xfsaild when log tail pushing.
Ideally we should only have a single method for pushing out inodes. The
step to that is tracking the dirty state in a single tree (the inode
trees). That means we have to hook ->dirty_inode() to catch all dirtying
mark_inode_dirty[_sync]() and mark the inodes dirty in the radix tree.
need to use xfs_mark_inode_dirty_sync() everywhere that we dirty the
Maybe. Maybe not. Tell me - does xfs_ichgtime() do the right thing?
[ I do know the answer to this question and there's a day of kdb tracing
behind the answer. I wrote a 15 line comment to explain what was going
on in one of my patches. ]
Are you referring to the !(inode->i_state & I_LOCK) check?
I've never liked that check, can we just get rid of it?
Anyway, since you know the answer why don't you enlighten me?
When allocating a new inode, we mark the inode dirty when first
setting the timestamps in xfs_dir_ialloc(). At the time this happens
the inode is I_LOCK|I_NEW and hence mark_inode_dirty_sync() would just
mark the inode dirty and *not* move it to the dirty list.
Because unlock_new_inode() does not check the dirty state when
removing the I_LOCK state, the inode is never moved to the dirty list
if it is already dirty (unlike __sync_single_inode()).
Further calls to mark_inode_dirty_sync() see the inode as dirty and
don't move it to the dirty list, either. Hence the inode would never
get flushed out by the generic code if we called
mark_inode_dirty_sync() in that location.
Why is it wrong? It should be checking I_NEW, not I_LOCK because all
other cases where I_LOCK might be set are covered by the code that
unlocks the inode.
Once we have all the dirty state in the radix trees we can now get rid of
i_update_core and i_update_size - all they do is mark the inode dirty and
we don't really care about the difference between them(*) - and just use
the dirty bit in the radix tree when necessary.
If we want to check if an inode is dirty do we have to look up the dirty
bit in the tree or is there some easy way to get it from the inode?
xfs_inode_clean(ip) is my preferred interface. How that is finally
implemented will be determined by how this all cleans up and what
performs the best. If lockless tree lookups don't cause performance
problems, then there is little reason to keep redundant information
I can't imagine that a tree lookup (lockless or not) would be faster
than dereferencing fields from the inode. If keeping the inode's dirty
flags and the ones in the radix tree in sync is an issue then maybe
tree lookups are a performance hit we can live with.
I'm hoping to avoid this problem altogether by removing as many
"is the inode dirty" checks as possible. If inode writeback is
driven exclusively by the radix tree dirty bit via a traversal
and we only write back logged changes, then I don't think we need
to be checking if the inode is clean very often.
That is, if we see the inode in xfs_flush_inode() then it is
dirty at the linux level, so we log the inode. That makes the
inode clean at the linux layer and dirty at the XFS level, and
we know that as long as the inode remains in the AIL it is dirty.
We only ever flush inodes based on a AIL push (which doesn't
require dirty bits) or via the syncd, which looks up dirty
inodes via the radix tree tag, and hence most of the dirty
checks on the inode can go away because we don't need to
check it during writeback now.
By consolidating the different ways of dirtying an inode we lose the
to know why it is dirty and what action needs to be done to undirty it.
The only way to undirty an inode is to write it to disk.
True. I was thinking about what may need to be done before we write it
to disk such as flushing the log but that would just be dependent on
whether the inode is pinned?
Right, flushing the log is only needed if it is pinned.
Could not be implemented? What was that patch I sent around then?
For example if the inode log item has bits set then we know we have to
the log otherwise there is no need. With a general purpose dirty bit we
No, if the log item is present and dirty (i.e. inode is in the AIL),
all it means is that we need to attach a callback to the buffer
(xfs_iflush_done) when dispatching the I/O to do processing of the
log item on I/O completion. Whether i_update_core is set or not
in this case is irrelevant - the log item state overrides that.
have to flush regardless. And my recent attempt to fix the log replay
relies on i_update_core to indicate there are unlogged changes - I don't
how that will work with these changes.
But your changes could not be implemented, either. You can't log the inode
to clean it - it merely transfers the writeback from one list to
Sorry, I missed an important work there - could not be implemented
Basically, you are logging the inode, then call xfs_iflush, which
immediately sees it pinned and forces the log. That's an extra
transaction *and* log I/O for every inode we write. That defeats all
inode clustering and and will seriously harm performance.
I didn't see another way around it. We only need to force the log for
pinned inodes if it is a sync writeback, otherwise we can just try again
Also, the change fails to log changes to inodes in the same
cluster that get written out because they are dirty.
That's where it all sort of falls apart. I didn't want to log the inode
in xfs_iflush_int() because we have the flush lock held and I was pretty
sure logging a transaction with the flush lock held would be a bad idea.
That's why I specifically removed the code that resets i_update_core in
xfs_iflush_int() - so that other inodes in the same cluster will still be
flagged as having unlogged changes even after the inodes have been synced
to disk. But as I said it was an idea that needed some polishing.
So, the cleaner fix is to do this - change the xfs_inode_flush()
just to unconditionally log the inode and don't do inode writeback *at
all* from there. That will catch all cases of unlogged changes and leave
inode writeback to tail-pushing or xfssyncd which can be driven by
the radix tree.
Huh? Aren't we trying to minimize the number of transactions we do? My
changes introduce new transactions but only when we have to. You're saying
here that we log the inode unconditionally - how is that better? I'm not
trying to defend my changes here (I don't care how the problem gets fixed)
- I'm just trying to understand why your suggestions are a good idea.
Because we can log entire inode cluster's worth of changes in a single
transaction. One transaction vs one I/O - it's a decent tradeoff to
avoid this problem, esp. as we'll get improved inode writeback clustering
if we flush from the radix tree (i.e. clusters get flushed in ascending
inode number order).....
That should help a lot but it will use even more space in the log - quite a
lot more if just one inode in the cluster needs to be logged. Do you plan
to do this in the write_inode path? If so we'll have inodes that have been
logged (with a previous cluster) that still have I_DIRTY set. When these
inodes go through the write_inode path we'll need to skip the transaction.
I do like the way it simplifies inode writeback though - a sync would
optionally log all the inodes and then just flush the log and that's it
Yup, pretty much.