In the dio write path, we only take shared inode lock for the case of
aligned overwriting initialized blocks inside EOF. But for overwriting
preallocated blocks, it may only need to split unwritten extents, this
procedure has been protected under i_data_sem lock, it's safe to
release the exclusive inode lock and take shared inode lock.
This could give a significant speed up for multi-threaded writes. Test
on Intel Xeon Gold 6140 and nvme SSD with below fio parameters.
direct=1
ioengine=libaio
iodepth=10
numjobs=10
runtime=60
rw=randwrite
size=100G
And the test result are:
Before:
bs=4k IOPS=11.1k, BW=43.2MiB/s
bs=16k IOPS=11.1k, BW=173MiB/s
bs=64k IOPS=11.2k, BW=697MiB/s
After:
bs=4k IOPS=41.4k, BW=162MiB/s
bs=16k IOPS=41.3k, BW=646MiB/s
bs=64k IOPS=13.5k, BW=843MiB/s
Signed-off-by: Zhang Yi <[email protected]>
---
It passed xfstests auto mode with 1k and 4k blocksize.
fs/ext4/file.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index a7a597c727e6..7edac94025ac 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -202,8 +202,9 @@ ext4_extending_io(struct inode *inode, loff_t offset, size_t len)
return false;
}
-/* Is IO overwriting allocated and initialized blocks? */
-static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
+/* Is IO overwriting allocated or initialized blocks? */
+static bool ext4_overwrite_io(struct inode *inode,
+ loff_t pos, loff_t len, bool *inited)
{
struct ext4_map_blocks map;
unsigned int blkbits = inode->i_blkbits;
@@ -217,12 +218,15 @@ static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
blklen = map.m_len;
err = ext4_map_blocks(NULL, inode, &map, 0);
+ if (err != blklen)
+ return false;
/*
* 'err==len' means that all of the blocks have been preallocated,
- * regardless of whether they have been initialized or not. To exclude
- * unwritten extents, we need to check m_flags.
+ * regardless of whether they have been initialized or not. We need to
+ * check m_flags to distinguish the unwritten extents.
*/
- return err == blklen && (map.m_flags & EXT4_MAP_MAPPED);
+ *inited = !!(map.m_flags & EXT4_MAP_MAPPED);
+ return true;
}
static ssize_t ext4_generic_write_checks(struct kiocb *iocb,
@@ -431,11 +435,16 @@ static const struct iomap_dio_ops ext4_dio_write_ops = {
* - For extending writes case we don't take the shared lock, since it requires
* updating inode i_disksize and/or orphan handling with exclusive lock.
*
- * - shared locking will only be true mostly with overwrites. Otherwise we will
- * switch to exclusive i_rwsem lock.
+ * - shared locking will only be true mostly with overwrites, include
+ * initialized blocks and unwritten blocks. For overwrite unwritten blocks
+ * we protects splitting extents by i_data_sem in ext4_inode_info, so we can
+ * also release exclusive i_rwsem lock.
+ *
+ * - Otherwise we will switch to exclusive i_rwsem lock.
*/
static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
- bool *ilock_shared, bool *extend)
+ bool *ilock_shared, bool *extend,
+ bool *overwrite)
{
struct file *file = iocb->ki_filp;
struct inode *inode = file_inode(file);
@@ -459,7 +468,7 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
* in file_modified().
*/
if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
- !ext4_overwrite_io(inode, offset, count))) {
+ !ext4_overwrite_io(inode, offset, count, overwrite))) {
if (iocb->ki_flags & IOCB_NOWAIT) {
ret = -EAGAIN;
goto out;
@@ -491,7 +500,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
loff_t offset = iocb->ki_pos;
size_t count = iov_iter_count(from);
const struct iomap_ops *iomap_ops = &ext4_iomap_ops;
- bool extend = false, unaligned_io = false;
+ bool extend = false, unaligned_io = false, overwrite = false;
bool ilock_shared = true;
/*
@@ -534,7 +543,8 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
return ext4_buffered_write_iter(iocb, from);
}
- ret = ext4_dio_write_checks(iocb, from, &ilock_shared, &extend);
+ ret = ext4_dio_write_checks(iocb, from,
+ &ilock_shared, &extend, &overwrite);
if (ret <= 0)
return ret;
@@ -582,7 +592,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
ext4_journal_stop(handle);
}
- if (ilock_shared)
+ if (overwrite)
iomap_ops = &ext4_iomap_overwrite_ops;
ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops,
(unaligned_io || extend) ? IOMAP_DIO_FORCE_WAIT : 0,
--
2.31.1
Hello, is anybody have advice?
Thanks,
Yi.
On 2022/12/3 18:39, Zhang Yi wrote:
> In the dio write path, we only take shared inode lock for the case of
> aligned overwriting initialized blocks inside EOF. But for overwriting
> preallocated blocks, it may only need to split unwritten extents, this
> procedure has been protected under i_data_sem lock, it's safe to
> release the exclusive inode lock and take shared inode lock.
>
> This could give a significant speed up for multi-threaded writes. Test
> on Intel Xeon Gold 6140 and nvme SSD with below fio parameters.
>
> direct=1
> ioengine=libaio
> iodepth=10
> numjobs=10
> runtime=60
> rw=randwrite
> size=100G
>
> And the test result are:
> Before:
> bs=4k IOPS=11.1k, BW=43.2MiB/s
> bs=16k IOPS=11.1k, BW=173MiB/s
> bs=64k IOPS=11.2k, BW=697MiB/s
>
> After:
> bs=4k IOPS=41.4k, BW=162MiB/s
> bs=16k IOPS=41.3k, BW=646MiB/s
> bs=64k IOPS=13.5k, BW=843MiB/s
>
> Signed-off-by: Zhang Yi <[email protected]>
> ---
> It passed xfstests auto mode with 1k and 4k blocksize.
>
> fs/ext4/file.c | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index a7a597c727e6..7edac94025ac 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -202,8 +202,9 @@ ext4_extending_io(struct inode *inode, loff_t offset, size_t len)
> return false;
> }
>
> -/* Is IO overwriting allocated and initialized blocks? */
> -static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
> +/* Is IO overwriting allocated or initialized blocks? */
> +static bool ext4_overwrite_io(struct inode *inode,
> + loff_t pos, loff_t len, bool *inited)
> {
> struct ext4_map_blocks map;
> unsigned int blkbits = inode->i_blkbits;
> @@ -217,12 +218,15 @@ static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
> blklen = map.m_len;
>
> err = ext4_map_blocks(NULL, inode, &map, 0);
> + if (err != blklen)
> + return false;
> /*
> * 'err==len' means that all of the blocks have been preallocated,
> - * regardless of whether they have been initialized or not. To exclude
> - * unwritten extents, we need to check m_flags.
> + * regardless of whether they have been initialized or not. We need to
> + * check m_flags to distinguish the unwritten extents.
> */
> - return err == blklen && (map.m_flags & EXT4_MAP_MAPPED);
> + *inited = !!(map.m_flags & EXT4_MAP_MAPPED);
> + return true;
> }
>
> static ssize_t ext4_generic_write_checks(struct kiocb *iocb,
> @@ -431,11 +435,16 @@ static const struct iomap_dio_ops ext4_dio_write_ops = {
> * - For extending writes case we don't take the shared lock, since it requires
> * updating inode i_disksize and/or orphan handling with exclusive lock.
> *
> - * - shared locking will only be true mostly with overwrites. Otherwise we will
> - * switch to exclusive i_rwsem lock.
> + * - shared locking will only be true mostly with overwrites, include
> + * initialized blocks and unwritten blocks. For overwrite unwritten blocks
> + * we protects splitting extents by i_data_sem in ext4_inode_info, so we can
> + * also release exclusive i_rwsem lock.
> + *
> + * - Otherwise we will switch to exclusive i_rwsem lock.
> */
> static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
> - bool *ilock_shared, bool *extend)
> + bool *ilock_shared, bool *extend,
> + bool *overwrite)
> {
> struct file *file = iocb->ki_filp;
> struct inode *inode = file_inode(file);
> @@ -459,7 +468,7 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
> * in file_modified().
> */
> if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
> - !ext4_overwrite_io(inode, offset, count))) {
> + !ext4_overwrite_io(inode, offset, count, overwrite))) {
> if (iocb->ki_flags & IOCB_NOWAIT) {
> ret = -EAGAIN;
> goto out;
> @@ -491,7 +500,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
> loff_t offset = iocb->ki_pos;
> size_t count = iov_iter_count(from);
> const struct iomap_ops *iomap_ops = &ext4_iomap_ops;
> - bool extend = false, unaligned_io = false;
> + bool extend = false, unaligned_io = false, overwrite = false;
> bool ilock_shared = true;
>
> /*
> @@ -534,7 +543,8 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
> return ext4_buffered_write_iter(iocb, from);
> }
>
> - ret = ext4_dio_write_checks(iocb, from, &ilock_shared, &extend);
> + ret = ext4_dio_write_checks(iocb, from,
> + &ilock_shared, &extend, &overwrite);
> if (ret <= 0)
> return ret;
>
> @@ -582,7 +592,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
> ext4_journal_stop(handle);
> }
>
> - if (ilock_shared)
> + if (overwrite)
> iomap_ops = &ext4_iomap_overwrite_ops;
> ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops,
> (unaligned_io || extend) ? IOMAP_DIO_FORCE_WAIT : 0,
>
On Sat 03-12-22 18:39:56, Zhang Yi wrote:
> In the dio write path, we only take shared inode lock for the case of
> aligned overwriting initialized blocks inside EOF. But for overwriting
> preallocated blocks, it may only need to split unwritten extents, this
> procedure has been protected under i_data_sem lock, it's safe to
> release the exclusive inode lock and take shared inode lock.
>
> This could give a significant speed up for multi-threaded writes. Test
> on Intel Xeon Gold 6140 and nvme SSD with below fio parameters.
>
> direct=1
> ioengine=libaio
> iodepth=10
> numjobs=10
> runtime=60
> rw=randwrite
> size=100G
>
> And the test result are:
> Before:
> bs=4k IOPS=11.1k, BW=43.2MiB/s
> bs=16k IOPS=11.1k, BW=173MiB/s
> bs=64k IOPS=11.2k, BW=697MiB/s
>
> After:
> bs=4k IOPS=41.4k, BW=162MiB/s
> bs=16k IOPS=41.3k, BW=646MiB/s
> bs=64k IOPS=13.5k, BW=843MiB/s
>
> Signed-off-by: Zhang Yi <[email protected]>
> ---
> It passed xfstests auto mode with 1k and 4k blocksize.
Besides some naming nits (see below) I think this should work. But I have
to say I'm a bit uneasy about this because we will now be changing block
mapping from unwritten to written only with shared i_rwsem. OTOH that
happens during writeback as well so we should be fine and the gain is very
nice.
Out of curiosity do you have a real usecase for this?
> fs/ext4/file.c | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index a7a597c727e6..7edac94025ac 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -202,8 +202,9 @@ ext4_extending_io(struct inode *inode, loff_t offset, size_t len)
> return false;
> }
>
> -/* Is IO overwriting allocated and initialized blocks? */
> -static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
> +/* Is IO overwriting allocated or initialized blocks? */
> +static bool ext4_overwrite_io(struct inode *inode,
> + loff_t pos, loff_t len, bool *inited)
'inited' just sounds weird. Correct is 'initialized' which is a bit long.
Maybe just negate the meaning and call this 'unwritten'?
> {
> struct ext4_map_blocks map;
> unsigned int blkbits = inode->i_blkbits;
> @@ -217,12 +218,15 @@ static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
> blklen = map.m_len;
>
> err = ext4_map_blocks(NULL, inode, &map, 0);
> + if (err != blklen)
> + return false;
> /*
> * 'err==len' means that all of the blocks have been preallocated,
> - * regardless of whether they have been initialized or not. To exclude
> - * unwritten extents, we need to check m_flags.
> + * regardless of whether they have been initialized or not. We need to
> + * check m_flags to distinguish the unwritten extents.
> */
> - return err == blklen && (map.m_flags & EXT4_MAP_MAPPED);
> + *inited = !!(map.m_flags & EXT4_MAP_MAPPED);
> + return true;
> }
>
> static ssize_t ext4_generic_write_checks(struct kiocb *iocb,
> @@ -431,11 +435,16 @@ static const struct iomap_dio_ops ext4_dio_write_ops = {
> * - For extending writes case we don't take the shared lock, since it requires
> * updating inode i_disksize and/or orphan handling with exclusive lock.
> *
> - * - shared locking will only be true mostly with overwrites. Otherwise we will
> - * switch to exclusive i_rwsem lock.
> + * - shared locking will only be true mostly with overwrites, include
> + * initialized blocks and unwritten blocks. For overwrite unwritten blocks
> + * we protects splitting extents by i_data_sem in ext4_inode_info, so we can
> + * also release exclusive i_rwsem lock.
> + *
> + * - Otherwise we will switch to exclusive i_rwsem lock.
> */
> static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
> - bool *ilock_shared, bool *extend)
> + bool *ilock_shared, bool *extend,
> + bool *overwrite)
And it would be good to preserve the argument name in other functions as
well - i.e., keep using 'unwritten' here as well.
> {
> struct file *file = iocb->ki_filp;
> struct inode *inode = file_inode(file);
> @@ -459,7 +468,7 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
> * in file_modified().
> */
> if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
> - !ext4_overwrite_io(inode, offset, count))) {
> + !ext4_overwrite_io(inode, offset, count, overwrite))) {
> if (iocb->ki_flags & IOCB_NOWAIT) {
> ret = -EAGAIN;
> goto out;
> @@ -491,7 +500,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
> loff_t offset = iocb->ki_pos;
> size_t count = iov_iter_count(from);
> const struct iomap_ops *iomap_ops = &ext4_iomap_ops;
> - bool extend = false, unaligned_io = false;
> + bool extend = false, unaligned_io = false, overwrite = false;
> bool ilock_shared = true;
>
> /*
> @@ -534,7 +543,8 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
> return ext4_buffered_write_iter(iocb, from);
> }
>
> - ret = ext4_dio_write_checks(iocb, from, &ilock_shared, &extend);
> + ret = ext4_dio_write_checks(iocb, from,
> + &ilock_shared, &extend, &overwrite);
> if (ret <= 0)
> return ret;
>
> @@ -582,7 +592,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
> ext4_journal_stop(handle);
> }
>
> - if (ilock_shared)
> + if (overwrite)
> iomap_ops = &ext4_iomap_overwrite_ops;
> ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops,
> (unaligned_io || extend) ? IOMAP_DIO_FORCE_WAIT : 0,
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, Dec 14, 2022 at 06:01:25PM +0100, Jan Kara wrote:
>
> Besides some naming nits (see below) I think this should work. But I have
> to say I'm a bit uneasy about this because we will now be changing block
> mapping from unwritten to written only with shared i_rwsem. OTOH that
> happens during writeback as well so we should be fine and the gain is very
> nice.
Hmm.... when I was looking potential impacts of the change what
ext4_overwrite_io() would do, I looked at the current user of that
function in ext4_dio_write_checks().
/*
* Determine whether the IO operation will overwrite allocated
* and initialized blocks.
* We need exclusive i_rwsem for changing security info
* in file_modified().
*/
if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
!ext4_overwrite_io(inode, offset, count))) {
if (iocb->ki_flags & IOCB_NOWAIT) {
ret = -EAGAIN;
goto out;
}
inode_unlock_shared(inode);
*ilock_shared = false;
inode_lock(inode);
goto restart;
}
ret = file_modified(file);
if (ret < 0)
goto out;
What is confusing me is the comment, "We need exclusive i_rwsem for
changing security info in file_modified().". But then we end up
calling file_modified() unconditionally, regardless of whether we've
transitioned from a shared lock to an exclusive lock.
So file_modified() can get called either with or without the inode
locked r/w. I realize that this patch doesn't change this
inconsistency, but it appears either the comment is wrong, or the code
is wrong.
What am I missing?
- Ted
On 2022/12/15 1:01, Jan Kara wrote:
> On Sat 03-12-22 18:39:56, Zhang Yi wrote:
>> In the dio write path, we only take shared inode lock for the case of
>> aligned overwriting initialized blocks inside EOF. But for overwriting
>> preallocated blocks, it may only need to split unwritten extents, this
>> procedure has been protected under i_data_sem lock, it's safe to
>> release the exclusive inode lock and take shared inode lock.
>>
>> This could give a significant speed up for multi-threaded writes. Test
>> on Intel Xeon Gold 6140 and nvme SSD with below fio parameters.
>>
>> direct=1
>> ioengine=libaio
>> iodepth=10
>> numjobs=10
>> runtime=60
>> rw=randwrite
>> size=100G
>>
>> And the test result are:
>> Before:
>> bs=4k IOPS=11.1k, BW=43.2MiB/s
>> bs=16k IOPS=11.1k, BW=173MiB/s
>> bs=64k IOPS=11.2k, BW=697MiB/s
>>
>> After:
>> bs=4k IOPS=41.4k, BW=162MiB/s
>> bs=16k IOPS=41.3k, BW=646MiB/s
>> bs=64k IOPS=13.5k, BW=843MiB/s
>>
>> Signed-off-by: Zhang Yi <[email protected]>
>> ---
>> It passed xfstests auto mode with 1k and 4k blocksize.
>
> Besides some naming nits (see below) I think this should work. But I have
> to say I'm a bit uneasy about this because we will now be changing block
> mapping from unwritten to written only with shared i_rwsem. OTOH that
> happens during writeback as well so we should be fine and the gain is very
> nice.
>
Thanks for advice, I will change the argument name to make it more reasonable.
> Out of curiosity do you have a real usecase for this?
No, I was just analyse the performance gap in our benchmark tests, and have
some question and idea while reading the code. Maybe it could speed up the
first time write in some database. :)
Besides, I want to discuss it a bit more. I originally changed this code to
switch to take the shared inode and also use ext4_iomap_overwrite_ops for
the overwriting preallocated blocks case. It will postpone the splitting extent
procedure to endio and could give a much more gain than this patch (+~27%).
This patch:
bs=4k IOPS=41.4k, BW=162MiB/s
Postpone split:
bs=4k IOPS=52.9k, BW=207MiB/s
But I think it's maybe too radical. I looked at the history and notice in commit
0031462b5b39 ("ext4: Split uninitialized extents for direct I/O"), it introduce
the flag EXT4_GET_BLOCKS_DIO(now it had been renamed to EXT4_GET_BLOCKS_PRE_IO)
to make sure that the preallocated unwritten extent have been splitted before
submitting the I/O, which is used to prevent the ENOSPC problem if the filesystem
run out-of-space in the endio procedure. And 4 years later, commit 27dd43854227
("ext4: introduce reserved space") reserve some blocks for metedata allocation.
It looks like this commit could also slove the ENOSPC problem for most cases if
we postpone extent splitting into the endio procedure. I don't find other side
effect, so I think it may also fine if we do that. Do you have some advice or
am I missing something?
Thanks,
Yi.
>
>> fs/ext4/file.c | 34 ++++++++++++++++++++++------------
>> 1 file changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
>> index a7a597c727e6..7edac94025ac 100644
>> --- a/fs/ext4/file.c
>> +++ b/fs/ext4/file.c
>> @@ -202,8 +202,9 @@ ext4_extending_io(struct inode *inode, loff_t offset, size_t len)
>> return false;
>> }
>>
>> -/* Is IO overwriting allocated and initialized blocks? */
>> -static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
>> +/* Is IO overwriting allocated or initialized blocks? */
>> +static bool ext4_overwrite_io(struct inode *inode,
>> + loff_t pos, loff_t len, bool *inited)
>
> 'inited' just sounds weird. Correct is 'initialized' which is a bit long.
> Maybe just negate the meaning and call this 'unwritten'?
>
>> {
>> struct ext4_map_blocks map;
>> unsigned int blkbits = inode->i_blkbits;
>> @@ -217,12 +218,15 @@ static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len)
>> blklen = map.m_len;
>>
>> err = ext4_map_blocks(NULL, inode, &map, 0);
>> + if (err != blklen)
>> + return false;
>> /*
>> * 'err==len' means that all of the blocks have been preallocated,
>> - * regardless of whether they have been initialized or not. To exclude
>> - * unwritten extents, we need to check m_flags.
>> + * regardless of whether they have been initialized or not. We need to
>> + * check m_flags to distinguish the unwritten extents.
>> */
>> - return err == blklen && (map.m_flags & EXT4_MAP_MAPPED);
>> + *inited = !!(map.m_flags & EXT4_MAP_MAPPED);
>> + return true;
>> }
>>
>> static ssize_t ext4_generic_write_checks(struct kiocb *iocb,
>> @@ -431,11 +435,16 @@ static const struct iomap_dio_ops ext4_dio_write_ops = {
>> * - For extending writes case we don't take the shared lock, since it requires
>> * updating inode i_disksize and/or orphan handling with exclusive lock.
>> *
>> - * - shared locking will only be true mostly with overwrites. Otherwise we will
>> - * switch to exclusive i_rwsem lock.
>> + * - shared locking will only be true mostly with overwrites, include
>> + * initialized blocks and unwritten blocks. For overwrite unwritten blocks
>> + * we protects splitting extents by i_data_sem in ext4_inode_info, so we can
>> + * also release exclusive i_rwsem lock.
>> + *
>> + * - Otherwise we will switch to exclusive i_rwsem lock.
>> */
>> static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
>> - bool *ilock_shared, bool *extend)
>> + bool *ilock_shared, bool *extend,
>> + bool *overwrite)
>
> And it would be good to preserve the argument name in other functions as
> well - i.e., keep using 'unwritten' here as well.
>
>> {
>> struct file *file = iocb->ki_filp;
>> struct inode *inode = file_inode(file);
>> @@ -459,7 +468,7 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
>> * in file_modified().
>> */
>> if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
>> - !ext4_overwrite_io(inode, offset, count))) {
>> + !ext4_overwrite_io(inode, offset, count, overwrite))) {
>> if (iocb->ki_flags & IOCB_NOWAIT) {
>> ret = -EAGAIN;
>> goto out;
>> @@ -491,7 +500,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
>> loff_t offset = iocb->ki_pos;
>> size_t count = iov_iter_count(from);
>> const struct iomap_ops *iomap_ops = &ext4_iomap_ops;
>> - bool extend = false, unaligned_io = false;
>> + bool extend = false, unaligned_io = false, overwrite = false;
>> bool ilock_shared = true;
>>
>> /*
>> @@ -534,7 +543,8 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
>> return ext4_buffered_write_iter(iocb, from);
>> }
>>
>> - ret = ext4_dio_write_checks(iocb, from, &ilock_shared, &extend);
>> + ret = ext4_dio_write_checks(iocb, from,
>> + &ilock_shared, &extend, &overwrite);
>> if (ret <= 0)
>> return ret;
>>
>> @@ -582,7 +592,7 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
>> ext4_journal_stop(handle);
>> }
>>
>> - if (ilock_shared)
>> + if (overwrite)
>> iomap_ops = &ext4_iomap_overwrite_ops;
>> ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops,
>> (unaligned_io || extend) ? IOMAP_DIO_FORCE_WAIT : 0,
On Wed 14-12-22 13:52:32, Theodore Ts'o wrote:
> On Wed, Dec 14, 2022 at 06:01:25PM +0100, Jan Kara wrote:
> >
> > Besides some naming nits (see below) I think this should work. But I have
> > to say I'm a bit uneasy about this because we will now be changing block
> > mapping from unwritten to written only with shared i_rwsem. OTOH that
> > happens during writeback as well so we should be fine and the gain is very
> > nice.
>
> Hmm.... when I was looking potential impacts of the change what
> ext4_overwrite_io() would do, I looked at the current user of that
> function in ext4_dio_write_checks().
>
> /*
> * Determine whether the IO operation will overwrite allocated
> * and initialized blocks.
> * We need exclusive i_rwsem for changing security info
> * in file_modified().
> */
> if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
> !ext4_overwrite_io(inode, offset, count))) {
> if (iocb->ki_flags & IOCB_NOWAIT) {
> ret = -EAGAIN;
> goto out;
> }
> inode_unlock_shared(inode);
> *ilock_shared = false;
> inode_lock(inode);
> goto restart;
> }
>
> ret = file_modified(file);
> if (ret < 0)
> goto out;
>
> What is confusing me is the comment, "We need exclusive i_rwsem for
> changing security info in file_modified().". But then we end up
> calling file_modified() unconditionally, regardless of whether we've
> transitioned from a shared lock to an exclusive lock.
>
> So file_modified() can get called either with or without the inode
> locked r/w. I realize that this patch doesn't change this
> inconsistency, but it appears either the comment is wrong, or the code
> is wrong.
Maybe the comment needs rephrasing but it seems correct. file_modified()
does multiple things. It updates timestamps - these are fine with shared
i_rwsem - and is calls into __file_remove_privs() to remove SUID bits etc.
Now if __file_remove_privs() is going to modify the inode, we need i_rwsem
exclusively. And we determine whether __file_remove_privs() will do
anything by checking !IS_NOSEC(inode) in the condition above. So the
sentence you're confused about speaks about this part of the condition.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2022/12/15 16:41, Zhang Yi wrote:
> On 2022/12/15 2:52, Theodore Ts'o wrote:
>> On Wed, Dec 14, 2022 at 06:01:25PM +0100, Jan Kara wrote:
>>>
>>> Besides some naming nits (see below) I think this should work. But I have
>>> to say I'm a bit uneasy about this because we will now be changing block
>>> mapping from unwritten to written only with shared i_rwsem. OTOH that
>>> happens during writeback as well so we should be fine and the gain is very
>>> nice.
>>
>> Hmm.... when I was looking potential impacts of the change what
>> ext4_overwrite_io() would do, I looked at the current user of that
>> function in ext4_dio_write_checks().
>>
>> /*
>> * Determine whether the IO operation will overwrite allocated
>> * and initialized blocks.
>> * We need exclusive i_rwsem for changing security info
>> * in file_modified().
>> */
>> if (*ilock_shared && (!IS_NOSEC(inode) || *extend ||
>> !ext4_overwrite_io(inode, offset, count))) {
>> if (iocb->ki_flags & IOCB_NOWAIT) {
>> ret = -EAGAIN;
>> goto out;
>> }
>> inode_unlock_shared(inode);
>> *ilock_shared = false;
>> inode_lock(inode);
>> goto restart;
>> }
>>
>> ret = file_modified(file);
>> if (ret < 0)
>> goto out;
>>
>> What is confusing me is the comment, "We need exclusive i_rwsem for
>> changing security info in file_modified().". But then we end up
>> calling file_modified() unconditionally, regardless of whether we've
>> transitioned from a shared lock to an exclusive lock.
>>
>> So file_modified() can get called either with or without the inode
>> locked r/w. I realize that this patch doesn't change this
>> inconsistency, but it appears either the comment is wrong, or the code
>> is wrong.
>>
>> What am I missing?
>>
>
> IIUC, both of the comment and the code are correct, the __file_remove_privs()
> in file_modified() should execute under exclusive lock, and we have already
> check the IS_NOSEC(inode) and could make sure taking exclusive lock before we
> remove privs. If we take share lock, __file_remove_privs() will return directly
> because below check. So it's find now, but it looks that call file_update_time()
> is enough for the shared lock case.
>
> int file_update_time(struct file *file)
static int __file_remove_privs(struct file *file, unsigned int flags)
> {
...
> if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode))
> return 0;
> ...
> }
>
> Thanks,
> Yi.
>
On Thu 15-12-22 16:24:49, Zhang Yi wrote:
> On 2022/12/15 1:01, Jan Kara wrote:
> > On Sat 03-12-22 18:39:56, Zhang Yi wrote:
> >> In the dio write path, we only take shared inode lock for the case of
> >> aligned overwriting initialized blocks inside EOF. But for overwriting
> >> preallocated blocks, it may only need to split unwritten extents, this
> >> procedure has been protected under i_data_sem lock, it's safe to
> >> release the exclusive inode lock and take shared inode lock.
> >>
> >> This could give a significant speed up for multi-threaded writes. Test
> >> on Intel Xeon Gold 6140 and nvme SSD with below fio parameters.
> >>
> >> direct=1
> >> ioengine=libaio
> >> iodepth=10
> >> numjobs=10
> >> runtime=60
> >> rw=randwrite
> >> size=100G
> >>
> >> And the test result are:
> >> Before:
> >> bs=4k IOPS=11.1k, BW=43.2MiB/s
> >> bs=16k IOPS=11.1k, BW=173MiB/s
> >> bs=64k IOPS=11.2k, BW=697MiB/s
> >>
> >> After:
> >> bs=4k IOPS=41.4k, BW=162MiB/s
> >> bs=16k IOPS=41.3k, BW=646MiB/s
> >> bs=64k IOPS=13.5k, BW=843MiB/s
> >>
> >> Signed-off-by: Zhang Yi <[email protected]>
> >> ---
> >> It passed xfstests auto mode with 1k and 4k blocksize.
> >
> > Besides some naming nits (see below) I think this should work. But I have
> > to say I'm a bit uneasy about this because we will now be changing block
> > mapping from unwritten to written only with shared i_rwsem. OTOH that
> > happens during writeback as well so we should be fine and the gain is very
> > nice.
> >
> Thanks for advice, I will change the argument name to make it more reasonable.
>
> > Out of curiosity do you have a real usecase for this?
>
> No, I was just analyse the performance gap in our benchmark tests, and have
> some question and idea while reading the code. Maybe it could speed up the
> first time write in some database. :)
>
> Besides, I want to discuss it a bit more. I originally changed this code to
> switch to take the shared inode and also use ext4_iomap_overwrite_ops for
> the overwriting preallocated blocks case. It will postpone the splitting extent
> procedure to endio and could give a much more gain than this patch (+~27%).
>
> This patch:
> bs=4k IOPS=41.4k, BW=162MiB/s
> Postpone split:
> bs=4k IOPS=52.9k, BW=207MiB/s
>
> But I think it's maybe too radical. I looked at the history and notice in
> commit 0031462b5b39 ("ext4: Split uninitialized extents for direct I/O"),
> it introduce the flag EXT4_GET_BLOCKS_DIO(now it had been renamed to
> EXT4_GET_BLOCKS_PRE_IO) to make sure that the preallocated unwritten
> extent have been splitted before submitting the I/O, which is used to
> prevent the ENOSPC problem if the filesystem run out-of-space in the
> endio procedure. And 4 years later, commit 27dd43854227 ("ext4: introduce
> reserved space") reserve some blocks for metedata allocation. It looks
> like this commit could also slove the ENOSPC problem for most cases if we
> postpone extent splitting into the endio procedure. I don't find other
> side effect, so I think it may also fine if we do that. Do you have some
> advice or am I missing something?
So you are right these days splitting of extents could be done only on IO
completion because we have a pool of blocks reserved for these cases. OTOH
this will make the pressure on the reserved pool higher and if we are
running out of space and there are enough operations running in parallel we
*could* run out of reserved blocks. So I wouldn't always defer extent
splitting to IO completion unless we have a practical and sufficiently
widespread usecase that would benefit from this optimization.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 2022/12/15 17:00, Jan Kara wrote:
> On Thu 15-12-22 16:24:49, Zhang Yi wrote:
>> On 2022/12/15 1:01, Jan Kara wrote:
>>> On Sat 03-12-22 18:39:56, Zhang Yi wrote:
>>>> In the dio write path, we only take shared inode lock for the case of
>>>> aligned overwriting initialized blocks inside EOF. But for overwriting
>>>> preallocated blocks, it may only need to split unwritten extents, this
>>>> procedure has been protected under i_data_sem lock, it's safe to
>>>> release the exclusive inode lock and take shared inode lock.
>>>>
>>>> This could give a significant speed up for multi-threaded writes. Test
>>>> on Intel Xeon Gold 6140 and nvme SSD with below fio parameters.
>>>>
>>>> direct=1
>>>> ioengine=libaio
>>>> iodepth=10
>>>> numjobs=10
>>>> runtime=60
>>>> rw=randwrite
>>>> size=100G
>>>>
>>>> And the test result are:
>>>> Before:
>>>> bs=4k IOPS=11.1k, BW=43.2MiB/s
>>>> bs=16k IOPS=11.1k, BW=173MiB/s
>>>> bs=64k IOPS=11.2k, BW=697MiB/s
>>>>
>>>> After:
>>>> bs=4k IOPS=41.4k, BW=162MiB/s
>>>> bs=16k IOPS=41.3k, BW=646MiB/s
>>>> bs=64k IOPS=13.5k, BW=843MiB/s
>>>>
>>>> Signed-off-by: Zhang Yi <[email protected]>
>>>> ---
>>>> It passed xfstests auto mode with 1k and 4k blocksize.
>>>
>>> Besides some naming nits (see below) I think this should work. But I have
>>> to say I'm a bit uneasy about this because we will now be changing block
>>> mapping from unwritten to written only with shared i_rwsem. OTOH that
>>> happens during writeback as well so we should be fine and the gain is very
>>> nice.
>>>
>> Thanks for advice, I will change the argument name to make it more reasonable.
>>
>>> Out of curiosity do you have a real usecase for this?
>>
>> No, I was just analyse the performance gap in our benchmark tests, and have
>> some question and idea while reading the code. Maybe it could speed up the
>> first time write in some database. :)
>>
>> Besides, I want to discuss it a bit more. I originally changed this code to
>> switch to take the shared inode and also use ext4_iomap_overwrite_ops for
>> the overwriting preallocated blocks case. It will postpone the splitting extent
>> procedure to endio and could give a much more gain than this patch (+~27%).
>>
>> This patch:
>> bs=4k IOPS=41.4k, BW=162MiB/s
>> Postpone split:
>> bs=4k IOPS=52.9k, BW=207MiB/s
>>
>> But I think it's maybe too radical. I looked at the history and notice in
>> commit 0031462b5b39 ("ext4: Split uninitialized extents for direct I/O"),
>> it introduce the flag EXT4_GET_BLOCKS_DIO(now it had been renamed to
>> EXT4_GET_BLOCKS_PRE_IO) to make sure that the preallocated unwritten
>> extent have been splitted before submitting the I/O, which is used to
>> prevent the ENOSPC problem if the filesystem run out-of-space in the
>> endio procedure. And 4 years later, commit 27dd43854227 ("ext4: introduce
>> reserved space") reserve some blocks for metedata allocation. It looks
>> like this commit could also slove the ENOSPC problem for most cases if we
>> postpone extent splitting into the endio procedure. I don't find other
>> side effect, so I think it may also fine if we do that. Do you have some
>> advice or am I missing something?
>
> So you are right these days splitting of extents could be done only on IO
> completion because we have a pool of blocks reserved for these cases. OTOH
> this will make the pressure on the reserved pool higher and if we are
> running out of space and there are enough operations running in parallel we
> *could* run out of reserved blocks. So I wouldn't always defer extent
> splitting to IO completion unless we have a practical and sufficiently
> widespread usecase that would benefit from this optimization.
>
Sure, I think so, thanks for advice.
Yi.