Received-SPF: pass (google.com: domain of linux-ext4+bounces-2199-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 26 Apr 2024 18:01:33 +0530
Message-Id: <87frv8nw4a.fsf@gmail.com>
From: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To: Zhang Yi <yi.zhang@huaweicloud.com>, linux-ext4@vger.kernel.org
Cc: linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, tytso@mit.edu, adilger.kernel@dilger.ca, jack@suse.cz, hch@infradead.org, djwong@kernel.org, david@fromorbit.com, willy@infradead.org, zokeefe@google.com, yi.zhang@huawei.com, yi.zhang@huaweicloud.com, chengzhihao1@huawei.com, yukuai3@huawei.com, wangkefeng.wang@huawei.com
Subject: Re: [PATCH v4 02/34] ext4: check the extent status again before inserting delalloc block
In-Reply-To: <20240410142948.2817554-3-yi.zhang@huaweicloud.com>
Precedence: bulk

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.

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.


> +	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