linux-kernel - Re: [PATCH V2] iomap: skip unnecessary ifs_block_is

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: <C11672FE2A6808C2+0da2e5a1-440e-4bd4-bf69-e9086d45ce42@uniontech.com>
Date: Thu, 10 Apr 2025 14:25:19 +0800
From: Gou Hao <gouhao@...ontech.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: brauner@...nel.org, djwong@...nel.org, gouhaojake@....com,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-xfs@...r.kernel.org, wangyuli@...ontech.com
Subject: Re: [PATCH V2] iomap: skip unnecessary ifs_block_is_uptodate check


On 2025/4/10 13:54, Christoph Hellwig wrote:
> On Thu, Apr 10, 2025 at 01:42:23PM +0800, Gou Hao wrote:
>> prior to the loop, $i is either the first !uptodate block, or
>> it's past $last.  Assuming there's no overflow (there's no combination
>> of huge folios and tiny blksize) then yeah, there's no point in
>> retesting that the same block $i is uptodate since we hold the folio
>> lock so nobody else could have set uptodate.
> Capitalize the first word in the sentence and use up the 73 characters
> available for the commit log:
>
> In iomap_adjust_read_range, i is either the first !uptodate block, or it
> is past last for the second loop looking for trailing uptodate blocks.
> Assuming there's no overflow (there's no combination of huge folios and
> tiny blksize) then yeah, there is no point in retesting that the same
> block pointed to by i is uptodate since we hold the folio lock so nobody
> else could have set it uptodate.
Thank you, i will change the log in next patch.
>>   		/* truncate len if we find any trailing uptodate block(s) */
>> -		for ( ; i <= last; i++) {
>> +		for (i++; i <= last; i++) {
> A bit nitpicky, but I find a i++ in the initialization condition of a
> for loop a bit odd.
>
> What about turning this into a:
>
> 		while (++i <= last) {
>
> ?
Yes,  it is better.  I will test this.
>