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@xxxxxxxxxxxxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
