Message-Id: <E1C33FC3-CEF7-458E-AC1F-FAA3223D2CBB@dilger.ca>
Date:   Mon, 14 Aug 2023 22:32:00 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Li Dongyang <dongyangli@....com>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Shuichi Ihara <sihara@....com>, wangshilong1991@...il.com
Subject: Re: [PATCH 1/2] ext4: introduce EXT4_BG_TRIMMED to optimize fstrim

On Aug 11, 2023, at 12:35 PM, Theodore Ts'o <tytso@....edu> wrote:
> 
> On Fri, Aug 11, 2023 at 04:19:04PM +1000, Li Dongyang wrote:
>> Currently the flag indicating block group has done fstrim is not
>> persistent, and trim status will be lost after remount, as
>> a result fstrim can not skip the already trimmed groups, which
>> could be slow on very large devices.
>> 
>> This patch introduces a new block group flag EXT4_BG_TRIMMED,
>> we need 1 extra block group descriptor write after trimming each
>> block group.
>> When clearing the flag, the block group descriptor is journalled
>> already so no extra overhead.
> 
> If we journalling is enabled (and it normally is enabled) then there
> is also writes to the journalling.  Updating the block group
> descriptor is also a random 4k write, which is not nothing.  So if we
> are going to do this, then we should not try to set the flag if the
> block group is unitialized, and we should actually send the discard in
> that case, since presumably the blocks in question were discard when
> the file system was mkfs'ed.

Sorry Ted, I'm not sure I understand your comment here.  If the device
is trimmed at mke2fs time, then the BG_TRIMMED flags are set in the
group descriptors, so if the flag is still set then no need to TRIM
those groups later.

The comment about "no extra overhead" is in the case of clearing the
BG_TRIMMED flag when freeing blocks.  In that case, the group descriptors
are already being updated with the new blocks count, so there is no
overhead to clear the BG_TRIMMED flag at the same time.

Definitely there is an extra GDT write after TRIM to set the BG_TRIMMED
flag, but since fstrim is done sequentially for groups it is likely that
multiple groups in a single GDT block would be updated at the same time,
so the overhead is relatively small.

>> Add a new super block flag EXT2_FLAGS_TRACK_TRIM, to indicate if
>> we should honour EXT4_BG_TRIMMED when doing fstrim.
>> The new super block flag can be turned on/off via tune2fs.
> 
> I don't see the point of having the superblock flag.  It seems to me
> that either we should either do this via a proper feature flag, which
> means that older kernels (and grub bootloaders that get release
> updates at a super-lackadasical pace) won't touch file systems that
> have the feature flag set --- or we don't have any kind of flag at
> all, and kernels and userspace utilities which are EXT4_BG_TRIMMED
> enlightened will honor and set/clear the flag.

In the previous email thread about the persistent BG_TRIMMED flag,
you were requesting a superblock flag and not a full feature, to avoid
the incompatibility issues with a new feature for this:

https://patchwork.ozlabs.org/project/linux-ext4/patch/1592831677-13945-1-git-send-email-wangshilong1991@gmail.com/#2502168

   "So what I was thinking was we could define a new flag which
    would be set in es->s_flags in the on-disk superblock:

    #define EXT2_FLAGS_PERSISTENT_TRIM_TRACKING 0x0008

    If this flag is set, then the EXT4_BG_WAS_TRIMMED flags will
    be honored; otherwise, they will be ignored when FITRIM is
    executed and the block group will be unconditionally trimmed.

    The advantage of doing it this way is that we don't need to
    allocate a new feature bit, and older versions of e2fsck won't
    have heartburn over seeing a feature bit it doesn't understand.
    I also suspect this is something that the system administrator
    will either always want enabled or disabled, so it's better to
    make it be a tunable to be set via tune2fs."

> This risk if we go down that path is that if we have a file system
> which is normally used by a kernel that has support for this feature,
> and that file system is mounted by an older kernel which doesn't have
> this flag, there might be cases where the file system would be trimmed
> without setting these flags, or blocks might get released on a block
> group without clearing the flag.  Fortunately, trim is advisory, so if
> we trim a block group that doesn't need it, or we don't trim a block
> group where discard might be useful, it's not the end of the world.
> And we could always have "e2fsck -E discard" ignore the
> EXT4_BG_TRIMMED flag, and just trim all the blocks[1].

I'm OK with the superblock flag.  Since TRIM is advisory you aren't
going to lose data or corrupt the filesystem if the flags are wrong.
At worst, some TRIM will be skipped until upgrading to a new kernel
or the flag is disabled in the superblock, but this is a corner case.

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists