In the append-write scenario, after ensuring that the dirty inode can be
seen by the writeback process, there is no need to execute
"mark_inode_dirty" for every write. Instead, we can rely on
"ext4_mark_inode_dirty" executed when updating i_disksize in
"mpage_map_and_submit_extent" to ensure data consistency, which can
significantly improve performance in high-frequency append-write
scenarios.

In test scenarios of Kafka version 2.6.2, using packet size of 2K
resulted in a 10% performance improvement.

Signed-off-by: Liu Song <liusong@linux.alibaba.com>
---
 fs/ext4/inode.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9d9f414f99fe..d1aa775c9936 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3128,6 +3128,57 @@ static int ext4_da_should_update_i_disksize(struct page *page,
 	return 1;
 }
 
+/*
+ * Copy from generic_write_end, add conditions to execute mark_inode_dirty
+ * to avoid additional overhead caused by frequent dirty inode operations
+ */
+static int ext4_da_generic_write_end(struct file *file, struct address_space *mapping,
+			loff_t pos, unsigned len, unsigned copied, bool need_dirty,
+			struct page *page, void *fsdata)
+{
+	struct inode *inode = mapping->host;
+	loff_t old_size = inode->i_size;
+	bool i_size_changed = false;
+	int was_dirty;
+
+	copied = block_write_end(file, mapping, pos, len, copied, page, fsdata);
+
+	/*
+	 * No need to use i_size_read() here, the i_size cannot change under us
+	 * because we hold i_rwsem.
+	 *
+	 * But it's important to update i_size while still holding page lock:
+	 * page writeout could otherwise come in and zero beyond i_size.
+	 */
+	if (pos + copied > inode->i_size) {
+		i_size_write(inode, pos + copied);
+		i_size_changed = true;
+	}
+
+	unlock_page(page);
+	put_page(page);
+
+	if (old_size < pos)
+		pagecache_isize_extended(inode, old_size, pos);
+
+	/*
+	 * In the append-write scenario, if the inode is marked as dirty,
+	 * it is ensured that the inode will be seen by the writeback process.
+	 * In the ext4_writepages process, when updating i_disksize,
+	 * corresponding metadata updates are also performed.
+	 * Therefore, it is unnecessary to repeatedly execute mark_inode_dirty
+	 * to improve performance.
+	 */
+	if (i_size_changed) {
+		spin_lock(&inode->i_lock);
+		was_dirty = inode->i_state & I_DIRTY;
+		spin_unlock(&inode->i_lock);
+		if (!was_dirty || need_dirty)
+			mark_inode_dirty(inode);
+	}
+	return copied;
+}
+
 static int ext4_da_write_end(struct file *file,
 			     struct address_space *mapping,
 			     loff_t pos, unsigned len, unsigned copied,
@@ -3137,6 +3188,7 @@ static int ext4_da_write_end(struct file *file,
 	loff_t new_i_size;
 	unsigned long start, end;
 	int write_mode = (int)(unsigned long)fsdata;
+	bool need_dirty = false;
 
 	if (write_mode == FALL_BACK_TO_NONDELALLOC)
 		return ext4_write_end(file, mapping, pos,
@@ -3169,10 +3221,12 @@ static int ext4_da_write_end(struct file *file,
 	 */
 	new_i_size = pos + copied;
 	if (copied && new_i_size > inode->i_size &&
-	    ext4_da_should_update_i_disksize(page, end))
+	    ext4_da_should_update_i_disksize(page, end)) {
 		ext4_update_i_disksize(inode, new_i_size);
+		need_dirty = true;
+	}
 
-	return generic_write_end(file, mapping, pos, len, copied, page, fsdata);
+	return ext4_da_generic_write_end(file, mapping, pos, len, copied, need_dirty, page, fsdata);
 }
 
 /*
-- 
2.19.1.6.gb485710b

On Tue 11-07-23 11:42:56, Liu Song wrote:
> In the append-write scenario, after ensuring that the dirty inode can be
> seen by the writeback process, there is no need to execute
> "mark_inode_dirty" for every write. Instead, we can rely on
> "ext4_mark_inode_dirty" executed when updating i_disksize in
> "mpage_map_and_submit_extent" to ensure data consistency, which can
> significantly improve performance in high-frequency append-write
> scenarios.
> 
> In test scenarios of Kafka version 2.6.2, using packet size of 2K
> resulted in a 10% performance improvement.
> 
> Signed-off-by: Liu Song <liusong@linux.alibaba.com>

Overall the benefit looks interesting. Nice work.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 9d9f414f99fe..d1aa775c9936 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3128,6 +3128,57 @@ static int ext4_da_should_update_i_disksize(struct page *page,
>  	return 1;
>  }
>  
> +/*
> + * Copy from generic_write_end, add conditions to execute mark_inode_dirty
> + * to avoid additional overhead caused by frequent dirty inode operations
> + */
> +static int ext4_da_generic_write_end(struct file *file, struct address_space *mapping,
> +			loff_t pos, unsigned len, unsigned copied, bool need_dirty,
> +			struct page *page, void *fsdata)
> +{
> +	struct inode *inode = mapping->host;
> +	loff_t old_size = inode->i_size;
> +	bool i_size_changed = false;
> +	int was_dirty;
> +
> +	copied = block_write_end(file, mapping, pos, len, copied, page, fsdata);
> +
> +	/*
> +	 * No need to use i_size_read() here, the i_size cannot change under us
> +	 * because we hold i_rwsem.
> +	 *
> +	 * But it's important to update i_size while still holding page lock:
> +	 * page writeout could otherwise come in and zero beyond i_size.
> +	 */
> +	if (pos + copied > inode->i_size) {
> +		i_size_write(inode, pos + copied);
> +		i_size_changed = true;
> +	}
> +
> +	unlock_page(page);
> +	put_page(page);
> +
> +	if (old_size < pos)
> +		pagecache_isize_extended(inode, old_size, pos);
> +

I dislike the duplication of generic_write_end() but exporting a variant of
generic_write_end() not doing the dirtying (and additionally returning the
i_size_changed value) doesn't look appealing either. So I guess I'll just
live with this...

> +	/*
> +	 * In the append-write scenario, if the inode is marked as dirty,
> +	 * it is ensured that the inode will be seen by the writeback process.
> +	 * In the ext4_writepages process, when updating i_disksize,
> +	 * corresponding metadata updates are also performed.
> +	 * Therefore, it is unnecessary to repeatedly execute mark_inode_dirty
> +	 * to improve performance.
> +	 */

Note that block_write_end() will mark the inode as dirty with I_DIRTY_PAGES
flag (which all that's needed to trigger page writeback). Since i_size is
never really written to disk by ext4, there's simply never a need to mark
inode dirty because of that. Also if inode timestamps were updated inode
was already properly marked dirty. Hence we can just mostly drop the code
below.

> +	if (i_size_changed) {
> +		spin_lock(&inode->i_lock);
> +		was_dirty = inode->i_state & I_DIRTY;
> +		spin_unlock(&inode->i_lock);
> +		if (!was_dirty || need_dirty)
> +			mark_inode_dirty(inode);
> +	}
> +	return copied;
> +}
> +
>  static int ext4_da_write_end(struct file *file,
>  			     struct address_space *mapping,
>  			     loff_t pos, unsigned len, unsigned copied,
> @@ -3137,6 +3188,7 @@ static int ext4_da_write_end(struct file *file,
>  	loff_t new_i_size;
>  	unsigned long start, end;
>  	int write_mode = (int)(unsigned long)fsdata;
> +	bool need_dirty = false;
>  
>  	if (write_mode == FALL_BACK_TO_NONDELALLOC)
>  		return ext4_write_end(file, mapping, pos,
> @@ -3169,10 +3221,12 @@ static int ext4_da_write_end(struct file *file,
>  	 */
>  	new_i_size = pos + copied;
>  	if (copied && new_i_size > inode->i_size &&
> -	    ext4_da_should_update_i_disksize(page, end))
> +	    ext4_da_should_update_i_disksize(page, end)) {
>  		ext4_update_i_disksize(inode, new_i_size);
> +		need_dirty = true;
> +	}

So when we create our own new helper function anyway I'd just move all this
logic for delalloc write end there. Something like:

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;

	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:
	 * 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 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.
	 */
	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(folio, 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;
}


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

On 2023/8/9 20:25, Jan Kara wrote:
> On Tue 11-07-23 11:42:56, Liu Song wrote:
>> In the append-write scenario, after ensuring that the dirty inode can be
>> seen by the writeback process, there is no need to execute
>> "mark_inode_dirty" for every write. Instead, we can rely on
>> "ext4_mark_inode_dirty" executed when updating i_disksize in
>> "mpage_map_and_submit_extent" to ensure data consistency, which can
>> significantly improve performance in high-frequency append-write
>> scenarios.
>>
>> In test scenarios of Kafka version 2.6.2, using packet size of 2K
>> resulted in a 10% performance improvement.
>>
>> Signed-off-by: Liu Song <liusong@linux.alibaba.com>
> Overall the benefit looks interesting. Nice work.
>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 9d9f414f99fe..d1aa775c9936 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -3128,6 +3128,57 @@ static int ext4_da_should_update_i_disksize(struct page *page,
>>   	return 1;
>>   }
>>   
>> +/*
>> + * Copy from generic_write_end, add conditions to execute mark_inode_dirty
>> + * to avoid additional overhead caused by frequent dirty inode operations
>> + */
>> +static int ext4_da_generic_write_end(struct file *file, struct address_space *mapping,
>> +			loff_t pos, unsigned len, unsigned copied, bool need_dirty,
>> +			struct page *page, void *fsdata)
>> +{
>> +	struct inode *inode = mapping->host;
>> +	loff_t old_size = inode->i_size;
>> +	bool i_size_changed = false;
>> +	int was_dirty;
>> +
>> +	copied = block_write_end(file, mapping, pos, len, copied, page, fsdata);
>> +
>> +	/*
>> +	 * No need to use i_size_read() here, the i_size cannot change under us
>> +	 * because we hold i_rwsem.
>> +	 *
>> +	 * But it's important to update i_size while still holding page lock:
>> +	 * page writeout could otherwise come in and zero beyond i_size.
>> +	 */
>> +	if (pos + copied > inode->i_size) {
>> +		i_size_write(inode, pos + copied);
>> +		i_size_changed = true;
>> +	}
>> +
>> +	unlock_page(page);
>> +	put_page(page);
>> +
>> +	if (old_size < pos)
>> +		pagecache_isize_extended(inode, old_size, pos);
>> +
> I dislike the duplication of generic_write_end() but exporting a variant of
> generic_write_end() not doing the dirtying (and additionally returning the
> i_size_changed value) doesn't look appealing either. So I guess I'll just
> live with this...
>
>> +	/*
>> +	 * In the append-write scenario, if the inode is marked as dirty,
>> +	 * it is ensured that the inode will be seen by the writeback process.
>> +	 * In the ext4_writepages process, when updating i_disksize,
>> +	 * corresponding metadata updates are also performed.
>> +	 * Therefore, it is unnecessary to repeatedly execute mark_inode_dirty
>> +	 * to improve performance.
>> +	 */
> Note that block_write_end() will mark the inode as dirty with I_DIRTY_PAGES
> flag (which all that's needed to trigger page writeback). Since i_size is
> never really written to disk by ext4, there's simply never a need to mark
> inode dirty because of that. Also if inode timestamps were updated inode
> was already properly marked dirty. Hence we can just mostly drop the code
> below.
>
>> +	if (i_size_changed) {
>> +		spin_lock(&inode->i_lock);
>> +		was_dirty = inode->i_state & I_DIRTY;
>> +		spin_unlock(&inode->i_lock);
>> +		if (!was_dirty || need_dirty)
>> +			mark_inode_dirty(inode);
>> +	}
>> +	return copied;
>> +}
>> +
>>   static int ext4_da_write_end(struct file *file,
>>   			     struct address_space *mapping,
>>   			     loff_t pos, unsigned len, unsigned copied,
>> @@ -3137,6 +3188,7 @@ static int ext4_da_write_end(struct file *file,
>>   	loff_t new_i_size;
>>   	unsigned long start, end;
>>   	int write_mode = (int)(unsigned long)fsdata;
>> +	bool need_dirty = false;
>>   
>>   	if (write_mode == FALL_BACK_TO_NONDELALLOC)
>>   		return ext4_write_end(file, mapping, pos,
>> @@ -3169,10 +3221,12 @@ static int ext4_da_write_end(struct file *file,
>>   	 */
>>   	new_i_size = pos + copied;
>>   	if (copied && new_i_size > inode->i_size &&
>> -	    ext4_da_should_update_i_disksize(page, end))
>> +	    ext4_da_should_update_i_disksize(page, end)) {
>>   		ext4_update_i_disksize(inode, new_i_size);
>> +		need_dirty = true;
>> +	}


Thank you very much for your response, and the implementation of 
"ext4_da_do_write_end" is great.
I will refer to your suggestions and send a V2 version of the patch.

Thanks


> So when we create our own new helper function anyway I'd just move all this
> logic for delalloc write end there. Something like:
>
> 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;
>
> 	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:
> 	 * 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 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.
> 	 */
> 	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(folio, 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;
> }
>
>
> 								Honza

Hi,

Any comments on this? Considering the most extreme scenario
where a file is appended with only one byte each time,
"jbd2_journal_dirty_metadata" will be executed because i_size
has changed. Therefore, it is better to update raw_inode's size
after the writeback is completed, which can also ensure the
consistency of metadata and data, and obtain better performance
benefits in high-frequency append write scenarios.

Thanks