linux-kernel - Re: [PATCH v2 03/10] ext4: don't set EXTENT_STATUS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240807133719.pjxlhfx25rfqiuul@quack3>
Date: Wed, 7 Aug 2024 15:37:19 +0200
From: Jan Kara <jack@...e.cz>
To: Zhang Yi <yi.zhang@...weicloud.com>
Cc: Jan Kara <jack@...e.cz>, 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 Wed 07-08-24 20:18:18, Zhang Yi wrote:
> 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 ?

Now you've got me confused again ;) Why I wrote the changelog that way is
because ext4_es_remove_extent() is calling ext4_da_release_space(). But now
I've realized I've confused ext4_es_remove_extent() with
__es_remove_extent() which is what gets called when inserting another
extent. So I was wrong and indeed your version of the last sentense is
correct. Thanks for catching this!

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR