Message ID | 20241022111059.2566137-10-yi.zhang@huaweicloud.com |
---|---|
State | New |
Headers | show |
Series | ext4: use iomap for regular file's buffered I/O path and enable large folio | expand |
On Tue 22-10-24 19:10:40, Zhang Yi wrote: > From: Zhang Yi <yi.zhang@huawei.com> > > Currently, all five sub-functions of ext4_fallocate() acquire the > inode's i_rwsem at the beginning and release it before exiting. This > process can be simplified by factoring out the management of i_rwsem > into the ext4_fallocate() function. > > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> Ah, nice. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> and please ignore my comments about renaming 'out' labels :). Honza > --- > fs/ext4/extents.c | 90 +++++++++++++++-------------------------------- > fs/ext4/inode.c | 13 +++---- > 2 files changed, 33 insertions(+), 70 deletions(-) > > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 2f727104f53d..a2db4e85790f 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -4573,23 +4573,18 @@ static long ext4_zero_range(struct file *file, loff_t offset, > int ret, flags, credits; > > trace_ext4_zero_range(inode, offset, len, mode); > + WARN_ON_ONCE(!inode_is_locked(inode)); > > - inode_lock(inode); > - > - /* > - * Indirect files do not support unwritten extents > - */ > - if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) { > - ret = -EOPNOTSUPP; > - goto out; > - } > + /* Indirect files do not support unwritten extents */ > + if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) > + return -EOPNOTSUPP; > > if (!(mode & FALLOC_FL_KEEP_SIZE) && > (end > inode->i_size || end > EXT4_I(inode)->i_disksize)) { > new_size = end; > ret = inode_newsize_ok(inode, new_size); > if (ret) > - goto out; > + return ret; > } > > /* Wait all existing dio workers, newcomers will block on i_rwsem */ > @@ -4597,7 +4592,7 @@ static long ext4_zero_range(struct file *file, loff_t offset, > > ret = file_modified(file); > if (ret) > - goto out; > + return ret; > > /* > * Prevent page faults from reinstantiating pages we have released > @@ -4687,8 +4682,6 @@ static long ext4_zero_range(struct file *file, loff_t offset, > ext4_journal_stop(handle); > out_invalidate_lock: > filemap_invalidate_unlock(mapping); > -out: > - inode_unlock(inode); > return ret; > } > > @@ -4702,12 +4695,11 @@ static long ext4_do_fallocate(struct file *file, loff_t offset, > int ret; > > trace_ext4_fallocate_enter(inode, offset, len, mode); > + WARN_ON_ONCE(!inode_is_locked(inode)); > > start_lblk = offset >> inode->i_blkbits; > len_lblk = EXT4_MAX_BLOCKS(len, offset, inode->i_blkbits); > > - inode_lock(inode); > - > /* We only support preallocation for extent-based files only. */ > if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) { > ret = -EOPNOTSUPP; > @@ -4739,7 +4731,6 @@ static long ext4_do_fallocate(struct file *file, loff_t offset, > EXT4_I(inode)->i_sync_tid); > } > out: > - inode_unlock(inode); > trace_ext4_fallocate_exit(inode, offset, len_lblk, ret); > return ret; > } > @@ -4774,9 +4765,8 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > > inode_lock(inode); > ret = ext4_convert_inline_data(inode); > - inode_unlock(inode); > if (ret) > - return ret; > + goto out; > > if (mode & FALLOC_FL_PUNCH_HOLE) > ret = ext4_punch_hole(file, offset, len); > @@ -4788,7 +4778,8 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > ret = ext4_zero_range(file, offset, len, mode); > else > ret = ext4_do_fallocate(file, offset, len, mode); > - > +out: > + inode_unlock(inode); > return ret; > } > > @@ -5298,36 +5289,27 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len) > int ret; > > trace_ext4_collapse_range(inode, offset, len); > - > - inode_lock(inode); > + WARN_ON_ONCE(!inode_is_locked(inode)); > > /* Currently just for extent based files */ > - if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) { > - ret = -EOPNOTSUPP; > - goto out; > - } > - > + if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) > + return -EOPNOTSUPP; > /* Collapse range works only on fs cluster size aligned regions. */ > - if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) { > - ret = -EINVAL; > - goto out; > - } > - > + if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) > + return -EINVAL; > /* > * There is no need to overlap collapse range with EOF, in which case > * it is effectively a truncate operation > */ > - if (end >= inode->i_size) { > - ret = -EINVAL; > - goto out; > - } > + if (end >= inode->i_size) > + return -EINVAL; > > /* Wait for existing dio to complete */ > inode_dio_wait(inode); > > ret = file_modified(file); > if (ret) > - goto out; > + return ret; > > /* > * Prevent page faults from reinstantiating pages we have released from > @@ -5402,8 +5384,6 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len) > ext4_journal_stop(handle); > out_invalidate_lock: > filemap_invalidate_unlock(mapping); > -out: > - inode_unlock(inode); > return ret; > } > > @@ -5429,39 +5409,27 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len) > loff_t start; > > trace_ext4_insert_range(inode, offset, len); > - > - inode_lock(inode); > + WARN_ON_ONCE(!inode_is_locked(inode)); > > /* Currently just for extent based files */ > - if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) { > - ret = -EOPNOTSUPP; > - goto out; > - } > - > + if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) > + return -EOPNOTSUPP; > /* Insert range works only on fs cluster size aligned regions. */ > - if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) { > - ret = -EINVAL; > - goto out; > - } > - > + if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) > + return -EINVAL; > /* Offset must be less than i_size */ > - if (offset >= inode->i_size) { > - ret = -EINVAL; > - goto out; > - } > - > + if (offset >= inode->i_size) > + return -EINVAL; > /* Check whether the maximum file size would be exceeded */ > - if (len > inode->i_sb->s_maxbytes - inode->i_size) { > - ret = -EFBIG; > - goto out; > - } > + 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) > - goto out; > + return ret; > > /* > * Prevent page faults from reinstantiating pages we have released from > @@ -5562,8 +5530,6 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len) > ext4_journal_stop(handle); > out_invalidate_lock: > filemap_invalidate_unlock(mapping); > -out: > - inode_unlock(inode); > return ret; > } > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 1d128333bd06..bea19cd6e676 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3962,15 +3962,14 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) > unsigned long blocksize = i_blocksize(inode); > handle_t *handle; > unsigned int credits; > - int ret = 0; > + int ret; > > trace_ext4_punch_hole(inode, offset, length, 0); > - > - inode_lock(inode); > + WARN_ON_ONCE(!inode_is_locked(inode)); > > /* No need to punch hole beyond i_size */ > if (offset >= inode->i_size) > - goto out; > + return 0; > > /* > * If the hole extends beyond i_size, set the hole to end after > @@ -3990,7 +3989,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) > if (offset & (blocksize - 1) || end & (blocksize - 1)) { > ret = ext4_inode_attach_jinode(inode); > if (ret < 0) > - goto out; > + return ret; > } > > /* Wait all existing dio workers, newcomers will block on i_rwsem */ > @@ -3998,7 +3997,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) > > ret = file_modified(file); > if (ret) > - goto out; > + return ret; > > /* > * Prevent page faults from reinstantiating pages we have released from > @@ -4082,8 +4081,6 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) > ext4_journal_stop(handle); > out_invalidate_lock: > filemap_invalidate_unlock(mapping); > -out: > - inode_unlock(inode); > return ret; > } > > -- > 2.46.1 >
On 2024/12/4 20:05, Jan Kara wrote: > On Tue 22-10-24 19:10:40, Zhang Yi wrote: >> From: Zhang Yi <yi.zhang@huawei.com> >> >> Currently, all five sub-functions of ext4_fallocate() acquire the >> inode's i_rwsem at the beginning and release it before exiting. This >> process can be simplified by factoring out the management of i_rwsem >> into the ext4_fallocate() function. >> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > > Ah, nice. Feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > and please ignore my comments about renaming 'out' labels :). > > Honza > ... >> @@ -4774,9 +4765,8 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len) >> >> inode_lock(inode); >> ret = ext4_convert_inline_data(inode); >> - inode_unlock(inode); >> if (ret) >> - return ret; >> + goto out; >> >> if (mode & FALLOC_FL_PUNCH_HOLE) >> ret = ext4_punch_hole(file, offset, len); >> @@ -4788,7 +4778,8 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len) >> ret = ext4_zero_range(file, offset, len, mode); >> else >> ret = ext4_do_fallocate(file, offset, len, mode); >> - >> +out: >> + inode_unlock(inode); >> return ret; >> } >> I guess you may want to suggest rename this out to out_inode_lock as well. Thanks, Yi.
On Fri 06-12-24 16:13:14, Zhang Yi wrote: > On 2024/12/4 20:05, Jan Kara wrote: > > On Tue 22-10-24 19:10:40, Zhang Yi wrote: > >> From: Zhang Yi <yi.zhang@huawei.com> > >> > >> Currently, all five sub-functions of ext4_fallocate() acquire the > >> inode's i_rwsem at the beginning and release it before exiting. This > >> process can be simplified by factoring out the management of i_rwsem > >> into the ext4_fallocate() function. > >> > >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > > > > Ah, nice. Feel free to add: > > > > Reviewed-by: Jan Kara <jack@suse.cz> > > > > and please ignore my comments about renaming 'out' labels :). > > > > Honza > > > > ... > > >> @@ -4774,9 +4765,8 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > >> > >> inode_lock(inode); > >> ret = ext4_convert_inline_data(inode); > >> - inode_unlock(inode); > >> if (ret) > >> - return ret; > >> + goto out; > >> > >> if (mode & FALLOC_FL_PUNCH_HOLE) > >> ret = ext4_punch_hole(file, offset, len); > >> @@ -4788,7 +4778,8 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len) > >> ret = ext4_zero_range(file, offset, len, mode); > >> else > >> ret = ext4_do_fallocate(file, offset, len, mode); > >> - > >> +out: > >> + inode_unlock(inode); > >> return ret; > >> } > >> > > I guess you may want to suggest rename this out to out_inode_lock as well. Right. This one should better be out_inode_lock. Honza
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index 2f727104f53d..a2db4e85790f 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c @@ -4573,23 +4573,18 @@ static long ext4_zero_range(struct file *file, loff_t offset, int ret, flags, credits; trace_ext4_zero_range(inode, offset, len, mode); + WARN_ON_ONCE(!inode_is_locked(inode)); - inode_lock(inode); - - /* - * Indirect files do not support unwritten extents - */ - if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) { - ret = -EOPNOTSUPP; - goto out; - } + /* Indirect files do not support unwritten extents */ + if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) + return -EOPNOTSUPP; if (!(mode & FALLOC_FL_KEEP_SIZE) && (end > inode->i_size || end > EXT4_I(inode)->i_disksize)) { new_size = end; ret = inode_newsize_ok(inode, new_size); if (ret) - goto out; + return ret; } /* Wait all existing dio workers, newcomers will block on i_rwsem */ @@ -4597,7 +4592,7 @@ static long ext4_zero_range(struct file *file, loff_t offset, ret = file_modified(file); if (ret) - goto out; + return ret; /* * Prevent page faults from reinstantiating pages we have released @@ -4687,8 +4682,6 @@ static long ext4_zero_range(struct file *file, loff_t offset, ext4_journal_stop(handle); out_invalidate_lock: filemap_invalidate_unlock(mapping); -out: - inode_unlock(inode); return ret; } @@ -4702,12 +4695,11 @@ static long ext4_do_fallocate(struct file *file, loff_t offset, int ret; trace_ext4_fallocate_enter(inode, offset, len, mode); + WARN_ON_ONCE(!inode_is_locked(inode)); start_lblk = offset >> inode->i_blkbits; len_lblk = EXT4_MAX_BLOCKS(len, offset, inode->i_blkbits); - inode_lock(inode); - /* We only support preallocation for extent-based files only. */ if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) { ret = -EOPNOTSUPP; @@ -4739,7 +4731,6 @@ static long ext4_do_fallocate(struct file *file, loff_t offset, EXT4_I(inode)->i_sync_tid); } out: - inode_unlock(inode); trace_ext4_fallocate_exit(inode, offset, len_lblk, ret); return ret; } @@ -4774,9 +4765,8 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len) inode_lock(inode); ret = ext4_convert_inline_data(inode); - inode_unlock(inode); if (ret) - return ret; + goto out; if (mode & FALLOC_FL_PUNCH_HOLE) ret = ext4_punch_hole(file, offset, len); @@ -4788,7 +4778,8 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len) ret = ext4_zero_range(file, offset, len, mode); else ret = ext4_do_fallocate(file, offset, len, mode); - +out: + inode_unlock(inode); return ret; } @@ -5298,36 +5289,27 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len) int ret; trace_ext4_collapse_range(inode, offset, len); - - inode_lock(inode); + WARN_ON_ONCE(!inode_is_locked(inode)); /* Currently just for extent based files */ - if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) { - ret = -EOPNOTSUPP; - goto out; - } - + if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) + return -EOPNOTSUPP; /* Collapse range works only on fs cluster size aligned regions. */ - if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) { - ret = -EINVAL; - goto out; - } - + if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) + return -EINVAL; /* * There is no need to overlap collapse range with EOF, in which case * it is effectively a truncate operation */ - if (end >= inode->i_size) { - ret = -EINVAL; - goto out; - } + if (end >= inode->i_size) + return -EINVAL; /* Wait for existing dio to complete */ inode_dio_wait(inode); ret = file_modified(file); if (ret) - goto out; + return ret; /* * Prevent page faults from reinstantiating pages we have released from @@ -5402,8 +5384,6 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len) ext4_journal_stop(handle); out_invalidate_lock: filemap_invalidate_unlock(mapping); -out: - inode_unlock(inode); return ret; } @@ -5429,39 +5409,27 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len) loff_t start; trace_ext4_insert_range(inode, offset, len); - - inode_lock(inode); + WARN_ON_ONCE(!inode_is_locked(inode)); /* Currently just for extent based files */ - if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) { - ret = -EOPNOTSUPP; - goto out; - } - + if (!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) + return -EOPNOTSUPP; /* Insert range works only on fs cluster size aligned regions. */ - if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) { - ret = -EINVAL; - goto out; - } - + if (!IS_ALIGNED(offset | len, EXT4_CLUSTER_SIZE(sb))) + return -EINVAL; /* Offset must be less than i_size */ - if (offset >= inode->i_size) { - ret = -EINVAL; - goto out; - } - + if (offset >= inode->i_size) + return -EINVAL; /* Check whether the maximum file size would be exceeded */ - if (len > inode->i_sb->s_maxbytes - inode->i_size) { - ret = -EFBIG; - goto out; - } + 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) - goto out; + return ret; /* * Prevent page faults from reinstantiating pages we have released from @@ -5562,8 +5530,6 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len) ext4_journal_stop(handle); out_invalidate_lock: filemap_invalidate_unlock(mapping); -out: - inode_unlock(inode); return ret; } diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 1d128333bd06..bea19cd6e676 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3962,15 +3962,14 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) unsigned long blocksize = i_blocksize(inode); handle_t *handle; unsigned int credits; - int ret = 0; + int ret; trace_ext4_punch_hole(inode, offset, length, 0); - - inode_lock(inode); + WARN_ON_ONCE(!inode_is_locked(inode)); /* No need to punch hole beyond i_size */ if (offset >= inode->i_size) - goto out; + return 0; /* * If the hole extends beyond i_size, set the hole to end after @@ -3990,7 +3989,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) if (offset & (blocksize - 1) || end & (blocksize - 1)) { ret = ext4_inode_attach_jinode(inode); if (ret < 0) - goto out; + return ret; } /* Wait all existing dio workers, newcomers will block on i_rwsem */ @@ -3998,7 +3997,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) ret = file_modified(file); if (ret) - goto out; + return ret; /* * Prevent page faults from reinstantiating pages we have released from @@ -4082,8 +4081,6 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length) ext4_journal_stop(handle); out_invalidate_lock: filemap_invalidate_unlock(mapping); -out: - inode_unlock(inode); return ret; }