[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <685055bc-0d56-6cf3-7716-f27e448c8c38@huaweicloud.com>
Date: Wed, 7 Aug 2024 20:18:18 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
ritesh.list@...il.com, yi.zhang@...wei.com, chengzhihao1@...wei.com,
yukuai3@...wei.com
Subject: Re: [PATCH v2 03/10] ext4: don't set EXTENT_STATUS_DELAYED on
allocated blocks
On 2024/8/6 23:23, Jan Kara wrote:
> On Fri 02-08-24 19:51:13, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@...wei.com>
>>
>> Since we always set EXT4_GET_BLOCKS_DELALLOC_RESERVE when allocating
>> delalloc blocks, there is no need to keep delayed flag on the unwritten
>> extent status entry, so just drop it after allocation.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
>
> Let me improve the changelog because I was confused for some time before I
> understood:
>
> Currently, we release delayed allocation reservation when removing delayed
> extent from extent status tree (which also happens when overwriting one
> extent with another one). When we allocated unwritten extent under
> some delayed allocated extent, we don't need the reservation anymore and
> hence we don't need to preserve the EXT4_MAP_DELAYED status bit. Inserting
> the new extent into extent status tree will properly release the
> reservation.
>
Thanks for your review and change log improvement. My original idea was very
simple, after patch 2, we always set EXT4_GET_BLOCKS_DELALLOC_RESERVE when
allocating blocks for delalloc extent, these two conditions in the 'if'
branch can never be true at the same time, so they become dead code and I
dropped them.
if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) &&
ext4_es_scan_range(inode, &ext4_es_is_delayed, ...)
But after thinking your change log, I agree with you that we have already
properly update the reservation by searching delayed blocks through
ext4_es_delayed_clu() in ext4_ext_map_blocks() when we allocated unwritten
extent under some delayed allocated extent even it's not from the write
back path, so I think we can also drop them even without patch 2. But just
one point, I think the last last sentence isn't exactly true before path 6,
should it be "Allocating the new extent blocks will properly release the
reservation." now ?
Thanks,
Yi.
> Otherwise feel free to add:
>
> Reviewed-by: Jan Kara <jack@...e.cz>
>
> Honza
>
>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 91b2610a6dc5..e9ce1e4e6acb 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -558,12 +558,6 @@ static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
>>
>> status = map->m_flags & EXT4_MAP_UNWRITTEN ?
>> EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>> - if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) &&
>> - !(status & EXTENT_STATUS_WRITTEN) &&
>> - ext4_es_scan_range(inode, &ext4_es_is_delayed, map->m_lblk,
>> - map->m_lblk + map->m_len - 1))
>> - status |= EXTENT_STATUS_DELAYED;
>> -
>> ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
>> map->m_pblk, status);
>>
>> @@ -682,11 +676,6 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>>
>> status = map->m_flags & EXT4_MAP_UNWRITTEN ?
>> EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>> - if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) &&
>> - !(status & EXTENT_STATUS_WRITTEN) &&
>> - ext4_es_scan_range(inode, &ext4_es_is_delayed, map->m_lblk,
>> - map->m_lblk + map->m_len - 1))
>> - status |= EXTENT_STATUS_DELAYED;
>> ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
>> map->m_pblk, status);
>> }
>> --
>> 2.39.2
>>
Powered by blists - more mailing lists