Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Ritesh Harjani]

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> Zhang Yi <yi.zhang@huaweicloud.com> writes:
>
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> Now we lookup extent status entry without holding the i_data_sem before
>> inserting delalloc block, it works fine in buffered write path and
>> because it holds i_rwsem and folio lock, and the mmap path holds folio
>> lock, so the found extent locklessly couldn't be modified concurrently.
>> But it could be raced by fallocate since it allocate block whitout
>> holding i_rwsem and folio lock.
>>
>> ext4_page_mkwrite()             ext4_fallocate()
>>  block_page_mkwrite()
>>   ext4_da_map_blocks()
>>    //find hole in extent status tree
>>                                  ext4_alloc_file_blocks()
>>                                   ext4_map_blocks()
>>                                    //allocate block and unwritten extent
>>    ext4_insert_delayed_block()
>>     ext4_da_reserve_space()
>>      //reserve one more block
>>     ext4_es_insert_delayed_block()
>>      //drop unwritten extent and add delayed extent by mistake
>>
>> Then, the delalloc extent is wrong until writeback, the one more
>> reserved block can't be release any more and trigger below warning:
>>
>>  EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!
>>
>> Hold i_data_sem in write mode directly can fix the problem, but it's
>> expansive, we should keep the lockless check and check the extent again
>> once we need to add an new delalloc block.
>
> Hi Zhang, 
>
> It's a nice finding. I was wondering if this was caught in any of the
> xfstests?
>
> I have reworded some of the commit message, feel free to use it if you
> think this version is better. The use of which path uses which locks was
> a bit confusing in the original commit message.
>
> <reworded from your original commit msg>
>
> ext4_da_map_blocks(), first looks up the extent status tree for any
> extent entry with i_data_sem held in read mode. It then unlocks
> i_data_sem, if it can't find an entry and take this lock in write
> mode for inserting a new da entry.

Sorry about this above paragraph. I messed this paragraph.
Here is the correct version of this.

ext4_da_map_blocks looks up for any extent entry in the extent status
tree (w/o i_data_sem) and then the looks up for any ondisk extent
mapping (with i_data_sem in read mode).

If it finds a hole in the extent status tree or if it couldn't find any
entry at all, it then takes the i_data_sem in write mode to add a da entry
into the extent status tree. This can actually race with page mkwrite
& fallocate path. 

Note that this is ok between...  <and the rest can remain same>

>
> This is ok between -
> 1. ext4 buffered-write path v/s ext4_page_mkwrite(), because of the
> folio lock
> 2. ext4 buffered write path v/s ext4 fallocate because of the inode
> lock.
>


> But this can race between ext4_page_mkwrite() & ext4 fallocate path - 
>
>  ext4_page_mkwrite()             ext4_fallocate()
>   block_page_mkwrite()
>    ext4_da_map_blocks()
>     //find hole in extent status tree
>                                   ext4_alloc_file_blocks()
>                                    ext4_map_blocks()
>                                     //allocate block and unwritten extent
>     ext4_insert_delayed_block()
>      ext4_da_reserve_space()
>       //reserve one more block
>      ext4_es_insert_delayed_block()
>       //drop unwritten extent and add delayed extent by mistake
>
> Then, the delalloc extent is wrong until writeback and the extra
> reserved block can't be released any more and it triggers below warning:
>
>   EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!
>
> This patch fixes the problem by looking up extent status tree again
> while the i_data_sem is held in write mode. If it still can't find
> any entry, then we insert a new da entry into the extent status tree.
>
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>  fs/ext4/inode.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 6a41172c06e1..118b0497a954 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -1737,6 +1737,7 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>>  		if (ext4_es_is_hole(&es))
>>  			goto add_delayed;
>>  
>> +found:
>>  		/*
>>  		 * Delayed extent could be allocated by fallocate.
>>  		 * So we need to check it.
>> @@ -1781,6 +1782,24 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>>  
>>  add_delayed:
>>  	down_write(&EXT4_I(inode)->i_data_sem);
>> +	/*
>> +	 * Lookup extents tree again under i_data_sem, make sure this
>> +	 * inserting delalloc range haven't been delayed or allocated
>> +	 * whitout holding i_rwsem and folio lock.
>> +	 */
>
> page fault path (ext4_page_mkwrite does not take i_rwsem) and fallocate
> path (no folio lock) can race. Make sure we lookup the extent status
> tree here again while i_data_sem is held in write mode, before inserting
> a new da entry in the extent status tree.
>
>


-ritesh

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Zhang Yi]

On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
> 
>> Zhang Yi <yi.zhang@huaweicloud.com> writes:
>>
>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>
>>> Now we lookup extent status entry without holding the i_data_sem before
>>> inserting delalloc block, it works fine in buffered write path and
>>> because it holds i_rwsem and folio lock, and the mmap path holds folio
>>> lock, so the found extent locklessly couldn't be modified concurrently.
>>> But it could be raced by fallocate since it allocate block whitout
>>> holding i_rwsem and folio lock.
>>>
>>> ext4_page_mkwrite()             ext4_fallocate()
>>>  block_page_mkwrite()
>>>   ext4_da_map_blocks()
>>>    //find hole in extent status tree
>>>                                  ext4_alloc_file_blocks()
>>>                                   ext4_map_blocks()
>>>                                    //allocate block and unwritten extent
>>>    ext4_insert_delayed_block()
>>>     ext4_da_reserve_space()
>>>      //reserve one more block
>>>     ext4_es_insert_delayed_block()
>>>      //drop unwritten extent and add delayed extent by mistake
>>>
>>> Then, the delalloc extent is wrong until writeback, the one more
>>> reserved block can't be release any more and trigger below warning:
>>>
>>>  EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!
>>>
>>> Hold i_data_sem in write mode directly can fix the problem, but it's
>>> expansive, we should keep the lockless check and check the extent again
>>> once we need to add an new delalloc block.
>>
>> Hi Zhang, 
>>
>> It's a nice finding. I was wondering if this was caught in any of the
>> xfstests?
>>

Hi, Ritesh

I caught this issue when I tested my iomap series in generic/344 and
generic/346. It's easy to reproduce because the iomap's buffered write path
doesn't hold folio lock while inserting delalloc blocks, so it could be raced
by the mmap page fault path. But the buffer_head's buffered write path can't
trigger this problem, the race between buffered write path and fallocate path
was discovered while I was analyzing the code, so I'm not sure if it could
be caught by xfstests now, at least I haven't noticed this problem so far.

>> I have reworded some of the commit message, feel free to use it if you
>> think this version is better. The use of which path uses which locks was
>> a bit confusing in the original commit message.
>>

Thanks for the message improvement, it looks more clear then mine, I will
use it.

Thanks,
Yi.

>> <reworded from your original commit msg>
>>
>> ext4_da_map_blocks(), first looks up the extent status tree for any
>> extent entry with i_data_sem held in read mode. It then unlocks
>> i_data_sem, if it can't find an entry and take this lock in write
>> mode for inserting a new da entry.
> 
> Sorry about this above paragraph. I messed this paragraph.
> Here is the correct version of this.
> 
> ext4_da_map_blocks looks up for any extent entry in the extent status
> tree (w/o i_data_sem) and then the looks up for any ondisk extent
> mapping (with i_data_sem in read mode).
> 
> If it finds a hole in the extent status tree or if it couldn't find any
> entry at all, it then takes the i_data_sem in write mode to add a da entry
> into the extent status tree. This can actually race with page mkwrite
> & fallocate path. 
> 
> Note that this is ok between...  <and the rest can remain same>
> 
>>
>> This is ok between -
>> 1. ext4 buffered-write path v/s ext4_page_mkwrite(), because of the
>> folio lock
>> 2. ext4 buffered write path v/s ext4 fallocate because of the inode
>> lock.
>>
> 
> 
>> But this can race between ext4_page_mkwrite() & ext4 fallocate path - 
>>
>>  ext4_page_mkwrite()             ext4_fallocate()
>>   block_page_mkwrite()
>>    ext4_da_map_blocks()
>>     //find hole in extent status tree
>>                                   ext4_alloc_file_blocks()
>>                                    ext4_map_blocks()
>>                                     //allocate block and unwritten extent
>>     ext4_insert_delayed_block()
>>      ext4_da_reserve_space()
>>       //reserve one more block
>>      ext4_es_insert_delayed_block()
>>       //drop unwritten extent and add delayed extent by mistake
>>
>> Then, the delalloc extent is wrong until writeback and the extra
>> reserved block can't be released any more and it triggers below warning:
>>
>>   EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!
>>
>> This patch fixes the problem by looking up extent status tree again
>> while the i_data_sem is held in write mode. If it still can't find
>> any entry, then we insert a new da entry into the extent status tree.
>>
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>> ---
>>>  fs/ext4/inode.c | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>>> index 6a41172c06e1..118b0497a954 100644
>>> --- a/fs/ext4/inode.c
>>> +++ b/fs/ext4/inode.c
>>> @@ -1737,6 +1737,7 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>>>  		if (ext4_es_is_hole(&es))
>>>  			goto add_delayed;
>>>  
>>> +found:
>>>  		/*
>>>  		 * Delayed extent could be allocated by fallocate.
>>>  		 * So we need to check it.
>>> @@ -1781,6 +1782,24 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>>>  
>>>  add_delayed:
>>>  	down_write(&EXT4_I(inode)->i_data_sem);
>>> +	/*
>>> +	 * Lookup extents tree again under i_data_sem, make sure this
>>> +	 * inserting delalloc range haven't been delayed or allocated
>>> +	 * whitout holding i_rwsem and folio lock.
>>> +	 */
>>
>> page fault path (ext4_page_mkwrite does not take i_rwsem) and fallocate
>> path (no folio lock) can race. Make sure we lookup the extent status
>> tree here again while i_data_sem is held in write mode, before inserting
>> a new da entry in the extent status tree.
>>
>>
> 
> 
> -ritesh
> 

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Zhang Yi]

On 2024/4/27 0:39, Ritesh Harjani (IBM) wrote:
> Zhang Yi <yi.zhang@huaweicloud.com> writes:
> 
>> On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
>>> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
>>>
>>>> Zhang Yi <yi.zhang@huaweicloud.com> writes:
>>>>
>>>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>>>
>>>>> Now we lookup extent status entry without holding the i_data_sem before
>>>>> inserting delalloc block, it works fine in buffered write path and
>>>>> because it holds i_rwsem and folio lock, and the mmap path holds folio
>>>>> lock, so the found extent locklessly couldn't be modified concurrently.
>>>>> But it could be raced by fallocate since it allocate block whitout
>>>>> holding i_rwsem and folio lock.
>>>>>
>>>>> ext4_page_mkwrite()             ext4_fallocate()
>>>>>  block_page_mkwrite()
>>>>>   ext4_da_map_blocks()
>>>>>    //find hole in extent status tree
>>>>>                                  ext4_alloc_file_blocks()
>>>>>                                   ext4_map_blocks()
>>>>>                                    //allocate block and unwritten extent
>>>>>    ext4_insert_delayed_block()
>>>>>     ext4_da_reserve_space()
>>>>>      //reserve one more block
>>>>>     ext4_es_insert_delayed_block()
>>>>>      //drop unwritten extent and add delayed extent by mistake
>>>>>
>>>>> Then, the delalloc extent is wrong until writeback, the one more
>>>>> reserved block can't be release any more and trigger below warning:
>>>>>
>>>>>  EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!
>>>>>
>>>>> Hold i_data_sem in write mode directly can fix the problem, but it's
>>>>> expansive, we should keep the lockless check and check the extent again
>>>>> once we need to add an new delalloc block.
>>>>
>>>> Hi Zhang, 
>>>>
>>>> It's a nice finding. I was wondering if this was caught in any of the
>>>> xfstests?
>>>>
>>
>> Hi, Ritesh
>>
>> I caught this issue when I tested my iomap series in generic/344 and
>> generic/346. It's easy to reproduce because the iomap's buffered write path
>> doesn't hold folio lock while inserting delalloc blocks, so it could be raced
>> by the mmap page fault path. But the buffer_head's buffered write path can't
>> trigger this problem,
> 
> ya right! That's the difference between how ->map_blocks() is called
> between buffer_head v/s iomap path. In iomap the ->map_blocks() call
> happens first to map a large extent and then it iterate over all the
> locked folios covering the mapped extent for doing writes.
> Whereas in buffer_head while iterating, we first instantiate/lock the
> folio and then call ->map_blocks() to map an extent for the given folio.
> 
> ... So this opens up this window for a race between iomap buffered write
> path v/s page mkwrite path for inserting delalloc blocks entries.
> 
>> the race between buffered write path and fallocate path
>> was discovered while I was analyzing the code, so I'm not sure if it could
>> be caught by xfstests now, at least I haven't noticed this problem so far.
>>
> 
> Did you mean the race between page fault path and fallocate path here?
> Because buffered write path and fallocate path should not have any race
> since both takes the inode_lock. I guess you meant page fault path and
> fallocate path for which you wrote this patch too :)

Yep.

> 
> I am surprised, why we cannot see the this race between page mkwrite and
> fallocate in fstests for inserting da entries to extent status cache.
> Because the race you identified looks like a legitimate race and is
> mostly happening since ext4_da_map_blocks() was not doing the right
> thing.
> ... looking at the src/holetest, it doesn't really excercise this path.
> So maybe we can writing such fstest to trigger this race.
> 

I guess the stress tests and smoke tests in fstests have caught it,
e.g. generic/476. Since there is only one error message in ext4_destroy_inode()
when the race issue happened, we can't detect it unless we go and check the logs
manually.

I suppose we need to add more warnings, something like this, how does it sound?

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c8b691e605f1..4b6fd9b63b12 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1255,6 +1255,8 @@ static void ext4_percpu_param_destroy(struct ext4_sb_info *sbi)
 	percpu_counter_destroy(&sbi->s_freeclusters_counter);
 	percpu_counter_destroy(&sbi->s_freeinodes_counter);
 	percpu_counter_destroy(&sbi->s_dirs_counter);
+	WARN_ON_ONCE(!ext4_forced_shutdown(sbi->s_sb) &&
+		     percpu_counter_sum(&sbi->s_dirtyclusters_counter));
 	percpu_counter_destroy(&sbi->s_dirtyclusters_counter);
 	percpu_counter_destroy(&sbi->s_sra_exceeded_retry_limit);
 	percpu_free_rwsem(&sbi->s_writepages_rwsem);
@@ -1476,7 +1478,8 @@ static void ext4_destroy_inode(struct inode *inode)
 		dump_stack();
 	}

-	if (EXT4_I(inode)->i_reserved_data_blocks)
+	if (!ext4_forced_shutdown(inode->i_sb) &&
+	    WARN_ON_ONCE(EXT4_I(inode)->i_reserved_data_blocks))
 		ext4_msg(inode->i_sb, KERN_ERR,
 			 "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
 			 inode->i_ino, EXT4_I(inode),


Thanks,
Yi.

> 
>>>> I have reworded some of the commit message, feel free to use it if you
>>>> think this version is better. The use of which path uses which locks was
>>>> a bit confusing in the original commit message.
>>>>
>>
>> Thanks for the message improvement, it looks more clear then mine, I will
>> use it.
>>
> 
> Glad, it was helpful.
> 
> -ritesh
> 

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Ritesh Harjani]

Zhang Yi <yi.zhang@huaweicloud.com> writes:

> On 2024/4/27 0:39, Ritesh Harjani (IBM) wrote:
>> Zhang Yi <yi.zhang@huaweicloud.com> writes:
>> 
>>> On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
>>>> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
>>>>
>>>>> Zhang Yi <yi.zhang@huaweicloud.com> writes:
>>>>>
>>>>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>>>>
>>>>>> Now we lookup extent status entry without holding the i_data_sem before
>>>>>> inserting delalloc block, it works fine in buffered write path and
>>>>>> because it holds i_rwsem and folio lock, and the mmap path holds folio
>>>>>> lock, so the found extent locklessly couldn't be modified concurrently.
>>>>>> But it could be raced by fallocate since it allocate block whitout
>>>>>> holding i_rwsem and folio lock.
>>>>>>
>>>>>> ext4_page_mkwrite()             ext4_fallocate()
>>>>>>  block_page_mkwrite()
>>>>>>   ext4_da_map_blocks()
>>>>>>    //find hole in extent status tree
>>>>>>                                  ext4_alloc_file_blocks()
>>>>>>                                   ext4_map_blocks()
>>>>>>                                    //allocate block and unwritten extent
>>>>>>    ext4_insert_delayed_block()
>>>>>>     ext4_da_reserve_space()
>>>>>>      //reserve one more block
>>>>>>     ext4_es_insert_delayed_block()
>>>>>>      //drop unwritten extent and add delayed extent by mistake
>>>>>>
>>>>>> Then, the delalloc extent is wrong until writeback, the one more
>>>>>> reserved block can't be release any more and trigger below warning:
>>>>>>
>>>>>>  EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!
>>>>>>
>>>>>> Hold i_data_sem in write mode directly can fix the problem, but it's
>>>>>> expansive, we should keep the lockless check and check the extent again
>>>>>> once we need to add an new delalloc block.
>>>>>
>>>>> Hi Zhang, 
>>>>>
>>>>> It's a nice finding. I was wondering if this was caught in any of the
>>>>> xfstests?
>>>>>
>>>
>>> Hi, Ritesh
>>>
>>> I caught this issue when I tested my iomap series in generic/344 and
>>> generic/346. It's easy to reproduce because the iomap's buffered write path
>>> doesn't hold folio lock while inserting delalloc blocks, so it could be raced
>>> by the mmap page fault path. But the buffer_head's buffered write path can't
>>> trigger this problem,
>> 
>> ya right! That's the difference between how ->map_blocks() is called
>> between buffer_head v/s iomap path. In iomap the ->map_blocks() call
>> happens first to map a large extent and then it iterate over all the
>> locked folios covering the mapped extent for doing writes.
>> Whereas in buffer_head while iterating, we first instantiate/lock the
>> folio and then call ->map_blocks() to map an extent for the given folio.
>> 
>> ... So this opens up this window for a race between iomap buffered write
>> path v/s page mkwrite path for inserting delalloc blocks entries.
>> 
>>> the race between buffered write path and fallocate path
>>> was discovered while I was analyzing the code, so I'm not sure if it could
>>> be caught by xfstests now, at least I haven't noticed this problem so far.
>>>
>> 
>> Did you mean the race between page fault path and fallocate path here?
>> Because buffered write path and fallocate path should not have any race
>> since both takes the inode_lock. I guess you meant page fault path and
>> fallocate path for which you wrote this patch too :)
>
> Yep.
>
>> 
>> I am surprised, why we cannot see the this race between page mkwrite and
>> fallocate in fstests for inserting da entries to extent status cache.
>> Because the race you identified looks like a legitimate race and is
>> mostly happening since ext4_da_map_blocks() was not doing the right
>> thing.
>> ... looking at the src/holetest, it doesn't really excercise this path.
>> So maybe we can writing such fstest to trigger this race.
>> 
>
> I guess the stress tests and smoke tests in fstests have caught it,
> e.g. generic/476. Since there is only one error message in ext4_destroy_inode()
> when the race issue happened, we can't detect it unless we go and check the logs
> manually.

Hi Zhang,

I wasn't able to reproduce the any error messages with generic/476.

>
> I suppose we need to add more warnings, something like this, how does it sound?
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c8b691e605f1..4b6fd9b63b12 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1255,6 +1255,8 @@ static void ext4_percpu_param_destroy(struct ext4_sb_info *sbi)
>  	percpu_counter_destroy(&sbi->s_freeclusters_counter);
>  	percpu_counter_destroy(&sbi->s_freeinodes_counter);
>  	percpu_counter_destroy(&sbi->s_dirs_counter);
> +	WARN_ON_ONCE(!ext4_forced_shutdown(sbi->s_sb) &&
> +		     percpu_counter_sum(&sbi->s_dirtyclusters_counter));
>  	percpu_counter_destroy(&sbi->s_dirtyclusters_counter);
>  	percpu_counter_destroy(&sbi->s_sra_exceeded_retry_limit);
>  	percpu_free_rwsem(&sbi->s_writepages_rwsem);
> @@ -1476,7 +1478,8 @@ static void ext4_destroy_inode(struct inode *inode)
>  		dump_stack();
>  	}
>
> -	if (EXT4_I(inode)->i_reserved_data_blocks)
> +	if (!ext4_forced_shutdown(inode->i_sb) &&
> +	    WARN_ON_ONCE(EXT4_I(inode)->i_reserved_data_blocks))
>  		ext4_msg(inode->i_sb, KERN_ERR,
>  			 "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
>  			 inode->i_ino, EXT4_I(inode),
>

I also ran ext4 -g auto and I couldn't reproduce anything with above
patch. Please note that I didn't use this patch series for testing. I was running
xfstests on upstream kernel with above diff (because that's what the
idea was that the problem even exists in upstream kernel and are we able
to observe the race with page mkwrite and fallocate path)

-ritesh

>
> Thanks,
> Yi.
>
>> 
>>>>> I have reworded some of the commit message, feel free to use it if you
>>>>> think this version is better. The use of which path uses which locks was
>>>>> a bit confusing in the original commit message.
>>>>>
>>>
>>> Thanks for the message improvement, it looks more clear then mine, I will
>>> use it.
>>>
>> 
>> Glad, it was helpful.
>> 
>> -ritesh
>> 

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Zhang Yi]

On 2024/4/29 22:59, Ritesh Harjani (IBM) wrote:
> Zhang Yi <yi.zhang@huaweicloud.com> writes:
> 
>> On 2024/4/27 0:39, Ritesh Harjani (IBM) wrote:
>>> Zhang Yi <yi.zhang@huaweicloud.com> writes:
>>>
>>>> On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
>>>>> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
>>>>>
>>>>>> Zhang Yi <yi.zhang@huaweicloud.com> writes:
>>>>>>
>>>>>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>>>>>
>>>>>>> Now we lookup extent status entry without holding the i_data_sem before
>>>>>>> inserting delalloc block, it works fine in buffered write path and
>>>>>>> because it holds i_rwsem and folio lock, and the mmap path holds folio
>>>>>>> lock, so the found extent locklessly couldn't be modified concurrently.
>>>>>>> But it could be raced by fallocate since it allocate block whitout
>>>>>>> holding i_rwsem and folio lock.
>>>>>>>
>>>>>>> ext4_page_mkwrite()             ext4_fallocate()
>>>>>>>  block_page_mkwrite()
>>>>>>>   ext4_da_map_blocks()
>>>>>>>    //find hole in extent status tree
>>>>>>>                                  ext4_alloc_file_blocks()
>>>>>>>                                   ext4_map_blocks()
>>>>>>>                                    //allocate block and unwritten extent
>>>>>>>    ext4_insert_delayed_block()
>>>>>>>     ext4_da_reserve_space()
>>>>>>>      //reserve one more block
>>>>>>>     ext4_es_insert_delayed_block()
>>>>>>>      //drop unwritten extent and add delayed extent by mistake
>>>>>>>
>>>>>>> Then, the delalloc extent is wrong until writeback, the one more
>>>>>>> reserved block can't be release any more and trigger below warning:
>>>>>>>
>>>>>>>  EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!
>>>>>>>
>>>>>>> Hold i_data_sem in write mode directly can fix the problem, but it's
>>>>>>> expansive, we should keep the lockless check and check the extent again
>>>>>>> once we need to add an new delalloc block.
>>>>>>
>>>>>> Hi Zhang, 
>>>>>>
>>>>>> It's a nice finding. I was wondering if this was caught in any of the
>>>>>> xfstests?
>>>>>>
>>>>
>>>> Hi, Ritesh
>>>>
>>>> I caught this issue when I tested my iomap series in generic/344 and
>>>> generic/346. It's easy to reproduce because the iomap's buffered write path
>>>> doesn't hold folio lock while inserting delalloc blocks, so it could be raced
>>>> by the mmap page fault path. But the buffer_head's buffered write path can't
>>>> trigger this problem,
>>>
>>> ya right! That's the difference between how ->map_blocks() is called
>>> between buffer_head v/s iomap path. In iomap the ->map_blocks() call
>>> happens first to map a large extent and then it iterate over all the
>>> locked folios covering the mapped extent for doing writes.
>>> Whereas in buffer_head while iterating, we first instantiate/lock the
>>> folio and then call ->map_blocks() to map an extent for the given folio.
>>>
>>> ... So this opens up this window for a race between iomap buffered write
>>> path v/s page mkwrite path for inserting delalloc blocks entries.
>>>
>>>> the race between buffered write path and fallocate path
>>>> was discovered while I was analyzing the code, so I'm not sure if it could
>>>> be caught by xfstests now, at least I haven't noticed this problem so far.
>>>>
>>>
>>> Did you mean the race between page fault path and fallocate path here?
>>> Because buffered write path and fallocate path should not have any race
>>> since both takes the inode_lock. I guess you meant page fault path and
>>> fallocate path for which you wrote this patch too :)
>>
>> Yep.
>>
>>>
>>> I am surprised, why we cannot see the this race between page mkwrite and
>>> fallocate in fstests for inserting da entries to extent status cache.
>>> Because the race you identified looks like a legitimate race and is
>>> mostly happening since ext4_da_map_blocks() was not doing the right
>>> thing.
>>> ... looking at the src/holetest, it doesn't really excercise this path.
>>> So maybe we can writing such fstest to trigger this race.
>>>
>>
>> I guess the stress tests and smoke tests in fstests have caught it,
>> e.g. generic/476. Since there is only one error message in ext4_destroy_inode()
>> when the race issue happened, we can't detect it unless we go and check the logs
>> manually.
> 
> Hi Zhang,
> 
> I wasn't able to reproduce the any error messages with generic/476.
> 
>>
>> I suppose we need to add more warnings, something like this, how does it sound?
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index c8b691e605f1..4b6fd9b63b12 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -1255,6 +1255,8 @@ static void ext4_percpu_param_destroy(struct ext4_sb_info *sbi)
>>  	percpu_counter_destroy(&sbi->s_freeclusters_counter);
>>  	percpu_counter_destroy(&sbi->s_freeinodes_counter);
>>  	percpu_counter_destroy(&sbi->s_dirs_counter);
>> +	WARN_ON_ONCE(!ext4_forced_shutdown(sbi->s_sb) &&
>> +		     percpu_counter_sum(&sbi->s_dirtyclusters_counter));
>>  	percpu_counter_destroy(&sbi->s_dirtyclusters_counter);
>>  	percpu_counter_destroy(&sbi->s_sra_exceeded_retry_limit);
>>  	percpu_free_rwsem(&sbi->s_writepages_rwsem);
>> @@ -1476,7 +1478,8 @@ static void ext4_destroy_inode(struct inode *inode)
>>  		dump_stack();
>>  	}
>>
>> -	if (EXT4_I(inode)->i_reserved_data_blocks)
>> +	if (!ext4_forced_shutdown(inode->i_sb) &&
>> +	    WARN_ON_ONCE(EXT4_I(inode)->i_reserved_data_blocks))
>>  		ext4_msg(inode->i_sb, KERN_ERR,
>>  			 "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
>>  			 inode->i_ino, EXT4_I(inode),
>>
> 
> I also ran ext4 -g auto and I couldn't reproduce anything with above
> patch. Please note that I didn't use this patch series for testing. I was running
> xfstests on upstream kernel with above diff (because that's what the
> idea was that the problem even exists in upstream kernel and are we able
> to observe the race with page mkwrite and fallocate path)
> 

I also ran fstests -g smoke about 2 days and I couldn't reproduce this issue too,
even if I modified generic/476 fstress to only run mmap write and fallocate. It's
pretty hard to reproduce this issue through stress tests. Now, it could only be
reproduced on my machine if I add a strategic delay in ext4_da_map_blocks()
before holding i_data_sem in write mode, but ext4's error injection infrastructure
doesn't support adding delay like xfs. So I guess there still has a lot of work
to do if we want to reproduce it reliably on fstests.

Thanks,
Yi.

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Dave Chinner]

On Fri, Apr 26, 2024 at 10:09:22PM +0530, Ritesh Harjani wrote:
> Zhang Yi <yi.zhang@huaweicloud.com> writes:
> 
> > On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
> >> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
> >> 
> >>> Zhang Yi <yi.zhang@huaweicloud.com> writes:
> >>>
> >>>> From: Zhang Yi <yi.zhang@huawei.com>
> >>>>
> >>>> Now we lookup extent status entry without holding the i_data_sem before
> >>>> inserting delalloc block, it works fine in buffered write path and
> >>>> because it holds i_rwsem and folio lock, and the mmap path holds folio
> >>>> lock, so the found extent locklessly couldn't be modified concurrently.
> >>>> But it could be raced by fallocate since it allocate block whitout
> >>>> holding i_rwsem and folio lock.
> >>>>
> >>>> ext4_page_mkwrite()             ext4_fallocate()
> >>>>  block_page_mkwrite()
> >>>>   ext4_da_map_blocks()
> >>>>    //find hole in extent status tree
> >>>>                                  ext4_alloc_file_blocks()
> >>>>                                   ext4_map_blocks()
> >>>>                                    //allocate block and unwritten extent
> >>>>    ext4_insert_delayed_block()
> >>>>     ext4_da_reserve_space()
> >>>>      //reserve one more block
> >>>>     ext4_es_insert_delayed_block()
> >>>>      //drop unwritten extent and add delayed extent by mistake
> >>>>
> >>>> Then, the delalloc extent is wrong until writeback, the one more
> >>>> reserved block can't be release any more and trigger below warning:
> >>>>
> >>>>  EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!
> >>>>
> >>>> Hold i_data_sem in write mode directly can fix the problem, but it's
> >>>> expansive, we should keep the lockless check and check the extent again
> >>>> once we need to add an new delalloc block.
> >>>
> >>> Hi Zhang, 
> >>>
> >>> It's a nice finding. I was wondering if this was caught in any of the
> >>> xfstests?
> >>>
> >
> > Hi, Ritesh
> >
> > I caught this issue when I tested my iomap series in generic/344 and
> > generic/346. It's easy to reproduce because the iomap's buffered write path
> > doesn't hold folio lock while inserting delalloc blocks, so it could be raced
> > by the mmap page fault path. But the buffer_head's buffered write path can't
> > trigger this problem,
> 
> ya right! That's the difference between how ->map_blocks() is called
> between buffer_head v/s iomap path. In iomap the ->map_blocks() call
> happens first to map a large extent and then it iterate over all the
> locked folios covering the mapped extent for doing writes.

Yes - a fundamental property of the iomap is that it is cached
filesystem state that isn't protected by locks in any way. It can
become stale if a concurrent operation modifies the extent map whilst
the write operation is progressing.

Have a look at iomap_begin_write(). Specifically:

	/*
	 * Now we have a locked folio, before we do anything with it we need to
	 * check that the iomap we have cached is not stale. The inode extent
	 * mapping can change due to concurrent IO in flight (e.g.
	 * IOMAP_UNWRITTEN state can change and memory reclaim could have
	 * reclaimed a previously partially written page at this index after IO
	 * completion before this write reaches this file offset) and hence we
	 * could do the wrong thing here (zero a page range incorrectly or fail
	 * to zero) and corrupt data.
	 */
	if (folio_ops && folio_ops->iomap_valid) {
		bool iomap_valid = folio_ops->iomap_valid(iter->inode,
							 &iter->iomap);
		if (!iomap_valid) {
			iter->iomap.flags |= IOMAP_F_STALE;
			status = 0;
			goto out_unlock;
		}
	}

Yup, there's the hook to detect stale cached iomaps.  The struct
iomap has a iomap->validity_cookie in it, which is an opaque cookie
set by the filesytem when it creates the iomap. Here we have locked
the folio so guaranteed exclusive access to this file range, and so
we pass the iomap with it's cookie back to the filesystem to
determine if the iomap is still valid.

XFS uses generation numbers in the extent tree to determine if the
cached iomap is still valid. ANy change to the extent tree bumps the
generation number, and the current generation number is placed in
iomap->validity_cookie when the iomap is created. If the generation
number on the inode extent tree is different to the number held in
the validity_cookie, then the extent tree has changed and the iomap
must be considered stale. The iomap iterator then sees IOMAP_F_STALE
and generates a new iomap for the remaining range of the write
operation.

Writeback has the same issue - the iomap_writepage_ctx caches the
iomap we obtained for the current writeback, and so if something
else changes the extent state while writeback is underway, then that
map is stale and needs to be refetched.

XFS does this by wrapping the iomap_writepage_ctx with a
xfs_writepage_ctx that holds generation numbers so that when
writeback calls iomap_writeback_ops->map_blocks(), it can check that
the cached iomap is still valid, same as we do in
iomap_begin_write().

> Whereas in buffer_head while iterating, we first instantiate/lock the
> folio and then call ->map_blocks() to map an extent for the given folio.
> 
> ... So this opens up this window for a race between iomap buffered write
> path v/s page mkwrite path for inserting delalloc blocks entries.

iomap allows them to to race - the filesystem extent tree needs it's
own internal locking to serialise lookups and modifications of the
extent tree, whilst the data modifications and page cache state
changes are serialised by the folio lock. That's why
iomap_begin_write() checks that the iomap is still valid only after
it has a locked folio it is ready to write data into.

Remeber that delalloc extents need to be inserted into the
filesystem internal tree when ->iomap_begin() creates them. Hence
anything that races to write over that same range range will only
create the delalloc extent once - the second operation will
simply find the existing delalloc extent the first operation
created...

> > the race between buffered write path and fallocate path
> > was discovered while I was analyzing the code, so I'm not sure if it could
> > be caught by xfstests now, at least I haven't noticed this problem so far.
> >
> 
> Did you mean the race between page fault path and fallocate path here?
> Because buffered write path and fallocate path should not have any race
> since both takes the inode_lock. I guess you meant page fault path and
> fallocate path for which you wrote this patch too :)
> 
> I am surprised, why we cannot see the this race between page mkwrite and
> fallocate in fstests for inserting da entries to extent status cache.

Finding workloads that hit these sorts of races reliably
is -real hard-. Read the commit message in commit d7b64041164c
("iomap: write iomap validity checks"), especially this link:

https://lore.kernel.org/linux-xfs/20220817093627.GZ3600936@dread.disaster.area/

And this comment I made in a followup email:

" [....] and it points out that every filesystem using iomap for
multi-page extent maps will need to implement iomap invalidation
detection in some way."

> Because the race you identified looks like a legitimate race and is
> mostly happening since ext4_da_map_blocks() was not doing the right
> thing.
> ... looking at the src/holetest, it doesn't really excercise this path.
> So maybe we can writing such fstest to trigger this race.

We have a regression test that exercises folio_ops->iomap_valid()
functionality: xfs/559.  It uses the XFS error injection
infrastructure to add a strategic delay which we placed in
xfs_iomap_valid() so that we can hold an iomap cached for an
arbitrary period of time to allow writeback and page cache reclaim
to do their stuff to cause the extent map held by the write to
become stale. It also uses ftrace to capture the tracepoint that
tells us that the invalid iomap state was seen and IOMAP_F_STALE
behaviour triggered.

This could be turned into a generic test, but there's a lot of
missing infrastructure bits to do it....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

[RESEND RFC PATCH v4 00/34] ext4: use iomap for regular file's buffered IO path and enable large folio

[Zhang Yi]

Hello!

This is the fourth version of RFC patch series that convert ext4 regular
file's buffered IO path to iomap and enable large folio. I've rebased it
on 6.9-rc3, it also **depends on my xfs/iomap fix series** which has
been reviewed but not merged yet[1]. Compared to the third vesion, this
iteration fixes an issue discovered in current ext4 code, and contains
another two main changes, 1) add bigalloc support and 2) simplify the
updating logic of reserved delalloc data block, both changes could be
sent out as preliminary patch series, besides these, others are some
small code cleanups, performance optimize and commit log improvements.
Please take a look at this series and any comments are welcome.

This series supports ext4 with the default features and mount
options(bigalloc is also supported), doesn't support non-extent(ext3),
inline_data, dax, fs_verity, fs_crypt and data=journal mode, ext4 would
fall back to buffer_head path automatically if you enabled those
features or options. Although it has many limitations now, it can satisfy
the requirements of most common cases and bring a significant performance
benefit for large IOs.

The iomap path would be simpler than the buffer_head path to some extent,
please note that there are 4 major differences:
1. Always allocate unwritten extent for new blocks, it means that it's
   not controlled by dioread_nolock mount option.
2. Since 1, there is no risk of exposing stale data during the append
   write, so we don't need to write back data before metadata, it's time
   to drop 'data = ordered' mode automatically.
3. Since 2, we don't need to reserve journal credits and use reserved
   handle for the extent status conversion during writeback.
4. We could postpone updating the i_disksize to the endio, it could
   avoid exposing zero data during append write and instantaneous power
   failure.

Series details:
Patch 1-9: this is the part 2 preparation series, it fix a problem
first, and makes ext4_insert_delayed_block() call path support inserting
multiple delalloc blocks (also support bigalloc), finally make
ext4_da_map_blocks() buffer_head unaware, I've send it out separately[2]
and hope this could be merged first.

Patch 10-19: this is the part 3 prepartory changes(picked out from my
metadata reservation series[3], these are not a strong dependency
patches, but I'd suggested these could be merged before the iomap
conversion). These patches moves ext4_da_update_reserve_space() to
ext4_es_insert_extent(), and always set EXT4_GET_BLOCKS_DELALLOC_RESERVE
when allocating delalloc blocks, no matter it's from delayed allocate or
non-delayed allocate (fallocate) path, it makes delalloc extents always
delonly. These can make delalloc reservation simpler and cleaner than
before.

Patch 20-34: These patches are the main implements of the buffered IO
iomap conversion, It first introduce a sequence counter for extent
status tree, then add a new iomap aops for read, write, mmap, replace
current buffered_head path. Finally, enable iomap path besides inline
data, non-extent, dax, fs_verity, fs_crypt, defrag and data=journal
mode, if user specify "buffered_iomap" mount option, also enable large
folio. Please look at the following patch for details.

About Tests:
 - Pass kvm-xfstests in auto mode, and the keep running stress tests and
   fault injection tests.
 - A performance tests below (tested on my version 3 series,
   theoretically there won't be much difference in this version).

   Fio tests with psync on my machine with Intel Xeon Gold 6240 CPU
   with 400GB system ram, 200GB ramdisk and 1TB nvme ssd disk.

   == buffer read ==

                  buffer head        iomap + large folio
   type     bs    IOPS    BW(MiB/s)  IOPS    BW(MiB/s)
   ----------------------------------------------------
   hole     4K    565k    2206       811k    3167
   hole     64K   45.1k   2820       78.1k   4879
   hole     1M    2744    2744       4890    4891
   ramdisk  4K    436k    1703       554k    2163
   ramdisk  64K   29.6k   1848       44.0k   2747
   ramdisk  1M    1994    1995       2809    2809
   nvme     4K    306k    1196       324k    1267
   nvme     64K   19.3k   1208       24.3k   1517
   nvme     1M    1694    1694       2256    2256

   == buffer write ==

                                        buffer head  iomap + large folio
   type   Overwrite Sync Writeback bs   IOPS   BW    IOPS   BW
   ------------------------------------------------------------
   cache    N       N    N         4K   395k   1544  415k   1621
   cache    N       N    N         64K  30.8k  1928  80.1k  5005
   cache    N       N    N         1M   1963   1963  5641   5642
   cache    Y       N    N         4K   423k   1652  443k   1730
   cache    Y       N    N         64K  33.0k  2063  80.8k  5051
   cache    Y       N    N         1M   2103   2103  5588   5589
   ramdisk  N       N    Y         4K   362k   1416  307k   1198
   ramdisk  N       N    Y         64K  22.4k  1399  64.8k  4050
   ramdisk  N       N    Y         1M   1670   1670  4559   4560
   ramdisk  N       Y    N         4K   9830   38.4  13.5k  52.8
   ramdisk  N       Y    N         64K  5834   365   10.1k  629
   ramdisk  N       Y    N         1M   1011   1011  2064   2064
   ramdisk  Y       N    Y         4K   397k   1550  409k   1598
   ramdisk  Y       N    Y         64K  29.2k  1827  73.6k  4597
   ramdisk  Y       N    Y         1M   1837   1837  4985   4985
   ramdisk  Y       Y    N         4K   173k   675   182k   710
   ramdisk  Y       Y    N         64K  17.7k  1109  33.7k  2105
   ramdisk  Y       Y    N         1M   1128   1129  1790   1791
   nvme     N       N    Y         4K   298k   1164  290k   1134
   nvme     N       N    Y         64K  21.5k  1343  57.4k  3590
   nvme     N       N    Y         1M   1308   1308  3664   3664
   nvme     N       Y    N         4K   10.7k  41.8  12.0k  46.9
   nvme     N       Y    N         64K  5962   373   8598   537
   nvme     N       Y    N         1M   676    677   1417   1418
   nvme     Y       N    Y         4K   366k   1430  373k   1456
   nvme     Y       N    Y         64K  26.7k  1670  56.8k  3547
   nvme     Y       N    Y         1M   1745   1746  3586   3586
   nvme     Y       Y    N         4K   59.0k  230   61.2k  239
   nvme     Y       Y    N         64K  13.0k  813   21.0k  1311
   nvme     Y       Y    N         1M   683    683   1368   1369
 
TODO
 - Keep on doing stress tests and fixing.
 - Reserve enough space for delalloc metadata blocks and try to drop
   ext4_nonda_switch().
 - First support defrag and then support other more unsupported features
   and mount options.

Changes since v3:
 - Drop the part 1 prepartory patches which have been merged [4].
 - Drop the two iomap patches since I've submitted separately [1].
 - Fix an incorrect reserved delalloc blocks count and incorrect extent
   status cache issue found on current ext4 code.
 - Pick out part 2 prepartory patch series [2], it make
   ext4_insert_delayed_block() call path support inserting multiple
   delalloc blocks (also support bigalloc )and make ext4_da_map_blocks()
   buffer_head unaware.
 - Adjust and simplify the reserved delalloc blocks updating logic,
   preparing for reserving meta data blocks for delalloc.
 - Drop datasync dirty check in ext4_set_iomap() for buffered
   read/write, improves the concurrent performance on small I/Os.
 - Prevent always hold invalid_lock in page_cache_ra_order(), add
   lockless check.
 - Disable iomap path by default since it's experimental new, add a
   mount option "buffered_iomap" to enable it.
 - Some other minor fixes and change log improvements.
Changes since v2:
 - Update patch 1-6 to v3.
 - iomap_zero and iomap_unshare don't need to update i_size and call
   iomap_write_failed(), introduce a new helper iomap_write_end_simple()
   to avoid doing that.
 - Factor out ext4_[ext|ind]_map_blocks() parts from ext4_map_blocks(),
   introduce a new helper ext4_iomap_map_one_extent() to allocate
   delalloc blocks in writeback, which is always under i_data_sem in
   write mode. This is done to prevent the writing back delalloc
   extents become stale if it raced by truncate.
 - Add a lock detection in mapping_clear_large_folios().
Changes since v1:
 - Introduce seq count for iomap buffered write and writeback to protect
   races from extents changes, e.g. truncate, mwrite.
 - Always allocate unwritten extents for new blocks, drop dioread_lock
   mode, and make no distinctions between dioread_lock and
   dioread_nolock.
 - Don't add ditry data range to jinode, drop data=ordered mode, and
   make no distinctions between data=ordered and data=writeback mode.
 - Postpone updating i_disksize to endio.
 - Allow splitting extents and use reserved space in endio.
 - Instead of reimplement a new delayed mapping helper
   ext4_iomap_da_map_blocks() for buffer write, try to reuse
   ext4_da_map_blocks().
 - Add support for disabling large folio on active inodes.
 - Support online defragmentation, make file fall back to buffer_head
   and disable large folio in ext4_move_extents().
 - Move ext4_nonda_switch() in advance to prevent deadlock in mwrite.
 - Add dirty_len and pos trace info to trace_iomap_writepage_map().
 - Update patch 1-6 to v2.

[1] https://lore.kernel.org/linux-xfs/20240320110548.2200662-1-yi.zhang@huaweicloud.com/
[2] https://lore.kernel.org/linux-ext4/20240410034203.2188357-1-yi.zhang@huaweicloud.com/
[3] https://lore.kernel.org/linux-ext4/20230824092619.1327976-1-yi.zhang@huaweicloud.com/
[4] https://lore.kernel.org/linux-ext4/20240105033018.1665752-1-yi.zhang@huaweicloud.com/

Thanks,
Yi.

---
v3: https://lore.kernel.org/linux-ext4/20240127015825.1608160-1-yi.zhang@huaweicloud.com/
v2: https://lore.kernel.org/linux-ext4/20240102123918.799062-1-yi.zhang@huaweicloud.com/
v1: https://lore.kernel.org/linux-ext4/20231123125121.4064694-1-yi.zhang@huaweicloud.com/

Zhang Yi (34):
  ext4: factor out a common helper to query extent map
  ext4: check the extent status again before inserting delalloc block
  ext4: trim delalloc extent
  ext4: drop iblock parameter
  ext4: make ext4_es_insert_delayed_block() insert multi-blocks
  ext4: make ext4_da_reserve_space() reserve multi-clusters
  ext4: factor out check for whether a cluster is allocated
  ext4: make ext4_insert_delayed_block() insert multi-blocks
  ext4: make ext4_da_map_blocks() buffer_head unaware
  ext4: factor out ext4_map_create_blocks() to allocate new blocks
  ext4: optimize the EXT4_GET_BLOCKS_DELALLOC_RESERVE flag set
  ext4: don't set EXTENT_STATUS_DELAYED on allocated blocks
  ext4: let __revise_pending() return newly inserted pendings
  ext4: count removed reserved blocks for delalloc only extent entry
  ext4: update delalloc data reserve spcae in ext4_es_insert_extent()
  ext4: drop ext4_es_delayed_clu()
  ext4: use ext4_map_query_blocks() in ext4_map_blocks()
  ext4: drop ext4_es_is_delonly()
  ext4: drop all delonly descriptions
  ext4: use reserved metadata blocks when splitting extent on endio
  ext4: introduce seq counter for the extent status entry
  ext4: add a new iomap aops for regular file's buffered IO path
  ext4: implement buffered read iomap path
  ext4: implement buffered write iomap path
  ext4: implement writeback iomap path
  ext4: implement mmap iomap path
  ext4: implement zero_range iomap path
  ext4: writeback partial blocks before zeroing out range
  ext4: fall back to buffer_head path for defrag
  ext4: partial enable iomap for regular file's buffered IO path
  filemap: support disable large folios on active inode
  ext4: enable large folio for regular file with iomap buffered IO path
  ext4: don't mark IOMAP_F_DIRTY for buffer write
  ext4: add mount option for buffered IO iomap path

-- 
2.39.2

[PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

Now we lookup extent status entry without holding the i_data_sem before
inserting delalloc block, it works fine in buffered write path and
because it holds i_rwsem and folio lock, and the mmap path holds folio
lock, so the found extent locklessly couldn't be modified concurrently.
But it could be raced by fallocate since it allocate block whitout
holding i_rwsem and folio lock.

ext4_page_mkwrite()             ext4_fallocate()
 block_page_mkwrite()
  ext4_da_map_blocks()
   //find hole in extent status tree
                                 ext4_alloc_file_blocks()
                                  ext4_map_blocks()
                                   //allocate block and unwritten extent
   ext4_insert_delayed_block()
    ext4_da_reserve_space()
     //reserve one more block
    ext4_es_insert_delayed_block()
     //drop unwritten extent and add delayed extent by mistake

Then, the delalloc extent is wrong until writeback, the one more
reserved block can't be release any more and trigger below warning:

 EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!

Hold i_data_sem in write mode directly can fix the problem, but it's
expansive, we should keep the lockless check and check the extent again
once we need to add an new delalloc block.

Cc: stable@vger.kernel.org
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/inode.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 6a41172c06e1..118b0497a954 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1737,6 +1737,7 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 		if (ext4_es_is_hole(&es))
 			goto add_delayed;
 
+found:
 		/*
 		 * Delayed extent could be allocated by fallocate.
 		 * So we need to check it.
@@ -1781,6 +1782,24 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 
 add_delayed:
 	down_write(&EXT4_I(inode)->i_data_sem);
+	/*
+	 * Lookup extents tree again under i_data_sem, make sure this
+	 * inserting delalloc range haven't been delayed or allocated
+	 * whitout holding i_rwsem and folio lock.
+	 */
+	if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) {
+		if (!ext4_es_is_hole(&es)) {
+			up_write(&EXT4_I(inode)->i_data_sem);
+			goto found;
+		}
+	} else if (!ext4_has_inline_data(inode)) {
+		retval = ext4_map_query_blocks(NULL, inode, map);
+		if (retval) {
+			up_write(&EXT4_I(inode)->i_data_sem);
+			return retval;
+		}
+	}
+
 	retval = ext4_insert_delayed_block(inode, map->m_lblk);
 	up_write(&EXT4_I(inode)->i_data_sem);
 	if (retval)
-- 
2.39.2

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Dave Chinner]

On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Now we lookup extent status entry without holding the i_data_sem before
> inserting delalloc block, it works fine in buffered write path and
> because it holds i_rwsem and folio lock, and the mmap path holds folio
> lock, so the found extent locklessly couldn't be modified concurrently.
> But it could be raced by fallocate since it allocate block whitout
> holding i_rwsem and folio lock.
> 
> ext4_page_mkwrite()             ext4_fallocate()
>  block_page_mkwrite()
>   ext4_da_map_blocks()
>    //find hole in extent status tree
>                                  ext4_alloc_file_blocks()
>                                   ext4_map_blocks()
>                                    //allocate block and unwritten extent
>    ext4_insert_delayed_block()
>     ext4_da_reserve_space()
>      //reserve one more block
>     ext4_es_insert_delayed_block()
>      //drop unwritten extent and add delayed extent by mistake

Shouldn't this be serialised by the file invalidation lock?  Hole
punching via fallocate must do this to avoid data use-after-free
bugs w.r.t racing page faults and all the other fallocate ops need
to serialise page faults to avoid page cache level data corruption.
Yet here we see a problem resulting from a fallocate operation
racing with a page fault....

Ah, I see that the invalidation lock is only picked up deep inside
ext4_punch_hole(), ext4_collapse_range(), ext4_insert_range() and
ext4_zero_range(). They all do the same flush, lock, and dio wait
preamble but each do it just a little bit differently. The allocation path does
it just a little bit differently again and does not take the
invalidate lock...

Perhaps the ext4 fallocate code should be factored so that all the
fallocate operations run the same flush, lock and wait code rather
than having 5 slightly different copies of the same code?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Ritesh Harjani]

Dave Chinner <david@fromorbit.com> writes:

> On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>> 
>> Now we lookup extent status entry without holding the i_data_sem before
>> inserting delalloc block, it works fine in buffered write path and
>> because it holds i_rwsem and folio lock, and the mmap path holds folio
>> lock, so the found extent locklessly couldn't be modified concurrently.
>> But it could be raced by fallocate since it allocate block whitout
>> holding i_rwsem and folio lock.
>> 
>> ext4_page_mkwrite()             ext4_fallocate()
>>  block_page_mkwrite()
>>   ext4_da_map_blocks()
>>    //find hole in extent status tree
>>                                  ext4_alloc_file_blocks()
>>                                   ext4_map_blocks()
>>                                    //allocate block and unwritten extent
>>    ext4_insert_delayed_block()
>>     ext4_da_reserve_space()
>>      //reserve one more block
>>     ext4_es_insert_delayed_block()
>>      //drop unwritten extent and add delayed extent by mistake
>
> Shouldn't this be serialised by the file invalidation lock?  Hole
> punching via fallocate must do this to avoid data use-after-free
> bugs w.r.t racing page faults and all the other fallocate ops need
> to serialise page faults to avoid page cache level data corruption.
> Yet here we see a problem resulting from a fallocate operation
> racing with a page fault....

IIUC, fallocate operations which invalidates the page cache contents needs
to take th invalidate_lock in exclusive mode to prevent page fault
operations from loading pages for stale mappings (blocks which were
marked free might get reused). This can cause stale data exposure.

Here the fallocate operation require allocation of unwritten extents and
does not require truncate of pagecache range. So I guess, it is not
strictly necessary to hold the invalidate lock here.
But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation.

I guess we could use the invalidate lock for fallocate operation in ext4
too. However, I think we still require the current patch. The reason is
ext4_da_map_blocks() call here first tries to lookup the extent status
cache w/o any i_data_sem lock in the fastpath. If it finds a hole, it
takes the i_data_sem in write mode and just inserts an entry into extent
status cache w/o re-checking for the same under the exclusive lock. 
...So I believe we still should have this patch which re-verify under
the write lock if whether any other operation has inserted any entry
already or not.


>
> Ah, I see that the invalidation lock is only picked up deep inside
> ext4_punch_hole(), ext4_collapse_range(), ext4_insert_range() and
> ext4_zero_range(). They all do the same flush, lock, and dio wait
> preamble but each do it just a little bit differently. The allocation path does
> it just a little bit differently again and does not take the
> invalidate lock...

Yes, I think it is not stricly required to take invalidate lock in the
allocation path of fallocate. Hence it could expose such a problem which
existed in ext4_da_map_blocks(), right?


>
> Perhaps the ext4 fallocate code should be factored so that all the
> fallocate operations run the same flush, lock and wait code rather
> than having 5 slightly different copies of the same code?

Yes. I agree. These paths can be refactored and if we are doing so, we
may as well just use the invalidate lock as you suggested.

-ritesh

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Dave Chinner]

On Wed, May 01, 2024 at 05:49:50PM +0530, Ritesh Harjani wrote:
> Dave Chinner <david@fromorbit.com> writes:
> 
> > On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote:
> >> From: Zhang Yi <yi.zhang@huawei.com>
> >> 
> >> Now we lookup extent status entry without holding the i_data_sem before
> >> inserting delalloc block, it works fine in buffered write path and
> >> because it holds i_rwsem and folio lock, and the mmap path holds folio
> >> lock, so the found extent locklessly couldn't be modified concurrently.
> >> But it could be raced by fallocate since it allocate block whitout
> >> holding i_rwsem and folio lock.
> >> 
> >> ext4_page_mkwrite()             ext4_fallocate()
> >>  block_page_mkwrite()
> >>   ext4_da_map_blocks()
> >>    //find hole in extent status tree
> >>                                  ext4_alloc_file_blocks()
> >>                                   ext4_map_blocks()
> >>                                    //allocate block and unwritten extent
> >>    ext4_insert_delayed_block()
> >>     ext4_da_reserve_space()
> >>      //reserve one more block
> >>     ext4_es_insert_delayed_block()
> >>      //drop unwritten extent and add delayed extent by mistake
> >
> > Shouldn't this be serialised by the file invalidation lock?  Hole
> > punching via fallocate must do this to avoid data use-after-free
> > bugs w.r.t racing page faults and all the other fallocate ops need
> > to serialise page faults to avoid page cache level data corruption.
> > Yet here we see a problem resulting from a fallocate operation
> > racing with a page fault....
> 
> IIUC, fallocate operations which invalidates the page cache contents needs
> to take th invalidate_lock in exclusive mode to prevent page fault
> operations from loading pages for stale mappings (blocks which were
> marked free might get reused). This can cause stale data exposure.
> 
> Here the fallocate operation require allocation of unwritten extents and
> does not require truncate of pagecache range. So I guess, it is not
> strictly necessary to hold the invalidate lock here.

True, but you can make exactly the same argument for write() vs
fallocate(). Yet this path in ext4_fallocate() locks out 
concurrent write()s and waits for DIOs in flight to drain. What
makes buffered writes triggered by page faults special?

i.e. if you are going to say "we don't need serialisation between
writes and fallocate() allocating unwritten extents", then why is it
still explicitly serialising against both buffered and direct IO and
not just truncate and other fallocate() operations?

> But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation.

Yes, that's the behaviour preallocation has had in XFS since we
introduced the MMAPLOCK almost a decade ago. This was long before
the file_invalidation_lock() was even a glimmer in Jan's eye.

btrfs does the same thing, for the same reasons. COW support makes
extent tree manipulations excitingly complex at times...

> I guess we could use the invalidate lock for fallocate operation in ext4
> too. However, I think we still require the current patch. The reason is
> ext4_da_map_blocks() call here first tries to lookup the extent status
> cache w/o any i_data_sem lock in the fastpath. If it finds a hole, it
> takes the i_data_sem in write mode and just inserts an entry into extent
> status cache w/o re-checking for the same under the exclusive lock. 
> ...So I believe we still should have this patch which re-verify under
> the write lock if whether any other operation has inserted any entry
> already or not.

Yup, I never said the code in the patch is wrong or unnecessary; I'm
commenting on the high level race condition that lead to the bug
beting triggered. i.e. that racing data modification operations with
low level extent manipulations is often dangerous and a potential
source of very subtle, hard to trigger, reproduce and debug issues
like the one reported...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Ritesh Harjani]

Dave Chinner <david@fromorbit.com> writes:

> On Wed, May 01, 2024 at 05:49:50PM +0530, Ritesh Harjani wrote:
>> Dave Chinner <david@fromorbit.com> writes:
>> 
>> > On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote:
>> >> From: Zhang Yi <yi.zhang@huawei.com>
>> >> 
>> >> Now we lookup extent status entry without holding the i_data_sem before
>> >> inserting delalloc block, it works fine in buffered write path and
>> >> because it holds i_rwsem and folio lock, and the mmap path holds folio
>> >> lock, so the found extent locklessly couldn't be modified concurrently.
>> >> But it could be raced by fallocate since it allocate block whitout
>> >> holding i_rwsem and folio lock.
>> >> 
>> >> ext4_page_mkwrite()             ext4_fallocate()
>> >>  block_page_mkwrite()
>> >>   ext4_da_map_blocks()
>> >>    //find hole in extent status tree
>> >>                                  ext4_alloc_file_blocks()
>> >>                                   ext4_map_blocks()
>> >>                                    //allocate block and unwritten extent
>> >>    ext4_insert_delayed_block()
>> >>     ext4_da_reserve_space()
>> >>      //reserve one more block
>> >>     ext4_es_insert_delayed_block()
>> >>      //drop unwritten extent and add delayed extent by mistake
>> >
>> > Shouldn't this be serialised by the file invalidation lock?  Hole
>> > punching via fallocate must do this to avoid data use-after-free
>> > bugs w.r.t racing page faults and all the other fallocate ops need
>> > to serialise page faults to avoid page cache level data corruption.
>> > Yet here we see a problem resulting from a fallocate operation
>> > racing with a page fault....
>> 
>> IIUC, fallocate operations which invalidates the page cache contents needs
>> to take th invalidate_lock in exclusive mode to prevent page fault
>> operations from loading pages for stale mappings (blocks which were
>> marked free might get reused). This can cause stale data exposure.
>> 
>> Here the fallocate operation require allocation of unwritten extents and
>> does not require truncate of pagecache range. So I guess, it is not
>> strictly necessary to hold the invalidate lock here.
>
> True, but you can make exactly the same argument for write() vs
> fallocate(). Yet this path in ext4_fallocate() locks out 
> concurrent write()s and waits for DIOs in flight to drain. What
> makes buffered writes triggered by page faults special?
>
> i.e. if you are going to say "we don't need serialisation between
> writes and fallocate() allocating unwritten extents", then why is it
> still explicitly serialising against both buffered and direct IO and
> not just truncate and other fallocate() operations?
>
>> But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation.
>
> Yes, that's the behaviour preallocation has had in XFS since we
> introduced the MMAPLOCK almost a decade ago. This was long before
> the file_invalidation_lock() was even a glimmer in Jan's eye.
>
> btrfs does the same thing, for the same reasons. COW support makes
> extent tree manipulations excitingly complex at times...
>
>> I guess we could use the invalidate lock for fallocate operation in ext4
>> too. However, I think we still require the current patch. The reason is
>> ext4_da_map_blocks() call here first tries to lookup the extent status
>> cache w/o any i_data_sem lock in the fastpath. If it finds a hole, it
>> takes the i_data_sem in write mode and just inserts an entry into extent
>> status cache w/o re-checking for the same under the exclusive lock. 
>> ...So I believe we still should have this patch which re-verify under
>> the write lock if whether any other operation has inserted any entry
>> already or not.
>
> Yup, I never said the code in the patch is wrong or unnecessary; I'm
> commenting on the high level race condition that lead to the bug
> beting triggered. i.e. that racing data modification operations with
> low level extent manipulations is often dangerous and a potential
> source of very subtle, hard to trigger, reproduce and debug issues
> like the one reported...
>

Yes, thanks for explaining and commenting on the high level design.
It was indeed helpful. And I agree with your comment on, we can refactor
out the common operations from fallocate path and use invalidate lock to
protect against data modification (page fault) and extent manipulation
path (fallocate operations).


-ritesh

Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Zhang Yi]

On 2024/5/2 12:11, Ritesh Harjani (IBM) wrote:
> Dave Chinner <david@fromorbit.com> writes:
> 
>> On Wed, May 01, 2024 at 05:49:50PM +0530, Ritesh Harjani wrote:
>>> Dave Chinner <david@fromorbit.com> writes:
>>>
>>>> On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote:
>>>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>>>
>>>>> Now we lookup extent status entry without holding the i_data_sem before
>>>>> inserting delalloc block, it works fine in buffered write path and
>>>>> because it holds i_rwsem and folio lock, and the mmap path holds folio
>>>>> lock, so the found extent locklessly couldn't be modified concurrently.
>>>>> But it could be raced by fallocate since it allocate block whitout
>>>>> holding i_rwsem and folio lock.
>>>>>
>>>>> ext4_page_mkwrite()             ext4_fallocate()
>>>>>  block_page_mkwrite()
>>>>>   ext4_da_map_blocks()
>>>>>    //find hole in extent status tree
>>>>>                                  ext4_alloc_file_blocks()
>>>>>                                   ext4_map_blocks()
>>>>>                                    //allocate block and unwritten extent
>>>>>    ext4_insert_delayed_block()
>>>>>     ext4_da_reserve_space()
>>>>>      //reserve one more block
>>>>>     ext4_es_insert_delayed_block()
>>>>>      //drop unwritten extent and add delayed extent by mistake
>>>>
>>>> Shouldn't this be serialised by the file invalidation lock?  Hole
>>>> punching via fallocate must do this to avoid data use-after-free
>>>> bugs w.r.t racing page faults and all the other fallocate ops need
>>>> to serialise page faults to avoid page cache level data corruption.
>>>> Yet here we see a problem resulting from a fallocate operation
>>>> racing with a page fault....
>>>
>>> IIUC, fallocate operations which invalidates the page cache contents needs
>>> to take th invalidate_lock in exclusive mode to prevent page fault
>>> operations from loading pages for stale mappings (blocks which were
>>> marked free might get reused). This can cause stale data exposure.
>>>
>>> Here the fallocate operation require allocation of unwritten extents and
>>> does not require truncate of pagecache range. So I guess, it is not
>>> strictly necessary to hold the invalidate lock here.
>>
>> True, but you can make exactly the same argument for write() vs
>> fallocate(). Yet this path in ext4_fallocate() locks out 
>> concurrent write()s and waits for DIOs in flight to drain. What
>> makes buffered writes triggered by page faults special?
>>
>> i.e. if you are going to say "we don't need serialisation between
>> writes and fallocate() allocating unwritten extents", then why is it
>> still explicitly serialising against both buffered and direct IO and
>> not just truncate and other fallocate() operations?
>>
>>> But I see XFS does take IOLOCK_EXCL AND MMAPLOCK_EXCL even for this operation.
>>
>> Yes, that's the behaviour preallocation has had in XFS since we
>> introduced the MMAPLOCK almost a decade ago. This was long before
>> the file_invalidation_lock() was even a glimmer in Jan's eye.
>>
>> btrfs does the same thing, for the same reasons. COW support makes
>> extent tree manipulations excitingly complex at times...
>>
>>> I guess we could use the invalidate lock for fallocate operation in ext4
>>> too. However, I think we still require the current patch. The reason is
>>> ext4_da_map_blocks() call here first tries to lookup the extent status
>>> cache w/o any i_data_sem lock in the fastpath. If it finds a hole, it
>>> takes the i_data_sem in write mode and just inserts an entry into extent
>>> status cache w/o re-checking for the same under the exclusive lock. 
>>> ...So I believe we still should have this patch which re-verify under
>>> the write lock if whether any other operation has inserted any entry
>>> already or not.
>>
>> Yup, I never said the code in the patch is wrong or unnecessary; I'm
>> commenting on the high level race condition that lead to the bug
>> beting triggered. i.e. that racing data modification operations with
>> low level extent manipulations is often dangerous and a potential
>> source of very subtle, hard to trigger, reproduce and debug issues
>> like the one reported...
>>
> 
> Yes, thanks for explaining and commenting on the high level design.
> It was indeed helpful. And I agree with your comment on, we can refactor
> out the common operations from fallocate path and use invalidate lock to
> protect against data modification (page fault) and extent manipulation
> path (fallocate operations).
> 

Yeah, thanks for explanation and suggestion, too. After looking at your
discussion, I also suppose we could refactor a common helper and use the
file invalidation lock for the whole ext4 fallocate path, current code is
too scattered.

Thanks,
Yi.

[RFC PATCH v4 00/34] ext4: use iomap for regular file's buffered IO path and enable large folio

[Zhang Yi]

Hello!

This is the fourth version of RFC patch series that convert ext4 regular
file's buffered IO path to iomap and enable large folio. I've rebased it
on 6.9-rc3, it also **depends on my xfs/iomap fix series** which has
been reviewed but not merged yet[1]. Compared to the third vesion, this
iteration fixes an issue discovered in current ext4 code, and contains
another two main changes, 1) add bigalloc support and 2) simplify the
updating logic of reserved delalloc data block, both changes could be
sent out as preliminary patch series, besides these, others are some
small code cleanups, performance optimize and commit log improvements.
Please take a look at this series and any comments are welcome.

This series supports ext4 with the default features and mount
options(bigalloc is also supported), doesn't support non-extent(ext3),
inline_data, dax, fs_verity, fs_crypt and data=journal mode, ext4 would
fall back to buffer_head path automatically if you enabled those
features or options. Although it has many limitations now, it can satisfy
the requirements of most common cases and bring a significant performance
benefit for large IOs.

The iomap path would be simpler than the buffer_head path to some extent,
please note that there are 4 major differences:
1. Always allocate unwritten extent for new blocks, it means that it's
   not controlled by dioread_nolock mount option.
2. Since 1, there is no risk of exposing stale data during the append
   write, so we don't need to write back data before metadata, it's time
   to drop 'data = ordered' mode automatically.
3. Since 2, we don't need to reserve journal credits and use reserved
   handle for the extent status conversion during writeback.
4. We could postpone updating the i_disksize to the endio, it could
   avoid exposing zero data during append write and instantaneous power
   failure.

Series details:
Patch 1-9: this is the part 2 preparation series, it fix a problem
first, and makes ext4_insert_delayed_block() call path support inserting
multiple delalloc blocks (also support bigalloc), finally make
ext4_da_map_blocks() buffer_head unaware, I've send it out separately[2]
and hope this could be merged first.

Patch 10-19: this is the part 3 prepartory changes(picked out from my
metadata reservation series[3], these are not a strong dependency
patches, but I'd suggested these could be merged before the iomap
conversion). These patches moves ext4_da_update_reserve_space() to
ext4_es_insert_extent(), and always set EXT4_GET_BLOCKS_DELALLOC_RESERVE
when allocating delalloc blocks, no matter it's from delayed allocate or
non-delayed allocate (fallocate) path, it makes delalloc extents always
delonly. These can make delalloc reservation simpler and cleaner than
before.

Patch 20-34: These patches are the main implements of the buffered IO
iomap conversion, It first introduce a sequence counter for extent
status tree, then add a new iomap aops for read, write, mmap, replace
current buffered_head path. Finally, enable iomap path besides inline
data, non-extent, dax, fs_verity, fs_crypt, defrag and data=journal
mode, if user specify "buffered_iomap" mount option, also enable large
folio. Please look at the following patch for details.

About Tests:
 - Pass kvm-xfstests in auto mode, and the keep running stress tests and
   fault injection tests.
 - A performance tests below (tested on my version 3 series,
   theoretically there won't be much difference in this version).

   Fio tests with psync on my machine with Intel Xeon Gold 6240 CPU
   with 400GB system ram, 200GB ramdisk and 1TB nvme ssd disk.

   == buffer read ==

                  buffer head        iomap + large folio
   type     bs    IOPS    BW(MiB/s)  IOPS    BW(MiB/s)
   ----------------------------------------------------
   hole     4K    565k    2206       811k    3167
   hole     64K   45.1k   2820       78.1k   4879
   hole     1M    2744    2744       4890    4891
   ramdisk  4K    436k    1703       554k    2163
   ramdisk  64K   29.6k   1848       44.0k   2747
   ramdisk  1M    1994    1995       2809    2809
   nvme     4K    306k    1196       324k    1267
   nvme     64K   19.3k   1208       24.3k   1517
   nvme     1M    1694    1694       2256    2256

   == buffer write ==

                                        buffer head  iomap + large folio
   type   Overwrite Sync Writeback bs   IOPS   BW    IOPS   BW
   ------------------------------------------------------------
   cache    N       N    N         4K   395k   1544  415k   1621
   cache    N       N    N         64K  30.8k  1928  80.1k  5005
   cache    N       N    N         1M   1963   1963  5641   5642
   cache    Y       N    N         4K   423k   1652  443k   1730
   cache    Y       N    N         64K  33.0k  2063  80.8k  5051
   cache    Y       N    N         1M   2103   2103  5588   5589
   ramdisk  N       N    Y         4K   362k   1416  307k   1198
   ramdisk  N       N    Y         64K  22.4k  1399  64.8k  4050
   ramdisk  N       N    Y         1M   1670   1670  4559   4560
   ramdisk  N       Y    N         4K   9830   38.4  13.5k  52.8
   ramdisk  N       Y    N         64K  5834   365   10.1k  629
   ramdisk  N       Y    N         1M   1011   1011  2064   2064
   ramdisk  Y       N    Y         4K   397k   1550  409k   1598
   ramdisk  Y       N    Y         64K  29.2k  1827  73.6k  4597
   ramdisk  Y       N    Y         1M   1837   1837  4985   4985
   ramdisk  Y       Y    N         4K   173k   675   182k   710
   ramdisk  Y       Y    N         64K  17.7k  1109  33.7k  2105
   ramdisk  Y       Y    N         1M   1128   1129  1790   1791
   nvme     N       N    Y         4K   298k   1164  290k   1134
   nvme     N       N    Y         64K  21.5k  1343  57.4k  3590
   nvme     N       N    Y         1M   1308   1308  3664   3664
   nvme     N       Y    N         4K   10.7k  41.8  12.0k  46.9
   nvme     N       Y    N         64K  5962   373   8598   537
   nvme     N       Y    N         1M   676    677   1417   1418
   nvme     Y       N    Y         4K   366k   1430  373k   1456
   nvme     Y       N    Y         64K  26.7k  1670  56.8k  3547
   nvme     Y       N    Y         1M   1745   1746  3586   3586
   nvme     Y       Y    N         4K   59.0k  230   61.2k  239
   nvme     Y       Y    N         64K  13.0k  813   21.0k  1311
   nvme     Y       Y    N         1M   683    683   1368   1369
 
TODO
 - Keep on doing stress tests and fixing.
 - Reserve enough space for delalloc metadata blocks and try to drop
   ext4_nonda_switch().
 - First support defrag and then support other more unsupported features
   and mount options.

Changes since v3:
 - Drop the part 1 prepartory patches which have been merged [4].
 - Drop the two iomap patches since I've submitted separately [1].
 - Fix an incorrect reserved delalloc blocks count and incorrect extent
   status cache issue found on current ext4 code.
 - Pick out part 2 prepartory patch series [2], it make
   ext4_insert_delayed_block() call path support inserting multiple
   delalloc blocks (also support bigalloc )and make ext4_da_map_blocks()
   buffer_head unaware.
 - Adjust and simplify the reserved delalloc blocks updating logic,
   preparing for reserving meta data blocks for delalloc.
 - Drop datasync dirty check in ext4_set_iomap() for buffered
   read/write, improves the concurrent performance on small I/Os.
 - Prevent always hold invalid_lock in page_cache_ra_order(), add
   lockless check.
 - Disable iomap path by default since it's experimental new, add a
   mount option "buffered_iomap" to enable it.
 - Some other minor fixes and change log improvements.
Changes since v2:
 - Update patch 1-6 to v3.
 - iomap_zero and iomap_unshare don't need to update i_size and call
   iomap_write_failed(), introduce a new helper iomap_write_end_simple()
   to avoid doing that.
 - Factor out ext4_[ext|ind]_map_blocks() parts from ext4_map_blocks(),
   introduce a new helper ext4_iomap_map_one_extent() to allocate
   delalloc blocks in writeback, which is always under i_data_sem in
   write mode. This is done to prevent the writing back delalloc
   extents become stale if it raced by truncate.
 - Add a lock detection in mapping_clear_large_folios().
Changes since v1:
 - Introduce seq count for iomap buffered write and writeback to protect
   races from extents changes, e.g. truncate, mwrite.
 - Always allocate unwritten extents for new blocks, drop dioread_lock
   mode, and make no distinctions between dioread_lock and
   dioread_nolock.
 - Don't add ditry data range to jinode, drop data=ordered mode, and
   make no distinctions between data=ordered and data=writeback mode.
 - Postpone updating i_disksize to endio.
 - Allow splitting extents and use reserved space in endio.
 - Instead of reimplement a new delayed mapping helper
   ext4_iomap_da_map_blocks() for buffer write, try to reuse
   ext4_da_map_blocks().
 - Add support for disabling large folio on active inodes.
 - Support online defragmentation, make file fall back to buffer_head
   and disable large folio in ext4_move_extents().
 - Move ext4_nonda_switch() in advance to prevent deadlock in mwrite.
 - Add dirty_len and pos trace info to trace_iomap_writepage_map().
 - Update patch 1-6 to v2.

[1] https://lore.kernel.org/linux-xfs/20240320110548.2200662-1-yi.zhang@huaweicloud.com/
[2] https://lore.kernel.org/linux-ext4/20240410034203.2188357-1-yi.zhang@huaweicloud.com/
[3] https://lore.kernel.org/linux-ext4/20230824092619.1327976-1-yi.zhang@huaweicloud.com/
[4] https://lore.kernel.org/linux-ext4/20240105033018.1665752-1-yi.zhang@huaweicloud.com/

Thanks,
Yi.

---
v3: https://lore.kernel.org/linux-ext4/20240127015825.1608160-1-yi.zhang@huaweicloud.com/
v2: https://lore.kernel.org/linux-ext4/20240102123918.799062-1-yi.zhang@huaweicloud.com/
v1: https://lore.kernel.org/linux-ext4/20231123125121.4064694-1-yi.zhang@huaweicloud.com/

Zhang Yi (34):
  ext4: factor out a common helper to query extent map
  ext4: check the extent status again before inserting delalloc block
  ext4: trim delalloc extent
  ext4: drop iblock parameter
  ext4: make ext4_es_insert_delayed_block() insert multi-blocks
  ext4: make ext4_da_reserve_space() reserve multi-clusters
  ext4: factor out check for whether a cluster is allocated
  ext4: make ext4_insert_delayed_block() insert multi-blocks
  ext4: make ext4_da_map_blocks() buffer_head unaware
  ext4: factor out ext4_map_create_blocks() to allocate new blocks
  ext4: optimize the EXT4_GET_BLOCKS_DELALLOC_RESERVE flag set
  ext4: don't set EXTENT_STATUS_DELAYED on allocated blocks
  ext4: let __revise_pending() return newly inserted pendings
  ext4: count removed reserved blocks for delalloc only extent entry
  ext4: update delalloc data reserve spcae in ext4_es_insert_extent()
  ext4: drop ext4_es_delayed_clu()
  ext4: use ext4_map_query_blocks() in ext4_map_blocks()
  ext4: drop ext4_es_is_delonly()
  ext4: drop all delonly descriptions
  ext4: use reserved metadata blocks when splitting extent on endio
  ext4: introduce seq counter for the extent status entry
  ext4: add a new iomap aops for regular file's buffered IO path
  ext4: implement buffered read iomap path
  ext4: implement buffered write iomap path
  ext4: implement writeback iomap path
  ext4: implement mmap iomap path
  ext4: implement zero_range iomap path
  ext4: writeback partial blocks before zeroing out range
  ext4: fall back to buffer_head path for defrag
  ext4: partial enable iomap for regular file's buffered IO path
  filemap: support disable large folios on active inode
  ext4: enable large folio for regular file with iomap buffered IO path
  ext4: don't mark IOMAP_F_DIRTY for buffer write
  ext4: add mount option for buffered IO iomap path

-- 
2.39.2

[PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

Now we lookup extent status entry without holding the i_data_sem before
inserting delalloc block, it works fine in buffered write path and
because it holds i_rwsem and folio lock, and the mmap path holds folio
lock, so the found extent locklessly couldn't be modified concurrently.
But it could be raced by fallocate since it allocate block whitout
holding i_rwsem and folio lock.

ext4_page_mkwrite()             ext4_fallocate()
 block_page_mkwrite()
  ext4_da_map_blocks()
   //find hole in extent status tree
                                 ext4_alloc_file_blocks()
                                  ext4_map_blocks()
                                   //allocate block and unwritten extent
   ext4_insert_delayed_block()
    ext4_da_reserve_space()
     //reserve one more block
    ext4_es_insert_delayed_block()
     //drop unwritten extent and add delayed extent by mistake

Then, the delalloc extent is wrong until writeback, the one more
reserved block can't be release any more and trigger below warning:

 EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared!

Hold i_data_sem in write mode directly can fix the problem, but it's
expansive, we should keep the lockless check and check the extent again
once we need to add an new delalloc block.

Cc: stable@vger.kernel.org
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/inode.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 6a41172c06e1..118b0497a954 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1737,6 +1737,7 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 		if (ext4_es_is_hole(&es))
 			goto add_delayed;
 
+found:
 		/*
 		 * Delayed extent could be allocated by fallocate.
 		 * So we need to check it.
@@ -1781,6 +1782,24 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 
 add_delayed:
 	down_write(&EXT4_I(inode)->i_data_sem);
+	/*
+	 * Lookup extents tree again under i_data_sem, make sure this
+	 * inserting delalloc range haven't been delayed or allocated
+	 * whitout holding i_rwsem and folio lock.
+	 */
+	if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) {
+		if (!ext4_es_is_hole(&es)) {
+			up_write(&EXT4_I(inode)->i_data_sem);
+			goto found;
+		}
+	} else if (!ext4_has_inline_data(inode)) {
+		retval = ext4_map_query_blocks(NULL, inode, map);
+		if (retval) {
+			up_write(&EXT4_I(inode)->i_data_sem);
+			return retval;
+		}
+	}
+
 	retval = ext4_insert_delayed_block(inode, map->m_lblk);
 	up_write(&EXT4_I(inode)->i_data_sem);
 	if (retval)
-- 
2.39.2