Re: [PATCH v2] ext4: do not mark inode dirty every time in delalloc append write scenario

[Jan Kara]

On Thu 10-08-23 23:43:33, Liu Song wrote:
> In the delalloc append write scenario, if inode's i_size is extended due
> to buffer write, there are delalloc writes pending in the range up to
> i_size, and no need to touch i_disksize since writeback will push
> i_disksize up to i_size eventually. Offers significant performance
> improvement in high-frequency append write scenarios.
> 
> I conducted tests in my 32-core environment by launching 32 concurrent
> threads to append write to the same file. Each write operation had a
> length of 1024 bytes and was repeated 100000 times. Without using this
> patch, the test was completed in 7705 ms. However, with this patch, the
> test was completed in 5066 ms, resulting in a performance improvement of
> 34%.
> 
> Moreover, in test scenarios of Kafka version 2.6.2, using packet size of
> 2K, with this patch resulted in a 10% performance improvement.
> 
> Signed-off-by: Liu Song <liusong@xxxxxxxxxxxxxxxxx>
> Suggested-by: Jan Kara <jack@xxxxxxx>

Looks good! Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/ext4/inode.c | 88 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 62 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 89737d5a1614..830b8e7e68cb 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2937,14 +2937,73 @@ static int ext4_da_should_update_i_disksize(struct folio *folio,
>  	return 1;
>  }
>  
> +static int ext4_da_do_write_end(struct address_space *mapping,
> +			loff_t pos, unsigned len, unsigned copied,
> +			struct page *page)
> +{
> +	struct inode *inode = mapping->host;
> +	loff_t old_size = inode->i_size;
> +	bool disksize_changed = false;
> +	loff_t new_i_size;
> +
> +	/*
> +	 * block_write_end() will mark the inode as dirty with I_DIRTY_PAGES
> +	 * flag, which all that's needed to trigger page writeback.
> +	 */
> +	copied = block_write_end(NULL, mapping, pos, len, copied, page, NULL);
> +	new_i_size = pos + copied;
> +
> +	/*
> +	 * It's important to update i_size while still holding page lock,
> +	 * because page writeout could otherwise come in and zero beyond
> +	 * i_size.
> +	 *
> +	 * Since we are holding inode lock, we are sure i_disksize <=
> +	 * i_size. We also know that if i_disksize < i_size, there are
> +	 * delalloc writes pending in the range up to i_size. If the end of
> +	 * the current write is <= i_size, there's no need to touch
> +	 * i_disksize since writeback will push i_disksize up to i_size
> +	 * eventually. If the end of the current write is > i_size and
> +	 * inside an allocated block which ext4_da_should_update_i_disksize()
> +	 * checked, we need to update i_disksize here as certain
> +	 * ext4_writepages() paths not allocating blocks and update i_disksize.
> +	 */
> +	if (new_i_size > inode->i_size) {
> +		unsigned long end;
> +
> +		i_size_write(inode, new_i_size);
> +		end = (new_i_size - 1) & (PAGE_SIZE - 1);
> +		if (copied && ext4_da_should_update_i_disksize(page_folio(page), end)) {
> +			ext4_update_i_disksize(inode, new_i_size);
> +			disksize_changed = true;
> +		}
> +	}
> +
> +	unlock_page(page);
> +	put_page(page);
> +
> +	if (old_size < pos)
> +		pagecache_isize_extended(inode, old_size, pos);
> +
> +	if (disksize_changed) {
> +		handle_t *handle;
> +
> +		handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
> +		if (IS_ERR(handle))
> +			return PTR_ERR(handle);
> +		ext4_mark_inode_dirty(handle, inode);
> +		ext4_journal_stop(handle);
> +	}
> +
> +	return copied;
> +}
> +
>  static int ext4_da_write_end(struct file *file,
>  			     struct address_space *mapping,
>  			     loff_t pos, unsigned len, unsigned copied,
>  			     struct page *page, void *fsdata)
>  {
>  	struct inode *inode = mapping->host;
> -	loff_t new_i_size;
> -	unsigned long start, end;
>  	int write_mode = (int)(unsigned long)fsdata;
>  	struct folio *folio = page_folio(page);
>  
> @@ -2963,30 +3022,7 @@ static int ext4_da_write_end(struct file *file,
>  	if (unlikely(copied < len) && !PageUptodate(page))
>  		copied = 0;
>  
> -	start = pos & (PAGE_SIZE - 1);
> -	end = start + copied - 1;
> -
> -	/*
> -	 * Since we are holding inode lock, we are sure i_disksize <=
> -	 * i_size. We also know that if i_disksize < i_size, there are
> -	 * delalloc writes pending in the range upto i_size. If the end of
> -	 * the current write is <= i_size, there's no need to touch
> -	 * i_disksize since writeback will push i_disksize upto i_size
> -	 * eventually. If the end of the current write is > i_size and
> -	 * inside an allocated block (ext4_da_should_update_i_disksize()
> -	 * check), we need to update i_disksize here as certain
> -	 * ext4_writepages() paths not allocating blocks update i_disksize.
> -	 *
> -	 * Note that we defer inode dirtying to generic_write_end() /
> -	 * ext4_da_write_inline_data_end().
> -	 */
> -	new_i_size = pos + copied;
> -	if (copied && new_i_size > inode->i_size &&
> -	    ext4_da_should_update_i_disksize(folio, end))
> -		ext4_update_i_disksize(inode, new_i_size);
> -
> -	return generic_write_end(file, mapping, pos, len, copied, &folio->page,
> -				 fsdata);
> +	return ext4_da_do_write_end(mapping, pos, len, copied, &folio->page);
>  }
>  
>  /*
> -- 
> 2.19.1.6.gb485710b
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR