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

[OGAWA Hirofumi]

Matheus Alves de Almeida <matheus.aalmeida@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxx>