Delayed logging of file sizes

The easy solution is to log everything so that log replay doesn't need to check if the on-disk version is newer - it can just replay the log. But logging everything would cause too much log traffic so this patch is a compromise and it logs a transaction before we flush an inode to disk only if it has changes that have not yet been logged.

Quoted 5 lines · Lachlan McIlroy

The easy solution is to log everything so that log replay doesn't need to check if the on-disk version is newer - it can just replay the log. But logging everything would cause too much log traffic so this patch is a compromise and it logs a transaction before we flush an inode to disk only if it has changes that have not yet been logged.

The problem with this is that the inode will be marked dirty during the transaction, so we'll never be able to clean an inode if we issue a transaction during inode writeback.

Quoted 7 lines · David Chinner

Quoted 5 lines · Lachlan McIlroy

The easy solution is to log everything so that log replay doesn't need to check if the on-disk version is newer - it can just replay the log. But logging everything would cause too much log traffic so this patch is a compromise and it logs a transaction before we flush an inode to disk only if it has changes that have not yet been logged.

Quoted 3 lines

The problem with this is that the inode will be marked dirty during the transaction, so we'll never be able to clean an inode if we issue a transaction during inode writeback.

Ah, yeah, good point. I wrote this patch back before that "dirty inode on transaction" patch went in.

For this transaction though the changes
to the inode have already been made (ie when we set i_update_core and called mark_inode_dirty_sync()) so there is no need to dirty it in this transaction. I'll keep digging. Thanks.

Quoted 7 lines · Lachlan McIlroy

Quoted 6 lines · David Chinner

Quoted 5 lines · Lachlan McIlroy

The easy solution is to log everything so that log replay doesn't need to check if the on-disk version is newer - it can just replay the log. But logging everything would cause too much log traffic so this patch is a compromise and it logs a transaction before we flush an inode to disk only if it has changes that have not yet been logged.

Quoted 5 lines

Quoted 3 lines

The problem with this is that the inode will be marked dirty during the transaction, so we'll never be able to clean an inode if we issue a transaction during inode writeback.

Ah, yeah, good point. I wrote this patch back before that "dirty inode on transaction" patch went in.

Wouldn't have made aany difference - the inode woul dbe marked dirty at transaction completion...

Quoted 4 lines

For this transaction though the changes
to the inode have already been made (ie when we set i_update_core and called mark_inode_dirty_sync()) so there is no need to dirty it in this transaction. I'll keep digging. Thanks.

I wouldn't worry too much about this problem right now - I'm working on moving the dirty state into the inode radix trees so i_update_core might even go away completely soon....

Quoted 22 lines · David Chinner

Quoted 7 lines · Lachlan McIlroy

Quoted 6 lines · David Chinner

Quoted 5 lines · Lachlan McIlroy

The easy solution is to log everything so that log replay doesn't need to check if the on-disk version is newer - it can just replay the log. But logging everything would cause too much log traffic so this patch is a compromise and it logs a transaction before we flush an inode to disk only if it has changes that have not yet been logged.

Quoted 5 lines

Quoted 3 lines

The problem with this is that the inode will be marked dirty during the transaction, so we'll never be able to clean an inode if we issue a transaction during inode writeback.

Ah, yeah, good point. I wrote this patch back before that "dirty inode on transaction" patch went in.

Wouldn't have made aany difference - the inode woul dbe marked dirty at transaction completion...

Quoted 4 lines

For this transaction though the changes
to the inode have already been made (ie when we set i_update_core and called mark_inode_dirty_sync()) so there is no need to dirty it in this transaction. I'll keep digging. Thanks.

Quoted 4 lines

I wouldn't worry too much about this problem right now - I'm working on moving the dirty state into the inode radix trees so i_update_core might even go away completely soon....

Which problem? Just the bit about dirtying the inode or will your changes allow us to log all inode changes?

What's the motivation for moving the dirty state?

Quoted 21 lines · Lachlan McIlroy

Quoted 20 lines · David Chinner

Quoted 7 lines · Lachlan McIlroy

Quoted 6 lines · David Chinner

Quoted 5 lines · Lachlan McIlroy

The easy solution is to log everything so that log replay doesn't need to check if the on-disk version is newer - it can just replay the log. But logging everything would cause too much log traffic so this patch is a compromise and it logs a transaction before we flush an inode to disk only if it has changes that have not yet been logged.

Quoted 5 lines

Quoted 3 lines

The problem with this is that the inode will be marked dirty during the transaction, so we'll never be able to clean an inode if we issue a transaction during inode writeback.

Ah, yeah, good point. I wrote this patch back before that "dirty inode on transaction" patch went in.

Wouldn't have made aany difference - the inode woul dbe marked dirty at transaction completion...

Quoted 4 lines

For this transaction though the changes
to the inode have already been made (ie when we set i_update_core and called mark_inode_dirty_sync()) so there is no need to dirty it in this transaction. I'll keep digging. Thanks.

Quoted 6 lines

Quoted 4 lines

I wouldn't worry too much about this problem right now - I'm working on moving the dirty state into the inode radix trees so i_update_core might even go away completely soon....

Which problem? Just the bit about dirtying the inode or will your changes allow us to log all inode changes?

Trying to change XFS to logging all updates.

Quoted 2 lines

What's the motivation for moving the dirty state?

Better inode writeback clustering. i.e. it's easy to find all the dirty inodes and then we can write them in larger contiguous chunks. The first "hack" at this I did tracked only inodes in the AIL. Sequential create of small files improved by about 20% with better clustering during tail pushing operations. I'm trying to make it track all dirty inodes at this point (via ->dirty_inode). This may mean that i_update_core is not needed to track whether an inode needs writeback or not.

Not to mention all that horrible IPOINTER crap can get removed from xfs_sync_inodes() because finding dirty inodes is now a lockless radix tree traverse based on a dirty tag lookup.

That also means the global mount inodes list can be replaced by a lockless radix tree traverse, so we can lose another 2 pointers in the xfs_inode_t and lock operations out of the inode get and reclaim paths.

Quoted 26 lines · David Chinner

Quoted 25 lines · Lachlan McIlroy

Quoted 24 lines · David Chinner

Quoted 7 lines · Lachlan McIlroy

Quoted 6 lines · David Chinner

Quoted 5 lines · Lachlan McIlroy

The easy solution is to log everything so that log replay doesn't need to check if the on-disk version is newer - it can just replay the log. But logging everything would cause too much log traffic so this patch is a compromise and it logs a transaction before we flush an inode to disk only if it has changes that have not yet been logged.

Quoted 5 lines

Quoted 3 lines

The problem with this is that the inode will be marked dirty during the transaction, so we'll never be able to clean an inode if we issue a transaction during inode writeback.

Ah, yeah, good point. I wrote this patch back before that "dirty inode on transaction" patch went in.

Wouldn't have made aany difference - the inode woul dbe marked dirty at transaction completion...

Quoted 4 lines

For this transaction though the changes
to the inode have already been made (ie when we set i_update_core and called mark_inode_dirty_sync()) so there is no need to dirty it in this transaction. I'll keep digging. Thanks.

I wouldn't worry too much about this problem right now - I'm working on moving the dirty state into the inode radix trees so i_update_core might even go away completely soon....

Quoted 4 lines

Quoted 2 lines

Which problem? Just the bit about dirtying the inode or will your changes allow us to log all inode changes?

Trying to change XFS to logging all updates.

That would be great. But what about the increase in log traffic that has deterred us from doing this in the past?

Quoted 27 lines · Lachlan McIlroy

Quoted 26 lines · David Chinner

Quoted 25 lines · Lachlan McIlroy

Quoted 24 lines · David Chinner

Quoted 7 lines · Lachlan McIlroy

Quoted 6 lines · David Chinner

Quoted 5 lines · Lachlan McIlroy

The easy solution is to log everything so that log replay doesn't need to check if the on-disk version is newer - it can just replay the log. But logging everything would cause too much log traffic so this patch is a compromise and it logs a transaction before we flush an inode to disk only if it has changes that have not yet been logged.

Quoted 5 lines

Quoted 3 lines

The problem with this is that the inode will be marked dirty during the transaction, so we'll never be able to clean an inode if we issue a transaction during inode writeback.

Ah, yeah, good point. I wrote this patch back before that "dirty inode on transaction" patch went in.

Wouldn't have made aany difference - the inode woul dbe marked dirty at transaction completion...

Quoted 4 lines

For this transaction though the changes
to the inode have already been made (ie when we set i_update_core and called mark_inode_dirty_sync()) so there is no need to dirty it in this transaction. I'll keep digging. Thanks.

I wouldn't worry too much about this problem right now - I'm working on moving the dirty state into the inode radix trees so i_update_core might even go away completely soon....

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

Which problem? Just the bit about dirtying the inode or will your changes allow us to log all inode changes?

Trying to change XFS to logging all updates.

That would be great. But what about the increase in log traffic that has deterred us from doing this in the past?

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 via the 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 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 first step to that is tracking the dirty state in a single tree (the inode radix trees). That means we have to hook ->dirty_inode() to catch all dirtying via mark_inode_dirty[_sync]() and mark the inodes dirty in the radix tree. Then we need to use xfs_mark_inode_dirty_sync() everywhere that we dirty 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.

To flush the dirty inodes we just do radix_tree_gang_lookup_tag_range() calls to do ascending cluster order writeback. This will replace the mount inode list walking in xfs_sync_inodes() and other places to find dirty inodes.

/me puts on flame-proof suite

I'd even like to go as far as a two pass writeback algorithm; pass one only writes data, and pass two only writes inodes. The second pass for XFS needs to be delayed until data writeback is complete because of delalloc and inode size updates redirtying the inode. The current mechanism means we often do two inode writes for the one data write...

Basically, our writeback code is a mess and I want to clean it up before we try to deal with the unlogged changes....

Cheers,

Dave.

(*) Even for FDATASYNC we should always force the log out because we may have delayed allocation transactions still sitting in iclog buffers. This, AFAICT, is a bug in the current implementation.

Quoted 14 lines · David Chinner

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 via the
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 xfssyncd for inodes that are only dirtied by setting i_update_core = 1 and the other from the xfsaild when log tail pushing.

Quoted 8 lines

Ideally we should only have a single method for pushing out inodes. The first
step to that is tracking the dirty state in a single tree (the inode radix trees). That means we have to hook ->dirty_inode() to catch all dirtying via
mark_inode_dirty[_sync]() and mark the inodes dirty in the radix tree. Then we
need to use xfs_mark_inode_dirty_sync() everywhere that we dirty the inode.

Don't we already call mark_inode_dirty[_sync]() everywhere we dirty the inode?

Quoted 4 lines

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?

By consolidating the different ways of dirtying an inode we lose the ability to know why it is dirty and what action needs to be done to undirty it.

For example if the inode log item has bits set then we know we have to flush the log otherwise there is no need. With a general purpose dirty bit we

will
have to flush regardless. And my recent attempt to fix the log replay issue relies on i_update_core to indicate there are unlogged changes - I don't see how that will work with these changes.

Quoted 5 lines

I'd even like to go as far as a two pass writeback algorithm; pass one only writes data, and pass two only writes inodes. The second pass for XFS needs to be delayed until data writeback is complete because of delalloc and inode size updates redirtying the inode. The current mechanism means we often do two inode writes for the one data write...

Quoted 19 lines · Lachlan McIlroy

Quoted 18 lines · David Chinner

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 via the 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 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 first step to that is tracking the dirty state in a single tree (the inode radix trees). That means we have to hook ->dirty_inode() to catch all dirtying via mark_inode_dirty[_sync]() and mark the inodes dirty in the radix tree. Then we need to use xfs_mark_inode_dirty_sync() everywhere that we dirty the inode.

Quoted 1 line

Don't we already call mark_inode_dirty[_sync]() everywhere we dirty the inode?

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. ]

Quoted 5 lines

Quoted 5 lines

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.

Quoted 2 lines

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 around.

Quoted 3 lines

By consolidating the different ways of dirtying an inode we lose the ability 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.

Quoted 3 lines

For example if the inode log item has bits set then we know we have to flush 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.

Quoted 4 lines

will
have to flush regardless. And my recent attempt to fix the log replay issue relies on i_update_core to indicate there are unlogged changes - I don't see 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 another.

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.

Basically, if we only ever write state to disk that we've logged, then we are home free. That means the only time we should update the unlogged fields - timestamps and inode size - is during a transaction commit and not during inode writeback. If we do that then i_update_core and i_update_size go away completely and the only place we need track inode dirty state in XFS is when the inodes are in the AIL list.

Even better: this removes one of the three places where we do inode writeback and is a significant step towards:

Quoted 5 lines

Quoted 5 lines

I'd even like to go as far as a two pass writeback algorithm; pass one only writes data, and pass two only writes inodes. The second pass for XFS needs to be delayed until data writeback is complete because of delalloc and inode size updates redirtying the inode. The current mechanism means we often do two inode writes for the one data write...

----

What I'm trying to say is that I don't think we can cleanly fix the problem with the current structure, so let's not waste time on it. A cleaner fix should just fall out a simpler writeback structure.

Quoted 29 lines · David Chinner

Quoted 27 lines · Lachlan McIlroy

Quoted 14 lines · David Chinner

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 via the
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 xfssyncd for inodes that are only dirtied by setting i_update_core = 1 and the other from the xfsaild when log tail pushing.

Quoted 10 lines

Ideally we should only have a single method for pushing out inodes. The first
step to that is tracking the dirty state in a single tree (the inode radix
trees). That means we have to hook ->dirty_inode() to catch all dirtying via
mark_inode_dirty[_sync]() and mark the inodes dirty in the radix tree. Then we
need to use xfs_mark_inode_dirty_sync() everywhere that we dirty the inode.

Don't we already call mark_inode_dirty[_sync]() everywhere we dirty the inode?

Quoted 5 lines

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?

Anyway, since you know the answer why don't you enlighten me?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 2 lines

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 around.

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.

Quoted 5 lines

Quoted 3 lines

By consolidating the different ways of dirtying an inode we lose the ability
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?

Quoted 17 lines

Quoted 3 lines

For example if the inode log item has bits set then we know we have to flush
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.

Quoted 6 lines

will
have to flush regardless. And my recent attempt to fix the log replay issue
relies on i_update_core to indicate there are unlogged changes - I don't see
how that will work with these changes.

Quoted 3 lines

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 another.

Could not be implemented? What was that patch I sent around then?

Quoted 5 lines

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.

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 (I think).

Quoted 22 lines · Lachlan McIlroy

Quoted 21 lines · David Chinner

Quoted 20 lines · Lachlan McIlroy

Quoted 13 lines · David Chinner

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 via the 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 xfssyncd for inodes that are only dirtied by setting i_update_core = 1 and the other from the xfsaild when log tail pushing.

Quoted 5 lines

Ideally we should only have a single method for pushing out inodes. The first step to that is tracking the dirty state in a single tree (the inode radix trees). That means we have to hook ->dirty_inode() to catch all dirtying via mark_inode_dirty[_sync]() and mark the inodes dirty in the radix tree. Then we need to use xfs_mark_inode_dirty_sync() everywhere that we dirty the inode.

Don't we already call mark_inode_dirty[_sync]() everywhere we dirty the inode?

Quoted 6 lines

Quoted 5 lines

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?

Yup.

Quoted 2 lines

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.

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

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 around.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

By consolidating the different ways of dirtying an inode we lose the ability 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.

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

For example if the inode log item has bits set then we know we have to flush 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.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

will
have to flush regardless. And my recent attempt to fix the log replay issue relies on i_update_core to indicate there are unlogged changes - I don't see 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 another.

Could not be implemented? What was that patch I sent around then?

Sorry, I missed an important work there - could not be implemented _efficiently_.

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.

Also, the change fails to log changes to inodes in the same cluster that get written out because they are dirty.

Quoted 11 lines

Quoted 6 lines

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).....

Quoted 4 lines

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 (I think).

Yup, pretty much.

Cheers,

Dave.

Quoted 6 lines · David Chinner

Quoted 4 lines

Quoted 3 lines

[ 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?

Yup.

I've never liked that check, can we just get rid of it?

Quoted 7 lines

Sorry, I missed an important work there - could not be implemented _efficiently_.

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 later.

Quoted 2 lines

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.

Quoted 12 lines

Quoted 11 lines

Quoted 5 lines

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.

Quoted 5 lines

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.