Re: [PATCH v2] fat: preallocate space before expanding in fat_cont_expand()

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

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

> The fat_cont_expand() implementation leaves allocation of new clusters
> to generic_cont_expand_simple(). This path does many small cluster
> allocations, harming performance.
>
> Preallocate cluster space in fat_cont_expand(). Move preallocation to
> a new fat_preallocate() helper used by both fat_cont_expand() and
> fat_fallocate(FALLOC_FL_KEEP_SIZE).
>
> Benchmarking was done under QEMU/KVM with a virtio-blk device using
> cache=none. Each result is the arithmetic mean of the listed samples.
>
> start = now_ns();
> for (unsigned i = 0; i < count; i++) {
>     if (truncate_mode)
>         ftruncate(fds[i], bytes);
>     else
>         fallocate(fds[i], 0, 0, bytes);
> }
> elapsed = now_ns() - start;
>
> 16 files, 512-byte clusters:
> length    mode      sync?    before    after    improvement    samples
> 256MiB    ftruncate no       9.576s    5.345s   1.79x          10
> 256MiB    fallocate no       9.634s    5.640s   1.71x          10
> 1MiB      ftruncate yes      58.66s    0.362s   162.15x        5
> 1MiB      fallocate yes      61.36s    0.390s   157.15x        5
>
> 128 files, 512-byte clusters:
> length    mode      sync?    before    after    improvement    samples
> 512KiB    ftruncate no       0.433s    0.352s   1.23x          10
> 512KiB    fallocate no       0.446s    0.393s   1.13x          10
> 64KiB     ftruncate yes      31.32s    0.429s   73.00x         10
> 64KiB     fallocate yes      29.59s    0.410s   72.42x         10
>
> 16 files, 4096-byte clusters:
> length    mode      sync?    before    after    improvement    samples
> 256MiB    ftruncate no       5.181s    4.417s   1.17x          10
> 256MiB    fallocate no       5.166s    4.442s   1.16x          10
> 1MiB      ftruncate yes      9.294s    0.111s   84.06x         10
> 1MiB      fallocate yes      8.710s    0.110s   79.36x         10
>
> The biggest improvement is on filesystems with a small cluster size and
> on synchronous mounts.
>
> Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>
> ---
>
> Changes in v2:
> - fat_cont_expand(): Hold truncate_lock around fat_truncate_blocks().
>
> This patch depends on:
> https://lore.kernel.org/linux-fsdevel/20261012171106.146243-1-matheus.aalmeida@inf.ufrgs.br/T/#t
>
>  fs/fat/file.c | 56 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index 3aa541c6b127..dd0ae795dc14 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -217,15 +217,50 @@ const struct file_operations fat_file_operations = {
>  	.setlease	= generic_setlease,
>  };
>  
> +static int fat_preallocate(struct inode *inode, loff_t new_size)
> +{
> +	int nr_cluster; /* Number of clusters to be allocated */
> +	loff_t mm_bytes; /* Number of bytes to be allocated for file */
> +	loff_t ondisksize; /* block aligned on-disk size in bytes*/
> +	struct super_block *sb = inode->i_sb;
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +
> +	ondisksize = inode->i_blocks << 9;
> +	if (new_size <= ondisksize)
> +		return 0;
> +
> +	/* First compute the number of clusters to be allocated */
> +	mm_bytes = new_size - ondisksize;
> +	nr_cluster = (mm_bytes + (sbi->cluster_size - 1)) >>
> +		     sbi->cluster_bits;
> +
> +	/* Allocate the clusters. We are not zeroing them out */
> +	return fat_add_clusters(inode, nr_cluster);
> +}
> +
>  static int fat_cont_expand(struct inode *inode, loff_t size)
>  {
>  	struct address_space *mapping = inode->i_mapping;
>  	loff_t start = inode->i_size, count = size - inode->i_size;
>  	int err;
>  
> -	err = generic_cont_expand_simple(inode, size);
> +	/*
> +	 * Preallocating space here is more efficient than during
> +	 * generic_cont_expand_simple() because of batching.
> +	 */
> +	err = inode_newsize_ok(inode, size);
>  	if (err)
>  		goto out;
> +	err = fat_preallocate(inode, size);
> +	if (err)
> +		goto out;
> +	err = generic_cont_expand_simple(inode, size);
> +	if (err) {
> +		down_write(&MSDOS_I(inode)->truncate_lock);
> +		fat_truncate_blocks(inode, inode->i_size);
> +		up_write(&MSDOS_I(inode)->truncate_lock);

Is this fat_truncate_blocks() really required?

> +		goto out;
> +	}
>  
>  	fat_truncate_time(inode, NULL, FAT_UPDATE_CMTIME);
>  	mark_inode_dirty(inode);
> @@ -263,12 +298,7 @@ static int fat_cont_expand(struct inode *inode, loff_t size)
>  static long fat_fallocate(struct file *file, int mode,
>  			  loff_t offset, loff_t len)
>  {
> -	int nr_cluster; /* Number of clusters to be allocated */
> -	loff_t mm_bytes; /* Number of bytes to be allocated for file */
> -	loff_t ondisksize; /* block aligned on-disk size in bytes*/
>  	struct inode *inode = file->f_mapping->host;
> -	struct super_block *sb = inode->i_sb;
> -	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>  	int err = 0;
>  
>  	/* No support for hole punch or other fallocate flags. */
> @@ -281,19 +311,7 @@ static long fat_fallocate(struct file *file, int mode,
>  
>  	inode_lock(inode);
>  	if (mode & FALLOC_FL_KEEP_SIZE) {
> -		ondisksize = inode->i_blocks << 9;
> -		if ((offset + len) <= ondisksize)
> -			goto error;
> -
> -		/* First compute the number of clusters to be allocated */
> -		mm_bytes = offset + len - ondisksize;
> -		nr_cluster = (mm_bytes + (sbi->cluster_size - 1)) >>
> -			sbi->cluster_bits;
> -
> -		/* Allocate the clusters. We are not zeroing them out */
> -		err = fat_add_clusters(inode, nr_cluster);
> -		if (err)
> -			goto error;
> +		err = fat_preallocate(inode, offset + len);
>  	} else {
>  		if ((offset + len) <= i_size_read(inode))
>  			goto error;

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