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

[PATCH 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.

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 | 81 +++++++++++++++++++++++++++++++++++--------------
 fs/fat/misc.c   | 14 ++++++++-
 3 files changed, 72 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH 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 | 81 +++++++++++++++++++++++++++++++++++--------------
 fs/fat/misc.c   |  6 ++++
 3 files changed, 65 insertions(+), 23 deletions(-)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index b18385461de8..5086c35230ac 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..4fa1fae5abf9 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -376,39 +376,74 @@ 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, err2, 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) {
+		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 err, copy;
+
+	err = 0;
+	for (copy = 1; copy < sbi->fats; copy++) {
+		err = fat_mirror_one(sb, bhs, nr_bhs, copy);
+		if (err)
+			break;
+	}
+
 	return err;
 }
 
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index e79762cf1975..f8f89b947baf 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 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 f8f89b947baf..29ca983b917a 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 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>
> ---
>  fs/fat/fat.h    |  1 +
>  fs/fat/fatent.c | 81 +++++++++++++++++++++++++++++++++++--------------
>  fs/fat/misc.c   |  6 ++++
>  3 files changed, 65 insertions(+), 23 deletions(-)
>
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index b18385461de8..5086c35230ac 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..4fa1fae5abf9 100644
> --- a/fs/fat/fatent.c
> +++ b/fs/fat/fatent.c
> @@ -376,39 +376,74 @@ 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, err2, 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) {
> +		err2 = fat_sync_bhs(dirty, nr_dirty);

		int err2 = fat_sync_bhs(dirty, nr_dirty);

I'd like to see err2 as local.

> +		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 err, copy;
> +
> +	err = 0;
> +	for (copy = 1; copy < sbi->fats; copy++) {
> +		err = fat_mirror_one(sb, bhs, nr_bhs, copy);
> +		if (err)
> +			break;
> +	}
> +
>  	return err;

		int err = fat_mirror_one(sb, bhs, nr_bhs, copy);
		if (err)
			return err;
	}

	return 0;

very minor (either is ok actually)

>  }
>  
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index e79762cf1975..f8f89b947baf 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]);
> +}

I'd like this is noop if n == 0, possibly easier to use and safer in future.

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

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

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br> writes:
>
>> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
>> index e79762cf1975..f8f89b947baf 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]);
>> +}
>
> I'd like this is noop if n == 0, possibly easier to use and safer in future.

Ouch, it works as is.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>