linux-kernel - Re: [RESEND v2] ext4: Optimization of no-op ext4

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] [day] [month] [year] [list]

Message-ID: <CAJcXncXSLsHah+7KvtEHK-Y0xD5K-b3yCSAFiYfibmmZnc4P1Q@mail.gmail.com>
Date: Tue, 12 Nov 2024 17:44:11 +0200
From: Max Brener <linmaxi@...il.com>
To: "Theodore Ts'o" <tytso@....edu>
Cc: adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RESEND v2] ext4: Optimization of no-op ext4_truncate triggers

‫בתאריך יום ה׳, 7 בנוב׳ 2024 ב-7:03 מאת ‪Theodore Ts'o‬‏ <‪tytso@....edu‬‏>:‬
>
> On Wed, Oct 16, 2024 at 02:16:24PM +0300, Max Brener wrote:
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219306
> > v1: https://lore.kernel.org/lkml/20240926221103.24423-1-linmaxi@gmail.com/T/
> >
> > Changes from last version: Moved vfs-level changes to be ext4-level,
> > and improved the description of the patch.
> >
> > This patch enables skipping no-op 'ext4_truncate' calls. Analyzing the kernel
> > with ftrace shows ext4_truncate is being sometimes called without making any
> > impact, and sometimes userspace programs might call ext4_truncate in vein. By
> > detecting these calls and skipping them, cpu time is saved.
> >
> > I'll fix this by skipping ext4_truncate call in 'ext4_setattr' when the file's size
> > hasn't changed AND it hasn't been truncated since the last disk space preallocation.
> > It is meant to consider the case when ext4_truncate is being called to truncate
> > preallocated blocks too. Notice that so far, the condition to triggering
> > ext4_truncate by the user was: if (attr->ia_size <= oldsize) which means it is
> > being triggered when attr->ia_size == oldsize regardless of whether there are
> > preallocated blocks or not - if there are none, then the call is redundant.
> >
> > Steps:
> > 1.Add a new inode state flag: EXT4_STATE_TRUNCATED
> > 2.Clear the flag when ext4_fallocate is being called with FALLOC_FL_KEEP_SIZE flag
> > to enable using ext4_truncate again, to remove preallocated disk space that may
> > have resulted from this call.
> > 3.Set EXT4_STATE_TRUNCATED when ext4_truncated is called successfully.
> > 4.Don't skip ext4_truncate in ext4_setattr when the size of the file has either been
> > reduced OR stayed the same, but hasn't been truncated yet. This is in order to allow
> > truncating of preallocated blocks.
>
> This patch is still not quite right.  See Jan's comment from [1]:
>
>    Agreed as well. I'll also note that keeping such flag uptodate is not as
>    simple as it seems because there are various places that may be allocating
>    blocks beyond EOF (for example extending writes) and that rely on
>    ext4_truncate() removing them so one needs to be careful to capture all the
>    places where the "truncated" state needs to be cleared.
>
> [1] https://lore.kernel.org/all/20240930095601.x66iqw74bxffytgq@quack3/
>
>                                                 - Ted

Okay I understand now, I initially thought any preallocation is necessarily done
through a VFS interface. Now that I see preallocations are done at mballoc,
what I can offer is to clear the TRUNCATED flag at ext4_mb_new_blocks().
Would that be ok in your opinion?