fs: Make sync_file_range(2) use WB_SYNC_NONE writeback

PATCH In review Jan Kara 3 participants 5 messages Oct 25, 2015 – Nov 6, 2015 VFS

Share & tools

Jan Kara

author

2015-10-25 04:14 UTC #0 lore raw

PATCH 1/1 fs: Make sync_file_range(2) use WB_SYNC_NONE writeback

sync_file_range(2) is documented to issue writeback only for pages that are not currently being written. After all the system call has been created for userspace to be able to issue background writeout and so waiting for in-flight IO is undesirable there. However commit ee53a891f474 (mm: do_sync_mapping_range integrity fix) switched do_sync_mapping_range() and thus sync_file_range() to issue writeback in WB_SYNC_ALL mode since do_sync_mapping_range() was used by other code relying on WB_SYNC_ALL semantics.

These days do_sync_mapping_range() went away and we can switch sync_file_range(2) back to issuing WB_SYNC_NONE writeback. That should help PostgreSQL avoid large latency spikes when flushing data in the background.

Reported-by Andres Freund <[email protected]>

Signed-off-by Jan Kara <[email protected]>

---
 fs/sync.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/sync.c +2 -1

index fbc98ee62044..ef60e812d771 100644
@@ -343,7 +343,8 @@ SYSCALL_DEFINE4(sync_file_range, int, fd, loff_t, offset, loff_t, nbytes,
 	}
 
 	if (flags & SYNC_FILE_RANGE_WRITE) {
-		ret = filemap_fdatawrite_range(mapping, offset, endbyte);
+		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
+						 WB_SYNC_NONE);
 		if (ret < 0)
 			goto out_put;
 	}

Signature · 1 line

2.1.4

Andres Freund

2015-10-28 09:30 UTC #1 lore raw

Hi Jan,

Thanks for looking into this and promptly sending a patch.

Quoted 10 lines · Jan Kara

These days do_sync_mapping_range() went away and we can switch sync_file_range(2) back to issuing WB_SYNC_NONE writeback. That should help PostgreSQL avoid large latency spikes when flushing data in the background.
fs/sync.c +0 -0
index fbc98ee62044..ef60e812d771 100644

fs/sync.c SYSCALL_DEFINE4(sync_file_range, int, fd, loff_t, offset, loff_t, nbytes, · line 350

Quoted 9 lines

fs/sync.c +2 -1

@@ -343,7 +343,8 @@ SYSCALL_DEFINE4(sync_file_range, int, fd, loff_t, offset, loff_t, nbytes,
 	}
 
 	if (flags & SYNC_FILE_RANGE_WRITE) {
-		ret = filemap_fdatawrite_range(mapping, offset, endbyte);
+		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
+						 WB_SYNC_NONE);
 		if (ret < 0)
 			goto out_put;
 	}

Thanks. Would scheduling a comparative benchmark of this be helpful pushing htis forward ? Would probably only be early next week, I'm at the european postgresql conference right now.

Regards,

Andres

Jan Kara

reviewer

2015-10-28 14:19 UTC #2 lore raw

Hi Andres,

Quoted 13 lines · Andres Freund

Quoted 12 lines · Jan Kara
These days do_sync_mapping_range() went away and we can switch sync_file_range(2) back to issuing WB_SYNC_NONE writeback. That should help PostgreSQL avoid large latency spikes when flushing data in the background.
fs/sync.c +0 -0
index fbc98ee62044..ef60e812d771 100644
@@ -343,7 +343,8 @@ SYSCALL_DEFINE4(sync_file_range, int, fd, loff_t, offset, loff_t, nbytes,
 	}
 

Quoted 11 lines

fs/sync.c SYSCALL_DEFINE4(sync_file_range, int, fd, loff_t, offset, loff_t, nbytes, · line 350
Quoted 7 lines
fs/sync.c +2 -1
 	if (flags & SYNC_FILE_RANGE_WRITE) {
-		ret = filemap_fdatawrite_range(mapping, offset, endbyte);
+		ret = __filemap_fdatawrite_range(mapping, offset, endbyte,
+						 WB_SYNC_NONE);
 		if (ret < 0)
 			goto out_put;
 	}
Thanks. Would scheduling a comparative benchmark of this be helpful pushing htis forward ? Would probably only be early next week, I'm at the european postgresql conference right now.

If you could run it, it would be nice. Thanks!

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Andres Freund

Tested-by

2015-11-06 13:06 UTC #3 lore raw

Hi,

Quoted 7 lines · Jan Kara

Quoted 4 lines · Andres Freund
On 2015-10-24 21:28:17 +0200, Jan Kara wrote:
Thanks. Would scheduling a comparative benchmark of this be helpful pushing htis forward ? Would probably only be early next week, I'm at the european postgresql conference right now.
If you could run it, it would be nice. Thanks!

Sorry that it took longer. Had some $work deadline making a surprise attack (sneaky bastard) and then difficulity getting time on a bigger box. Hence I only have numbers from a single SSD drive (840 EVO 1TB) laptop (16GB RAM, i7-4800MQ), with nothing else running.

I've compared 4.3.0-andres-07965-gd1e41ff, with/without the patch applied. Best of three results:

Before:
transaction type: TPC-B (sort of)
scaling factor: 300
query mode: prepared
number of clients: 48
number of threads: 48
duration: 1000 s
number of transactions actually processed: 7873859 latency average: 6.094 ms
latency stddev: 35.376 ms
tps = 7871.887045 (including connections establishing) tps = 7872.135871 (excluding connections establishing)

After:
transaction type: TPC-B (sort of)
scaling factor: 300
query mode: prepared
number of clients: 48
number of threads: 48
duration: 1000 s
number of transactions actually processed: 9370637 latency average: 5.119 ms
latency stddev: 24.423 ms
tps = 9369.212486 (including connections establishing) tps = 9369.725595 (excluding connections establishing)

Pretty tidy improvement I'd say.

Feel free to add a

Tested-by Andres Freund <[email protected]>

There's still quite some further room of improvement. E.g. some quite massive peaks in latency, which don't coincide with background work in postgres:
progress: 869.0 s, 9971.2 tps, lat 4.828 ms stddev 6.201
progress: 870.0 s, 8196.4 tps, lat 4.632 ms stddev 6.774
progress: 871.0 s, 497.0 tps, lat 89.598 ms stddev 308.398
progress: 872.0 s, 10360.6 tps, lat 5.917 ms stddev 40.041
progress: 873.0 s, 7005.5 tps, lat 6.743 ms stddev 21.985

Without controlling writeout via sync_file_range(), we frequently can see stalls in the tens of seconds on busy machines though... So this is still much better ;)

Greetings,

Andres Freund

Jan Kara

reviewer

2015-11-06 20:43 UTC #4 lore raw

Hello,

Quoted 58 lines · Andres Freund

Quoted 7 lines · Jan Kara
Quoted 4 lines · Andres Freund
On 2015-10-24 21:28:17 +0200, Jan Kara wrote:
Thanks. Would scheduling a comparative benchmark of this be helpful pushing htis forward ? Would probably only be early next week, I'm at the european postgresql conference right now.
If you could run it, it would be nice. Thanks!
Sorry that it took longer. Had some $work deadline making a surprise attack (sneaky bastard) and then difficulity getting time on a bigger box. Hence I only have numbers from a single SSD drive (840 EVO 1TB) laptop (16GB RAM, i7-4800MQ), with nothing else running.
I've compared 4.3.0-andres-07965-gd1e41ff, with/without the patch applied. Best of three results:
Before:
transaction type: TPC-B (sort of)
scaling factor: 300
query mode: prepared
number of clients: 48
number of threads: 48
duration: 1000 s
number of transactions actually processed: 7873859 latency average: 6.094 ms
latency stddev: 35.376 ms
tps = 7871.887045 (including connections establishing) tps = 7872.135871 (excluding connections establishing)
After:
transaction type: TPC-B (sort of)
scaling factor: 300
query mode: prepared
number of clients: 48
number of threads: 48
duration: 1000 s
number of transactions actually processed: 9370637 latency average: 5.119 ms
latency stddev: 24.423 ms
tps = 9369.212486 (including connections establishing) tps = 9369.725595 (excluding connections establishing)
Pretty tidy improvement I'd say.
Feel free to add a
Tested-by Andres Freund <[email protected]>
There's still quite some further room of improvement. E.g. some quite massive peaks in latency, which don't coincide with background work in postgres:
progress: 869.0 s, 9971.2 tps, lat 4.828 ms stddev 6.201
progress: 870.0 s, 8196.4 tps, lat 4.632 ms stddev 6.774
progress: 871.0 s, 497.0 tps, lat 89.598 ms stddev 308.398
progress: 872.0 s, 10360.6 tps, lat 5.917 ms stddev 40.041
progress: 873.0 s, 7005.5 tps, lat 6.743 ms stddev 21.985

Quoted 3 lines

Without controlling writeout via sync_file_range(), we frequently can see stalls in the tens of seconds on busy machines though... So this is still much better ;)

Thanks for doing the test and I'm happy it improved your results. Andrew has already picked up the patch into his tree so it will eventually hit the upstream kernel (not sure if for 4.4 already but for 4.5 certainly).

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR