[PATCH v2 0/2] fat: improve synchronous mount performance

[PATCH v2 0/2] fat: improve synchronous mount performance

[Matheus Alves de Almeida]

Hi,

This series improves performance on synchronous mounts by batching
FAT mirror writes and improving fat_sync_bhs() submission.
 
The benchmarks were run on top of my other FAT allocation batching
changes currently in linux-next.

Changes in v2:
- fat_sync_bhs(): make err a local variable and return early on error.
- fat_sync_one(): make err2 a local variable.

Thanks,
Matheus

Matheus Alves de Almeida (2):
  fat: batch synchronous mirror writes
  fat: improve fat_sync_bhs() write submission

 fs/fat/fat.h    |  1 +
 fs/fat/fatent.c | 80 +++++++++++++++++++++++++++++++++++--------------
 fs/fat/misc.c   | 14 ++++++++-
 3 files changed, 71 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] fat: batch synchronous mirror writes

[Matheus Alves de Almeida]

fat_mirror_bhs() currently synchronizes buffer-by-buffer on synchronous
mounts, serializing I/O and degrading performance.

Make fat_mirror_bhs() batch dirty buffer heads and use fat_sync_bhs()
to sync several at once. Add a fat_brelse_array() helper that releases
buffer heads in an array.

Benchmarking was done under QEMU/KVM with a virtio-blk device using
cache=directsync. Each result is the arithmetic mean of 10 samples.

start = now_ns();
for (unsigned i = 0; i < count; i++) {
	fallocate(fds[i], FALLOC_FL_KEEP_SIZE, 0, bytes);
}
elapsed = now_ns() - start;

Sync-mounted FAT32 using count=16 files:
File size    Cluster size    Before    After    Improvement
256MiB       512B            49.81s    12.26s   4.06x
1MiB         512B            0.198s    0.057s   3.50x
256MiB       4096B           7.060s    1.640s   4.31x
1MiB         4096B           0.073s    0.028s   2.60x

Asynchronous mounts were also benchmarked with no noticeable performance
change.

Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>
---
 fs/fat/fat.h    |  1 +
 fs/fat/fatent.c | 80 +++++++++++++++++++++++++++++++++++--------------
 fs/fat/misc.c   |  6 ++++
 3 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 1d051d0628b6..204c7b87108f 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -479,6 +479,7 @@ void fat_truncate_time(struct inode *inode, struct timespec64 *now,
 int fat_update_time(struct inode *inode, enum fs_update_time type,
 		unsigned int flags);
 extern int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs);
+void fat_brelse_array(struct buffer_head *bhs[], int n);
 
 int fat_cache_init(void);
 void fat_cache_destroy(void);
diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
index 8897a5dd522d..d42b5324531a 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -376,42 +376,76 @@ int fat_ent_read(struct inode *inode, struct fat_entry *fatent, int entry)
 	return ops->ent_get(fatent);
 }
 
-/* FIXME: We can write the blocks as more big chunk. */
-static int fat_mirror_bhs(struct super_block *sb, struct buffer_head **bhs,
-			  int nr_bhs)
+static int fat_mirror_one(struct super_block *sb, struct buffer_head **bhs,
+			  int nr_bhs, int target)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	sector_t backup_fat = sbi->fat_length * target;
+	/*
+	 * Use the value of MAX_BUF_PER_PAGE for 4K pages. This reduces stack
+	 * usage on large-page architectures as callers may already have a
+	 * MAX_BUF_PER_PAGE-sized array in their stack.
+	 */
+	struct buffer_head *dirty[8];
 	struct buffer_head *c_bh;
-	int err, n, copy;
+	int err, n, nr_dirty;
 
+	nr_dirty = 0;
 	err = 0;
-	for (copy = 1; copy < sbi->fats; copy++) {
-		sector_t backup_fat = sbi->fat_length * copy;
-
-		for (n = 0; n < nr_bhs; n++) {
-			c_bh = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
-			if (!c_bh) {
-				err = -ENOMEM;
-				goto error;
+	for (n = 0; n < nr_bhs; n++) {
+		c_bh = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
+		if (!c_bh) {
+			err = -ENOMEM;
+			goto error;
+		}
+		/* Avoid race with userspace read via bdev */
+		lock_buffer(c_bh);
+		memcpy(c_bh->b_data, bhs[n]->b_data, sb->s_blocksize);
+		set_buffer_uptodate(c_bh);
+		unlock_buffer(c_bh);
+		mmb_mark_buffer_dirty(c_bh,
+			&MSDOS_I(sbi->fat_inode)->i_metadata_bhs);
+		if (sb->s_flags & SB_SYNCHRONOUS) {
+			dirty[nr_dirty] = c_bh;
+			++nr_dirty;
+			if (nr_dirty == ARRAY_SIZE(dirty)) {
+				err = fat_sync_bhs(dirty, nr_dirty);
+				fat_brelse_array(dirty, nr_dirty);
+				nr_dirty = 0;
+				if (err)
+					goto error;
 			}
-			/* Avoid race with userspace read via bdev */
-			lock_buffer(c_bh);
-			memcpy(c_bh->b_data, bhs[n]->b_data, sb->s_blocksize);
-			set_buffer_uptodate(c_bh);
-			unlock_buffer(c_bh);
-			mmb_mark_buffer_dirty(c_bh,
-				&MSDOS_I(sbi->fat_inode)->i_metadata_bhs);
-			if (sb->s_flags & SB_SYNCHRONOUS)
-				err = sync_dirty_buffer(c_bh);
+		} else {
 			brelse(c_bh);
-			if (err)
-				goto error;
 		}
 	}
+
 error:
+	if (nr_dirty) {
+		int err2 = fat_sync_bhs(dirty, nr_dirty);
+		if (!err)
+			err = err2;
+		fat_brelse_array(dirty, nr_dirty);
+	}
+
 	return err;
 }
 
+static int fat_mirror_bhs(struct super_block *sb, struct buffer_head **bhs,
+			  int nr_bhs)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	int copy;
+
+	for (copy = 1; copy < sbi->fats; copy++) {
+		int err = fat_mirror_one(sb, bhs, nr_bhs, copy);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 int fat_ent_write(struct inode *inode, struct fat_entry *fatent,
 		  int new, int wait)
 {
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index 0d04228f916e..b6a9e2e122cd 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -365,3 +365,9 @@ int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs)
 	}
 	return err;
 }
+
+void fat_brelse_array(struct buffer_head *bhs[], int n)
+{
+	while (--n >= 0)
+		brelse(bhs[n]);
+}
-- 
2.43.0

[PATCH v2 2/2] fat: improve fat_sync_bhs() write submission

[Matheus Alves de Almeida]

fat_sync_bhs() does many individual write_dirty_buffer() calls without
plugging, making batching in the block layer more difficult. It also
does not pass the REQ_SYNC hint for these synchronous writes.

Use a plug around the writes so the block layer can more easily batch
them. Pass the REQ_SYNC hint to write_dirty_buffer().

Benchmarking was done under QEMU/KVM with a virtio-blk device using
cache=directsync. Each result is the arithmetic mean of 10 samples.

start = now_ns();
for (unsigned i = 0; i < count; i++) {
    fallocate(fds[i], FALLOC_FL_KEEP_SIZE, 0, bytes);
}
elapsed = now_ns() - start;

Sync-mounted FAT32 using count=16 files:
File size    Cluster size    Before    After    Improvement
256MiB       512B            13.11s    11.87s   1.10x
256MiB       4096B           1.657s    1.469s   1.13x

Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>
---
 fs/fat/misc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index b6a9e2e122cd..e8e2f13254d5 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -9,6 +9,7 @@
 
 #include "fat.h"
 #include <linux/iversion.h>
+#include <linux/blk_plug.h>
 
 /*
  * fat_fs_error reports a file system problem that might indicate fa data
@@ -354,9 +355,14 @@ EXPORT_SYMBOL_GPL(fat_update_time);
 int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs)
 {
 	int i, err = 0;
+	struct blk_plug plug;
+
+	blk_start_plug(&plug);
 
 	for (i = 0; i < nr_bhs; i++)
-		write_dirty_buffer(bhs[i], 0);
+		write_dirty_buffer(bhs[i], REQ_SYNC);
+
+	blk_finish_plug(&plug);
 
 	for (i = 0; i < nr_bhs; i++) {
 		wait_on_buffer(bhs[i]);
-- 
2.43.0

Re: [PATCH v2 1/2] fat: batch synchronous mirror writes

[OGAWA Hirofumi]

Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br> writes:

> fat_mirror_bhs() currently synchronizes buffer-by-buffer on synchronous
> mounts, serializing I/O and degrading performance.
>
> Make fat_mirror_bhs() batch dirty buffer heads and use fat_sync_bhs()
> to sync several at once. Add a fat_brelse_array() helper that releases
> buffer heads in an array.
>
> Benchmarking was done under QEMU/KVM with a virtio-blk device using
> cache=directsync. Each result is the arithmetic mean of 10 samples.
>
> start = now_ns();
> for (unsigned i = 0; i < count; i++) {
> 	fallocate(fds[i], FALLOC_FL_KEEP_SIZE, 0, bytes);
> }
> elapsed = now_ns() - start;
>
> Sync-mounted FAT32 using count=16 files:
> File size    Cluster size    Before    After    Improvement
> 256MiB       512B            49.81s    12.26s   4.06x
> 1MiB         512B            0.198s    0.057s   3.50x
> 256MiB       4096B           7.060s    1.640s   4.31x
> 1MiB         4096B           0.073s    0.028s   2.60x
>
> Asynchronous mounts were also benchmarked with no noticeable performance
> change.
>
> Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>

Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

> ---
>  fs/fat/fat.h    |  1 +
>  fs/fat/fatent.c | 80 +++++++++++++++++++++++++++++++++++--------------
>  fs/fat/misc.c   |  6 ++++
>  3 files changed, 64 insertions(+), 23 deletions(-)
>
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 1d051d0628b6..204c7b87108f 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -479,6 +479,7 @@ void fat_truncate_time(struct inode *inode, struct timespec64 *now,
>  int fat_update_time(struct inode *inode, enum fs_update_time type,
>  		unsigned int flags);
>  extern int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs);
> +void fat_brelse_array(struct buffer_head *bhs[], int n);
>  
>  int fat_cache_init(void);
>  void fat_cache_destroy(void);
> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
> index 8897a5dd522d..d42b5324531a 100644
> --- a/fs/fat/fatent.c
> +++ b/fs/fat/fatent.c
> @@ -376,42 +376,76 @@ int fat_ent_read(struct inode *inode, struct fat_entry *fatent, int entry)
>  	return ops->ent_get(fatent);
>  }
>  
> -/* FIXME: We can write the blocks as more big chunk. */
> -static int fat_mirror_bhs(struct super_block *sb, struct buffer_head **bhs,
> -			  int nr_bhs)
> +static int fat_mirror_one(struct super_block *sb, struct buffer_head **bhs,
> +			  int nr_bhs, int target)
>  {
>  	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +	sector_t backup_fat = sbi->fat_length * target;
> +	/*
> +	 * Use the value of MAX_BUF_PER_PAGE for 4K pages. This reduces stack
> +	 * usage on large-page architectures as callers may already have a
> +	 * MAX_BUF_PER_PAGE-sized array in their stack.
> +	 */
> +	struct buffer_head *dirty[8];
>  	struct buffer_head *c_bh;
> -	int err, n, copy;
> +	int err, n, nr_dirty;
>  
> +	nr_dirty = 0;
>  	err = 0;
> -	for (copy = 1; copy < sbi->fats; copy++) {
> -		sector_t backup_fat = sbi->fat_length * copy;
> -
> -		for (n = 0; n < nr_bhs; n++) {
> -			c_bh = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
> -			if (!c_bh) {
> -				err = -ENOMEM;
> -				goto error;
> +	for (n = 0; n < nr_bhs; n++) {
> +		c_bh = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr);
> +		if (!c_bh) {
> +			err = -ENOMEM;
> +			goto error;
> +		}
> +		/* Avoid race with userspace read via bdev */
> +		lock_buffer(c_bh);
> +		memcpy(c_bh->b_data, bhs[n]->b_data, sb->s_blocksize);
> +		set_buffer_uptodate(c_bh);
> +		unlock_buffer(c_bh);
> +		mmb_mark_buffer_dirty(c_bh,
> +			&MSDOS_I(sbi->fat_inode)->i_metadata_bhs);
> +		if (sb->s_flags & SB_SYNCHRONOUS) {
> +			dirty[nr_dirty] = c_bh;
> +			++nr_dirty;
> +			if (nr_dirty == ARRAY_SIZE(dirty)) {
> +				err = fat_sync_bhs(dirty, nr_dirty);
> +				fat_brelse_array(dirty, nr_dirty);
> +				nr_dirty = 0;
> +				if (err)
> +					goto error;
>  			}
> -			/* Avoid race with userspace read via bdev */
> -			lock_buffer(c_bh);
> -			memcpy(c_bh->b_data, bhs[n]->b_data, sb->s_blocksize);
> -			set_buffer_uptodate(c_bh);
> -			unlock_buffer(c_bh);
> -			mmb_mark_buffer_dirty(c_bh,
> -				&MSDOS_I(sbi->fat_inode)->i_metadata_bhs);
> -			if (sb->s_flags & SB_SYNCHRONOUS)
> -				err = sync_dirty_buffer(c_bh);
> +		} else {
>  			brelse(c_bh);
> -			if (err)
> -				goto error;
>  		}
>  	}
> +
>  error:
> +	if (nr_dirty) {
> +		int err2 = fat_sync_bhs(dirty, nr_dirty);
> +		if (!err)
> +			err = err2;
> +		fat_brelse_array(dirty, nr_dirty);
> +	}
> +
>  	return err;
>  }
>  
> +static int fat_mirror_bhs(struct super_block *sb, struct buffer_head **bhs,
> +			  int nr_bhs)
> +{
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +	int copy;
> +
> +	for (copy = 1; copy < sbi->fats; copy++) {
> +		int err = fat_mirror_one(sb, bhs, nr_bhs, copy);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
>  int fat_ent_write(struct inode *inode, struct fat_entry *fatent,
>  		  int new, int wait)
>  {
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index 0d04228f916e..b6a9e2e122cd 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
> @@ -365,3 +365,9 @@ int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs)
>  	}
>  	return err;
>  }
> +
> +void fat_brelse_array(struct buffer_head *bhs[], int n)
> +{
> +	while (--n >= 0)
> +		brelse(bhs[n]);
> +}

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2 2/2] fat: improve fat_sync_bhs() write submission

[OGAWA Hirofumi]

Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br> writes:

> fat_sync_bhs() does many individual write_dirty_buffer() calls without
> plugging, making batching in the block layer more difficult. It also
> does not pass the REQ_SYNC hint for these synchronous writes.
>
> Use a plug around the writes so the block layer can more easily batch
> them. Pass the REQ_SYNC hint to write_dirty_buffer().
>
> Benchmarking was done under QEMU/KVM with a virtio-blk device using
> cache=directsync. Each result is the arithmetic mean of 10 samples.
>
> start = now_ns();
> for (unsigned i = 0; i < count; i++) {
>     fallocate(fds[i], FALLOC_FL_KEEP_SIZE, 0, bytes);
> }
> elapsed = now_ns() - start;
>
> Sync-mounted FAT32 using count=16 files:
> File size    Cluster size    Before    After    Improvement
> 256MiB       512B            13.11s    11.87s   1.10x
> 256MiB       4096B           1.657s    1.469s   1.13x
>
> Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>

Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Thanks.

> ---
>  fs/fat/misc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index b6a9e2e122cd..e8e2f13254d5 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
> @@ -9,6 +9,7 @@
>  
>  #include "fat.h"
>  #include <linux/iversion.h>
> +#include <linux/blk_plug.h>
>  
>  /*
>   * fat_fs_error reports a file system problem that might indicate fa data
> @@ -354,9 +355,14 @@ EXPORT_SYMBOL_GPL(fat_update_time);
>  int fat_sync_bhs(struct buffer_head **bhs, int nr_bhs)
>  {
>  	int i, err = 0;
> +	struct blk_plug plug;
> +
> +	blk_start_plug(&plug);
>  
>  	for (i = 0; i < nr_bhs; i++)
> -		write_dirty_buffer(bhs[i], 0);
> +		write_dirty_buffer(bhs[i], REQ_SYNC);
> +
> +	blk_finish_plug(&plug);
>  
>  	for (i = 0; i < nr_bhs; i++) {
>  		wait_on_buffer(bhs[i]);

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>