From: Zhang Yi <[email protected]>
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: [email protected]
Signed-off-by: Zhang Yi <[email protected]>
---
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
Zhang Yi <[email protected]> writes:
> From: Zhang Yi <[email protected]>
>
> 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.
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: [email protected]
> Signed-off-by: Zhang Yi <[email protected]>
> ---
> 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.
> + 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
Ritesh Harjani (IBM) <[email protected]> writes:
> Zhang Yi <[email protected]> writes:
>
>> From: Zhang Yi <[email protected]>
>>
>> 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: [email protected]
>> Signed-off-by: Zhang Yi <[email protected]>
>> ---
>> 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
On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
> Ritesh Harjani (IBM) <[email protected]> writes:
>
>> Zhang Yi <[email protected]> writes:
>>
>>> From: Zhang Yi <[email protected]>
>>>
>>> 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: [email protected]
>>> Signed-off-by: Zhang Yi <[email protected]>
>>> ---
>>> 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
>
Zhang Yi <[email protected]> writes:
> On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
>> Ritesh Harjani (IBM) <[email protected]> writes:
>>
>>> Zhang Yi <[email protected]> writes:
>>>
>>>> From: Zhang Yi <[email protected]>
>>>>
>>>> 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 :)
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 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
On 2024/4/27 0:39, Ritesh Harjani (IBM) wrote:
> Zhang Yi <[email protected]> writes:
>
>> On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
>>> Ritesh Harjani (IBM) <[email protected]> writes:
>>>
>>>> Zhang Yi <[email protected]> writes:
>>>>
>>>>> From: Zhang Yi <[email protected]>
>>>>>
>>>>> 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
>
Zhang Yi <[email protected]> writes:
> On 2024/4/27 0:39, Ritesh Harjani (IBM) wrote:
>> Zhang Yi <[email protected]> writes:
>>
>>> On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
>>>> Ritesh Harjani (IBM) <[email protected]> writes:
>>>>
>>>>> Zhang Yi <[email protected]> writes:
>>>>>
>>>>>> From: Zhang Yi <[email protected]>
>>>>>>
>>>>>> 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
>>
Dave Chinner <[email protected]> writes:
> On Wed, May 01, 2024 at 05:49:50PM +0530, Ritesh Harjani wrote:
>> Dave Chinner <[email protected]> writes:
>>
>> > On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote:
>> >> From: Zhang Yi <[email protected]>
>> >>
>> >> 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
On 2024/5/2 12:11, Ritesh Harjani (IBM) wrote:
> Dave Chinner <[email protected]> writes:
>
>> On Wed, May 01, 2024 at 05:49:50PM +0530, Ritesh Harjani wrote:
>>> Dave Chinner <[email protected]> writes:
>>>
>>>> On Wed, Apr 10, 2024 at 10:29:16PM +0800, Zhang Yi wrote:
>>>>> From: Zhang Yi <[email protected]>
>>>>>
>>>>> 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.
On 2024/4/29 22:59, Ritesh Harjani (IBM) wrote:
> Zhang Yi <[email protected]> writes:
>
>> On 2024/4/27 0:39, Ritesh Harjani (IBM) wrote:
>>> Zhang Yi <[email protected]> writes:
>>>
>>>> On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote:
>>>>> Ritesh Harjani (IBM) <[email protected]> writes:
>>>>>
>>>>>> Zhang Yi <[email protected]> writes:
>>>>>>
>>>>>>> From: Zhang Yi <[email protected]>
>>>>>>>
>>>>>>> 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.