| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z2Kh9SxYfkIv6NYJ@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Date: Wed, 18 Dec 2024 15:50:37 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: Zhang Yi <yi.zhang@...weicloud.com>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
jack@...e.cz, yi.zhang@...wei.com, chengzhihao1@...wei.com,
yukuai3@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH v4 10/10] ext4: move out common parts into
ext4_fallocate()
On Mon, Dec 16, 2024 at 09:39:15AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@...wei.com>
>
> Currently, all zeroing ranges, punch holes, collapse ranges, and insert
> ranges first wait for all existing direct I/O workers to complete, and
> then they acquire the mapping's invalidate lock before performing the
> actual work. These common components are nearly identical, so we can
> simplify the code by factoring them out into the ext4_fallocate().
>
I really like the long due cleanup of fallocate paths. Thanks for
taking it up! Feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> Reviewed-by: Jan Kara <jack@...e.cz>
> ---
> fs/ext4/extents.c | 124 ++++++++++++++++------------------------------
> fs/ext4/inode.c | 25 ++--------
> 2 files changed, 45 insertions(+), 104 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 85f0de1abe78..1b028be19193 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4568,7 +4568,6 @@ static long ext4_zero_range(struct file *file, loff_t offset,
> loff_t len, int mode)
> {
> struct inode *inode = file_inode(file);
> - struct address_space *mapping = file->f_mapping;
> handle_t *handle = NULL;
> loff_t new_size = 0;
> loff_t end = offset + len;
> @@ -4592,23 +4591,6 @@ static long ext4_zero_range(struct file *file, loff_t offset,
> return ret;
> }
>
> - /* Wait all existing dio workers, newcomers will block on i_rwsem */
> - inode_dio_wait(inode);
> -
> - ret = file_modified(file);
> - if (ret)
> - return ret;
> -
> - /*
> - * Prevent page faults from reinstantiating pages we have released
> - * from page cache.
> - */
> - filemap_invalidate_lock(mapping);
> -
> - ret = ext4_break_layouts(inode);
> - if (ret)
> - goto out_invalidate_lock;
> -
> flags = EXT4_GET_BLOCKS_CREATE_UNWRIT_EXT;
> /* Preallocate the range including the unaligned edges */
> if (!IS_ALIGNED(offset | end, blocksize)) {
> @@ -4618,17 +4600,17 @@ static long ext4_zero_range(struct file *file, loff_t offset,
> ret = ext4_alloc_file_blocks(file, alloc_lblk, len_lblk,
> new_size, flags);
> if (ret)
> - goto out_invalidate_lock;
> + return ret;
> }
>
> ret = ext4_update_disksize_before_punch(inode, offset, len);
> if (ret)
> - goto out_invalidate_lock;
> + return ret;
>
> /* Now release the pages and zero block aligned part of pages */
> ret = ext4_truncate_page_cache_block_range(inode, offset, end);
> if (ret)
> - goto out_invalidate_lock;
> + return ret;
>
> /* Zero range excluding the unaligned edges */
> start_lblk = EXT4_B_TO_LBLK(inode, offset);
> @@ -4640,11 +4622,11 @@ static long ext4_zero_range(struct file *file, loff_t offset,
> ret = ext4_alloc_file_blocks(file, start_lblk, zero_blks,
> new_size, flags);
> if (ret)
> - goto out_invalidate_lock;
> + return ret;
> }
> /* Finish zeroing out if it doesn't contain partial block */
> if (IS_ALIGNED(offset | end, blocksize))
> - goto out_invalidate_lock;
> + return ret;
>
> /*
> * In worst case we have to writeout two nonadjacent unwritten
> @@ -4657,7 +4639,7 @@ static long ext4_zero_range(struct file *file, loff_t offset,
> if (IS_ERR(handle)) {
> ret = PTR_ERR(handle);
> ext4_std_error(inode->i_sb, ret);
> - goto out_invalidate_lock;
> + return ret;
> }
>
> /* Zero out partial block at the edges of the range */
> @@ -4677,8 +4659,6 @@ static long ext4_zero_range(struct file *file, loff_t offset,
>
> out_handle:
> ext4_journal_stop(handle);
> -out_invalidate_lock:
> - filemap_invalidate_unlock(mapping);
> return ret;
> }
>
> @@ -4711,13 +4691,6 @@ static long ext4_do_fallocate(struct file *file, loff_t offset,
> goto out;
> }
>
> - /* Wait all existing dio workers, newcomers will block on i_rwsem */
> - inode_dio_wait(inode);
> -
> - ret = file_modified(file);
> - if (ret)
> - goto out;
> -
> ret = ext4_alloc_file_blocks(file, start_lblk, len_lblk, new_size,
> EXT4_GET_BLOCKS_CREATE_UNWRIT_EXT);
> if (ret)
> @@ -4742,6 +4715,7 @@ static long ext4_do_fallocate(struct file *file, loff_t offset,
> long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> {
> struct inode *inode = file_inode(file);
> + struct address_space *mapping = file->f_mapping;
> int ret;
>
> /*
> @@ -4765,6 +4739,29 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> if (ret)
> goto out_inode_lock;
>
> + /* Wait all existing dio workers, newcomers will block on i_rwsem */
> + inode_dio_wait(inode);
> +
> + ret = file_modified(file);
> + if (ret)
> + return ret;
> +
> + if ((mode & FALLOC_FL_MODE_MASK) == FALLOC_FL_ALLOCATE_RANGE) {
> + ret = ext4_do_fallocate(file, offset, len, mode);
> + goto out_inode_lock;
> + }
> +
> + /*
> + * Follow-up operations will drop page cache, hold invalidate lock
> + * to prevent page faults from reinstantiating pages we have
> + * released from page cache.
> + */
> + filemap_invalidate_lock(mapping);
> +
> + ret = ext4_break_layouts(inode);
> + if (ret)
> + goto out_invalidate_lock;
> +
> if (mode & FALLOC_FL_PUNCH_HOLE)
> ret = ext4_punch_hole(file, offset, len);
> else if (mode & FALLOC_FL_COLLAPSE_RANGE)
> @@ -4774,7 +4771,10 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> else if (mode & FALLOC_FL_ZERO_RANGE)
> ret = ext4_zero_range(file, offset, len, mode);
> else
> - ret = ext4_do_fallocate(file, offset, len, mode);
> + ret = -EOPNOTSUPP;
> +
> +out_invalidate_lock:
> + filemap_invalidate_unlock(mapping);
> out_inode_lock:
> inode_unlock(inode);
> return ret;
> @@ -5301,23 +5301,6 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
> if (end >= inode->i_size)
> return -EINVAL;
>
> - /* Wait for existing dio to complete */
> - inode_dio_wait(inode);
> -
> - ret = file_modified(file);
> - if (ret)
> - return ret;
> -
> - /*
> - * Prevent page faults from reinstantiating pages we have released from
> - * page cache.
> - */
> - filemap_invalidate_lock(mapping);
> -
> - ret = ext4_break_layouts(inode);
> - if (ret)
> - goto out_invalidate_lock;
> -
> /*
> * Write tail of the last page before removed range and data that
> * will be shifted since they will get removed from the page cache
> @@ -5331,16 +5314,15 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
> if (!ret)
> ret = filemap_write_and_wait_range(mapping, end, LLONG_MAX);
> if (ret)
> - goto out_invalidate_lock;
> + return ret;
>
> truncate_pagecache(inode, start);
>
> credits = ext4_writepage_trans_blocks(inode);
> handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
> - if (IS_ERR(handle)) {
> - ret = PTR_ERR(handle);
> - goto out_invalidate_lock;
> - }
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> +
> ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_FALLOC_RANGE, handle);
>
> start_lblk = offset >> inode->i_blkbits;
> @@ -5379,8 +5361,6 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
>
> out_handle:
> ext4_journal_stop(handle);
> -out_invalidate_lock:
> - filemap_invalidate_unlock(mapping);
> return ret;
> }
>
> @@ -5421,23 +5401,6 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
> if (len > inode->i_sb->s_maxbytes - inode->i_size)
> return -EFBIG;
>
> - /* Wait for existing dio to complete */
> - inode_dio_wait(inode);
> -
> - ret = file_modified(file);
> - if (ret)
> - return ret;
> -
> - /*
> - * Prevent page faults from reinstantiating pages we have released from
> - * page cache.
> - */
> - filemap_invalidate_lock(mapping);
> -
> - ret = ext4_break_layouts(inode);
> - if (ret)
> - goto out_invalidate_lock;
> -
> /*
> * Write out all dirty pages. Need to round down to align start offset
> * to page size boundary for page size > block size.
> @@ -5445,16 +5408,15 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
> start = round_down(offset, PAGE_SIZE);
> ret = filemap_write_and_wait_range(mapping, start, LLONG_MAX);
> if (ret)
> - goto out_invalidate_lock;
> + return ret;
>
> truncate_pagecache(inode, start);
>
> credits = ext4_writepage_trans_blocks(inode);
> handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
> - if (IS_ERR(handle)) {
> - ret = PTR_ERR(handle);
> - goto out_invalidate_lock;
> - }
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> +
> ext4_fc_mark_ineligible(sb, EXT4_FC_REASON_FALLOC_RANGE, handle);
>
> /* Expand file to avoid data loss if there is error while shifting */
> @@ -5525,8 +5487,6 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
>
> out_handle:
> ext4_journal_stop(handle);
> -out_invalidate_lock:
> - filemap_invalidate_unlock(mapping);
> return ret;
> }
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 2e1f070ab449..2677a2cace58 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4009,7 +4009,6 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
> struct inode *inode = file_inode(file);
> struct super_block *sb = inode->i_sb;
> ext4_lblk_t start_lblk, end_lblk;
> - struct address_space *mapping = inode->i_mapping;
> loff_t max_end = EXT4_SB(sb)->s_bitmap_maxbytes - sb->s_blocksize;
> loff_t end = offset + length;
> handle_t *handle;
> @@ -4044,31 +4043,15 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
> return ret;
> }
>
> - /* Wait all existing dio workers, newcomers will block on i_rwsem */
> - inode_dio_wait(inode);
> -
> - ret = file_modified(file);
> - if (ret)
> - return ret;
> -
> - /*
> - * Prevent page faults from reinstantiating pages we have released from
> - * page cache.
> - */
> - filemap_invalidate_lock(mapping);
> -
> - ret = ext4_break_layouts(inode);
> - if (ret)
> - goto out_invalidate_lock;
>
> ret = ext4_update_disksize_before_punch(inode, offset, length);
> if (ret)
> - goto out_invalidate_lock;
> + return ret;
>
> /* Now release the pages and zero block aligned part of pages*/
> ret = ext4_truncate_page_cache_block_range(inode, offset, end);
> if (ret)
> - goto out_invalidate_lock;
> + return ret;
>
> if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> credits = ext4_writepage_trans_blocks(inode);
> @@ -4078,7 +4061,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
> if (IS_ERR(handle)) {
> ret = PTR_ERR(handle);
> ext4_std_error(sb, ret);
> - goto out_invalidate_lock;
> + return ret;
> }
>
> ret = ext4_zero_partial_blocks(handle, inode, offset, length);
> @@ -4123,8 +4106,6 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
> ext4_handle_sync(handle);
> out_handle:
> ext4_journal_stop(handle);
> -out_invalidate_lock:
> - filemap_invalidate_unlock(mapping);
> return ret;
> }
>
> --
> 2.46.1
>
Powered by blists - more mailing lists