linux-ext4 - Re: [PATCH 2/2] ext4: skip non-loaded groups at cr=0/1

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: <914597DA-395A-47A5-A8D6-DFCE2D674289@whamcloud.com>
Date:   Fri, 15 May 2020 08:56:42 +0000
From:   Alex Zhuravlev <azhuravlev@...mcloud.com>
To:     Ritesh Harjani <riteshh@...ux.ibm.com>
CC:     Alex Zhuravlev <azhuravlev@...mcloud.com>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 2/2] ext4: skip non-loaded groups at cr=0/1

Thanks for the review, answers inline..


> On 14 May 2020, at 13:04, Ritesh Harjani <riteshh@...ux.ibm.com> wrote:
> Not needed in a commit msg.

OK

> 
>> cr=0 is supposed to be an optimization to save CPU cycles, but if buddy data (in memory)
>> is not initialized then all this makes no sense as we have to do sync IO taking a lot of cycles.
>> also, at cr=0 mballoc doesn't store any avaibale chunk. cr=1 also skips groups using heuristic
> /s/avaibale/available/

OK

> 
>> based on avg. fragment size. it's more useful to skip such groups and switch to cr=2 where
>> groups will be scanned for available chunks.
>> using sparse image and dm-slow virtual device of 120TB was simulated. then the image was
>> formatted and filled using debugfs to mark ~85% of available space as busy. mount process w/o
>> the patch couldn't complete in half an hour (according to vmstat it would take ~10-11 hours).
>> with the patch applied mount took ~20 seconds.
> 
> I guess what we should edit the commit msg to explain that it is not the
> mount process but the very first write whose performance is improved via
> this patch.

Correct

>> +		/* cr=0/1 is a very optimistic search to find large
>> +		 * good chunks almost for free. if buddy data is
>> +		 * not ready, then this optimization makes no sense */
> 
> I guess it will be also helpful to mention a comment related to the
> discussion that we had on why this should be ok to skip those groups.
> Because this could result into we skipping the group which is closer to
> our inode. I somehow couldn't recollect it completely.

Please remind where the discussion took place? I must be missing that.