MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Fri, 17 Dec 2021 15:09:50 +0000
From:   Jeroen van Wolffelaar <jeroen@wolffelaar.nl>
To:     Theodore Ts'o <tytso@mit.edu>
Cc:     Lukas Czerner <lczerner@redhat.com>, Jan Kara <jack@suse.cz>,
        "Darrick J. Wong" <djwong@kernel.org>,
        =?UTF-8?Q?Lu=C3=ADs_Henriques?= <lhenriques@suse.de>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] ext4: set csum seed in tmp inode while migrating to
 extents
In-Reply-To: <YbuGLsQy6TSM2xOl@mit.edu>
References: <20211214175058.19511-1-lhenriques@suse.de>
 <20211215004945.GD69182@magnolia> <20211215112852.GM14044@quack2.suse.cz>
 <20211215141237.lrymhbebgjunh4n2@work> <YbuGLsQy6TSM2xOl@mit.edu>
Message-ID: <f8a4352c32b1c6d4407f6d1b99a2c43c@wolffelaar.nl>
User-Agent: Roundcube Webmail/1.3.17
Precedence: bulk

First of all, thanks Luís for figuring out the bug and writing a patch.

On 2021-12-16 18:32, Theodore Ts'o wrote:
> So the question is, is it worth it to continue supporting the migrate
> feature, or should we just delete all of the migration code, and risk
> users complaining that we've broken their use case?  The chances of
> that happening is admittedly low, and Linus's rule that "it's only
> breaking userspace if a user complains" means we might very well get
> away with it.  :-)

As the person who ran into this bug and then filed the issue in
bugzilla, my two cents:

I can do without the migration functionality, it is slightly
inconvenient, but that's it. I do believe having an always-broken
migration path (HEAD kernel, if csum is on) is much worse than not
having one. I don't personally care if a crash means I lose one or
several files, but I feel it's really not okay for chattr to risk losing
a file in a crash on a journalled filesystem. I would consider that a
severe data loss bug.

Thus, I support removing this feature. At the same time, I believe the
original patch is strictly better than the existing situation, so I also
support adding that to the kernel so that in the happy case, this
doesn't cause e2fsck failures, pending the (much longer) deprecation
path.


In case anyone cares, my usecase, so you can see it's quite esoteric.
You can also stop reading, I believe that's better :-P.

I have a rotation of harddisks that I use to have offline backups. They
were ext3 until the day I ran into this bug, I finally got around to
upgrading the filesystems (I never wanted to upgrade the instant ext4
became available, but arguably this is somewhat late too).

Because I knew -- out of prior interest -- that ext3 blocklists did not
have checksums, I wanted to ensure all existing files were csum too, so
extents. chattr +e promised to do exactly that. Well, it didn't work.

All files are indexed and checksummed, losing any or all, I can recover
from.  So a working chattr +e, even if it risks losing some or all data,
would be 'best' for my case. My alternative is just copying all files
in-disk, which is a few TiB of extra I/O, but whatever.

Recreating the filesystem is annoying, since that brings the number of
backup copies *intentionally* down by one, temporarily, which I do not
want to do. I want my buffer for unintentional failures.

Thank you all,
--Jeroen