Optimize wait_sb_inodes()

 fs/fs-writeback.c  |    5 ++++-
 include/linux/fs.h |    1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff -puN include/linux/fs.h~tux3-fix-wait_sb_inodes include/linux/fs.h

include/linux/fs.h renamed from include/linux/fs.h~tux3-fix-wait_sb_inodes +1 -0

@@ -1595,6 +1595,7 @@ struct super_operations {
 	int (*write_inode) (struct inode *, struct writeback_control *wbc);
 	int (*drop_inode) (struct inode *);
 	void (*evict_inode) (struct inode *);
+	void (*wait_inodes)(struct super_block *);
 	void (*put_super) (struct super_block *);
 	int (*sync_fs)(struct super_block *sb, int wait);
 	int (*freeze_fs) (struct super_block *);

diff -puN fs/fs-writeback.c~tux3-fix-wait_sb_inodes fs/fs-writeback.c

Two things. Until there are actual implementations of s_op->wait_inodes, this is pure obfuscation. You already know this, of course.

More interestingly, I personally hate methods with a default option if they are not implemented. Not too bad in this particular case, but the same pattern has burned me a number of times and wasted weeks of my life. So I would prefer to unconditionally call sb->s_op->wait_inodes(sb) and set it to wait_sb_inodes for all filesystems that don't have a smarter way to do things.

Hi,

On the following stress, sync(2) became top of cpu load.

	fsstress -s 1 -n 50000 -p 3 -d `pwd`

After profile by perf, cpu load was lock contention of inode_sb_list_lock.

sync(2) is data integrity operation, so it has to make sure all dirty data was written before sync(2) point. The bdi flusher flushes current dirty data and wait those. But, if there is in-flight I/O before sync_inodes_sb(), sync_inodes_sb() can be done before prior in-flight I/O.

So, wait_sb_inodes() is walking the all inodes on sb to make sure in-flight I/O was done too. When it is walking inodes, inode_sb_list_lock is contending with other operations like create(2). This is the cause of cpu load.

On another view, wait_sb_inodes() would (arguably) be necessary for legacy FSes. But, for example, if data=journal on ext*, wait_sb_inodes() would be more than useless, because ext* can be done it by own transaction list (and more efficient way).

Likewise, on tux3, the state is same with data=journal.

Also, even if data=ordered, ext* might be able to check in-flight I/O by ordered data list (with some new additional check, I'm not sure).

Quoted 9 lines

On another view, wait_sb_inodes() would (arguably) be necessary for legacy FSes. But, for example, if data=journal on ext*, wait_sb_inodes() would be more than useless, because ext* can be done it by own transaction list (and more efficient way).

Likewise, on tux3, the state is same with data=journal.

Also, even if data=ordered, ext* might be able to check in-flight I/O by ordered data list (with some new additional check, I'm not sure).

Why would you bother solving this problem differently in every single filesystem? It's solvable at the VFS by tracking inodes that are no longer dirty but still under writeback on the BDI. Then converting wait_sb_inodes() to walk all the dirty and writeback inodes would be sufficient for data integrity purposes, and it would be done under the bdi writeback lock, not the inode_sb_list_lock....

Alternatively, splitting up the inode sb list and lock (say via the per-node list_lru structures in -mm and -next that are being added for exactly this purpose) would also significantly reduce lock contention on both the create/evict fast paths and the wait_sb_inodes() walk that is currently done....

So I think that you should address the problem properly at the VFS level so everyone benefits, not push interfaces that allow filesystem specific hacks to work around VFS level deficiencies...

Quoted 23 lines · Dave Chinner

Quoted 9 lines

On another view, wait_sb_inodes() would (arguably) be necessary for legacy FSes. But, for example, if data=journal on ext*, wait_sb_inodes() would be more than useless, because ext* can be done it by own transaction list (and more efficient way).

Likewise, on tux3, the state is same with data=journal.

Also, even if data=ordered, ext* might be able to check in-flight I/O by ordered data list (with some new additional check, I'm not sure).

Why would you bother solving this problem differently in every single filesystem? It's solvable at the VFS by tracking inodes that are no longer dirty but still under writeback on the BDI. Then converting wait_sb_inodes() to walk all the dirty and writeback inodes would be sufficient for data integrity purposes, and it would be done under the bdi writeback lock, not the inode_sb_list_lock....

Alternatively, splitting up the inode sb list and lock (say via the per-node list_lru structures in -mm and -next that are being added for exactly this purpose) would also significantly reduce lock contention on both the create/evict fast paths and the wait_sb_inodes() walk that is currently done....

Quoted 3 lines

So I think that you should address the problem properly at the VFS level so everyone benefits, not push interfaces that allow filesystem specific hacks to work around VFS level deficiencies...

Optimizing wait_sb_inodes() might help lock contention, but it doesn't help unnecessary wait/check.

Since some FSes know about current
in-flight I/O already in those internal, so I think, those FSes can be done it here, or are already doing in ->sync_fs().

Quoted 2 lines

Optimizing wait_sb_inodes() might help lock contention, but it doesn't help unnecessary wait/check.

You have your own wait code, that doesn't make what the VFS does unnecesary. Quite frankly, I don't trust individual filesystems to get it right - there's a long history of filesystem specific data sync problems (including in XFS), and the best way to avoid that is to ensure the VFS gets it right for you.

Indeed, we've gone from having sooper special secret sauce data sync code in XFS to using the VFS over the past few years, and the result is that it is now more reliable and faster than when we were trying to be smart and do it all ourselves. We got to where we are by fixing the problems in the VFS rather than continuing to try to work around them.

Quoted 3 lines

Since some FSes know about current
in-flight I/O already in those internal, so I think, those FSes can be done it here, or are already doing in ->sync_fs().

Sure, do your internal checks in ->sync_fs(), but if wait_sb_inodes() does not have any lock contention and very little overhead, then why do you need to avoid it? This wait has to be done somewhere between sync_inodes_sb() dispatching all the IO and ->sync_fs completing, so what's the problem with hving the VFS do that *for everyone* efficiently?

Fix the root cause of the problem - the sub-optimal VFS code. Hacking around it specifically for out-of-tree code is not the way things get done around here...

Quoted 9 lines · Dave Chinner

Quoted 2 lines

Optimizing wait_sb_inodes() might help lock contention, but it doesn't help unnecessary wait/check.

You have your own wait code, that doesn't make what the VFS does unnecesary. Quite frankly, I don't trust individual filesystems to get it right - there's a long history of filesystem specific data sync problems (including in XFS), and the best way to avoid that is to ensure the VFS gets it right for you.

Quoted 6 lines

Indeed, we've gone from having sooper special secret sauce data sync code in XFS to using the VFS over the past few years, and the result is that it is now more reliable and faster than when we were trying to be smart and do it all ourselves. We got to where we are by fixing the problems in the VFS rather than continuing to try to work around them.

I guess you are assuming FS which is using data=writeback or such.

Quoted 11 lines

Quoted 4 lines

Since some FSes know about current
in-flight I/O already in those internal, so I think, those FSes can be done it here, or are already doing in ->sync_fs().

Sure, do your internal checks in ->sync_fs(), but if wait_sb_inodes() does not have any lock contention and very little overhead, then why do you need to avoid it? This wait has to be done somewhere between sync_inodes_sb() dispatching all the IO and ->sync_fs completing, so what's the problem with hving the VFS do that *for everyone* efficiently?

Are you saying the vfs should track all in-flight I/O with some sort of transactional way?

Otherwise, vfs can't know the data is whether after sync point or before sync point, and have to wait or not. FS is using the behavior like data=journal has tracking of those already, and can reuse it.

Quoted 3 lines

Fix the root cause of the problem - the sub-optimal VFS code. Hacking around it specifically for out-of-tree code is not the way things get done around here...

I'm thinking the root cause is vfs can't have knowledge of FS internal, e.g. FS is handling data transactional way, or not.

Quoted 3 lines

Otherwise, vfs can't know the data is whether after sync point or before sync point, and have to wait or not. FS is using the behavior like data=journal has tracking of those already, and can reuse it.

The VFS writeback code already differentiates between data written during a sync operation and that dirtied after a sync operation. Perhaps you should look at the tagging for WB_SYNC_ALL writeback that write_cache_pages does....

But, anyway, we don't have to do that on the waiting side of things. All we need to do is add the inode to a "under IO" list on the bdi when the mapping is initially tagged with pages under writeback, and remove it from that list during IO completion when the mapping is no longer tagged as having pages under writeback.

wait_sb_inodes() just needs to walk that list and wait on each inode to complete IO. It doesn't require *any awareness* of the underlying filesystem implementation or how the IO is actually issued - if there's IO in progress at the time wait_sb_inodes() is called, then it waits for it.

Quoted 6 lines

Quoted 3 lines

Fix the root cause of the problem - the sub-optimal VFS code. Hacking around it specifically for out-of-tree code is not the way things get done around here...

I'm thinking the root cause is vfs can't have knowledge of FS internal, e.g. FS is handling data transactional way, or not.

If the filesystem has transactional data/metadata that the VFS is not tracking, then that is what the ->sync_fs call is for. i.e. so the filesystem can then do what ever extra writeback/waiting it needs to do that the VFS is unaware of.

We already cater for what Tux3 needs in the VFS - all you've done is found an inefficient algorithm that needs fixing.

Quoted 33 lines · Dave Chinner

Quoted 3 lines

Otherwise, vfs can't know the data is whether after sync point or before sync point, and have to wait or not. FS is using the behavior like data=journal has tracking of those already, and can reuse it.

The VFS writeback code already differentiates between data written during a sync operation and that dirtied after a sync operation. Perhaps you should look at the tagging for WB_SYNC_ALL writeback that write_cache_pages does....

But, anyway, we don't have to do that on the waiting side of things. All we need to do is add the inode to a "under IO" list on the bdi when the mapping is initially tagged with pages under writeback, and remove it from that list during IO completion when the mapping is no longer tagged as having pages under writeback.

wait_sb_inodes() just needs to walk that list and wait on each inode to complete IO. It doesn't require *any awareness* of the underlying filesystem implementation or how the IO is actually issued - if there's IO in progress at the time wait_sb_inodes() is called, then it waits for it.

Quoted 6 lines

Quoted 3 lines

Fix the root cause of the problem - the sub-optimal VFS code. Hacking around it specifically for out-of-tree code is not the way things get done around here...

I'm thinking the root cause is vfs can't have knowledge of FS internal, e.g. FS is handling data transactional way, or not.

If the filesystem has transactional data/metadata that the VFS is not tracking, then that is what the ->sync_fs call is for. i.e. so the filesystem can then do what ever extra writeback/waiting it needs to do that the VFS is unaware of.

Quoted 2 lines

We already cater for what Tux3 needs in the VFS - all you've done is found an inefficient algorithm that needs fixing.

write_cache_pages() is library function to be called from per-FS. So, it is not under vfs control can be assume already. And it doesn't do right things via write_cache_pages() for data=journal, because it handles for each inodes, not at once. So, new dirty data can be inserted while marking.

Quoted 12 lines

Quoted 12 lines

Quoted 6 lines

Quoted 3 lines

Fix the root cause of the problem - the sub-optimal VFS code. Hacking around it specifically for out-of-tree code is not the way things get done around here...

I'm thinking the root cause is vfs can't have knowledge of FS internal, e.g. FS is handling data transactional way, or not.

If the filesystem has transactional data/metadata that the VFS is not tracking, then that is what the ->sync_fs call is for. i.e. so the filesystem can then do what ever extra writeback/waiting it needs to do that the VFS is unaware of.

Quoted 8 lines

Quoted 2 lines

We already cater for what Tux3 needs in the VFS - all you've done is found an inefficient algorithm that needs fixing.

write_cache_pages() is library function to be called from per-FS. So, it is not under vfs control can be assume already. And it doesn't do right things via write_cache_pages() for data=journal, because it handles for each inodes, not at once. So, new dirty data can be inserted while marking.

Sure it can. But that newly dirtied data has occurred after the data integrity writeback call was begun, so it's not part of what the writeback code call needs to write back. We are quite entitled to ignore it for the purposes of a data integrity sync because it as dirtied *after* write_cache_pages() was asked to sync the range of the inode.

IOWs, the VFS draws a line in the sand at a point in time when each inode is written for a data integrity sync. You have to do that somewhere, and there's little point in making that a global barrier when it is not necessary to do so.

tux3 draws a different line in the sand, as does ext3/4 data=journal. In effect, tux3 and ext3/4 data=journal define a global point in time that everything is "in sync", and that's way above what is necessary for a sync(2) operation. The VFS already has equivalent functionality - it's the state we force filesystems into when they are frozen. i.e. freezing a filesystem forces it down into a state where it is transactionally consistent on disk w.r.t both data and metadata. sync(2) does not require these "transactionally consistent" semantics, so the VFS does not try to provide them.

Anyway, this is a moot discussion. I've already got prototype code that fixes the wait_sb_inodes() problem as somebody is having problems with many concurrent executions of wait_sb_inodes() causing severe lock contention...

Let's go up a level. Why are you so focused on optimizing sync(2)? What's the use case where you anticipate applications using sync(2) or
syncfs(2)? Or is this for the purpose of Tux3's benchmarketing?

In practice, sync(2) as a data integrity mechanism is a bit heavyweight for most applications. In general the complaint with ext3 or ext4 is that fsync(2) is forcing a full filesystem-wide sync, and not just a sync specific to the file.

System administrators use it sometimes[1], sometimes out of superstition, but how many applications actually use it? And why?

[1] In fact, I've heard an argument that sync(2) should be optionally made privileged and only accessible to root, since there were cases where system administrators would login to a heavily loaded server as a non-privileged user, type "sync" out of reflex or habit, and the resulting huge amounts of I/O would blow the 99.9th percentile latency guarantees for all of the latency-sensitive services running on that server.

If the goal is to optimize file system freeze operations, sure. But that's also not a common operation, so if it burns a little extra CPU time, it wouldn't cause me to think that it was a high priority issue. Decreasing the wall clock time for a file system freeze operation would probably be a far more interesting goal.

Well, anyway, it is simple. This issue was came as the performance regression when I was experimenting to use kernel bdi flusher from own flusher. The issue was sync(2) like I said. And this was just I couldn't solve this issue by tux3 side unlike other optimizations.

Quoted 5 lines · OGAWA Hirofumi

Well, anyway, it is simple. This issue was came as the performance regression when I was experimenting to use kernel bdi flusher from own flusher. The issue was sync(2) like I said. And this was just I couldn't solve this issue by tux3 side unlike other optimizations.

A performance regression using fsstress? That's not a program intended to be a useful benchmark for measuring performance.

Quoted 7 lines · Theodore Ts'o

Quoted 5 lines · OGAWA Hirofumi

Well, anyway, it is simple. This issue was came as the performance regression when I was experimenting to use kernel bdi flusher from own flusher. The issue was sync(2) like I said. And this was just I couldn't solve this issue by tux3 side unlike other optimizations.

Quoted 2 lines

A performance regression using fsstress? That's not a program intended to be a useful benchmark for measuring performance.

Right. fsstress is used as stress tool for me too as part of CI, with background vmstat 1. Anyway, it is why I noticed this.

I agree it would not be high priority. But I don't think we should stop to optimize it.

Quoted 6 lines

Quoted 2 lines

A performance regression using fsstress? That's not a program intended to be a useful benchmark for measuring performance.

Right. fsstress is used as stress tool for me too as part of CI, with background vmstat 1. Anyway, it is why I noticed this.

Quoted 2 lines

I agree it would not be high priority. But I don't think we should stop to optimize it.

But you're not proposing any sort of optimisation at all - you're simply proposing to hack around the problem so you don't have to care about it. The VFS is a shared resource - it has to work well for everyone - and that means we need to fix problems and not ignore them.

If the goal is to optimize file system freeze operations, sure. But that's also not a common operation, so if it burns a little extra CPU time, it wouldn't cause me to think that it was a high priority issue. Decreasing the wall clock time for a file system freeze operation would probably be a far more interesting goal.

...vfs can't know the data is whether after sync point or before sync point, and have to wait or not. FS is using the behavior like data=journal has tracking of those already, and can reuse it.

You have your own wait code, that doesn't make what the VFS does unnecesary. Quite frankly, I don't trust individual filesystems to get it right - there's a long history of filesystem specific data sync problems (including in XFS), and the best way to avoid that is to ensure the VFS gets it right for you.