Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx> writes:
> "Theodore Ts'o" <tytso@xxxxxxx> writes:
>
>> So the failure of "e2fsck -f -C 0 -b 32768 -z /root/e2fsck.e2undo
>> /dev/md0" appears to be a bug where e2fsck doesn't work correctly with
>> an undo file when using a backup superblock. I can replicate this
>> using these commands:
>>
>> mke2fs -q -t ext4 /tmp/foo.img 2G
>> e2fsck -b 32768 -z /tmp/undo /tmp/foo.img
>>
>> Running e2fsck without the -z option succeeds. The combination of the
>> -b and -z option seems to be broken. As a workaround, I would suggest
>> doing is to try running e2fsck with -n, which will open the block
>> device read-only, e.g. "e2fsck -b 32768 -n /dev/mdXX". If the changes
>> e2fsck look safe, then you can run e2fsck without the -n option.
>
> Ted,
>
> I think this is a fix for the combination of -b and -z.
>
Hi Ted, any interest in picking this up? quite a corner case of
e2fsprogs, but I think it simplifies that path a bit. :)
> Thanks,
>
>>8
>
> Combining superblock (-b) with undo file (-z) fails iff the block size
> is not specified (-B) and is different from the first blocksize probed
> in try_open_fs (1k). The reason is as follows:
>
> try_open_fs will probe different blocksizes if none is provided on the
> command line. It is done by opening and closing the filesystem until it
> finds a blocksize that makes sense. This is fine for all io_managers,
> but undo_io creates the undo file with that blocksize during
> ext2fs_open. Once try_open_fs realizes it had the wrong blocksize and
> retries with a different blocksize, undo_io will read the previously
> created file and think it's corrupt for this filesystem.
>
> Ideally, undo_io would know this is a probe and would fix the undo file.
> It is not simple, though, because it would require undo_io to know the
> file was just created by the probe code, since an undo file survives
> through different fsck sessions. We'd have to pass this information
> around somehow. This seems like a complex change to solve a corner
> case.
>
> Instead, this patch changes the blocksize probe to always use the
> unix_io_manager. This way, we safely probe for the blocksize without
> side effects. Once the blocksize is known, we can safely reopen the
> filesystem under the proper io_manager.
>
> An easily reproducer for this issue (from Ted, adapted by me) is:
>
> mke2fs -b 4k -q -t ext4 /tmp/foo.img 2G
> e2fsck -b 32768 -z /tmp/undo /tmp/foo.img
>
> Reported-by: Peter Urbanec <linux-ext4.vger.kernel.org@xxxxxxxxxxx>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxx>
> ---
> e2fsck/unix.c | 41 ++++++++++++++++++++++++-----------------
> 1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index ae231f93deb7..341b484e6ede 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -1171,25 +1171,32 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
> errcode_t retval;
>
> *ret_fs = NULL;
> - if (ctx->superblock && ctx->blocksize) {
> - retval = ext2fs_open2(ctx->filesystem_name, ctx->io_options,
> - flags, ctx->superblock, ctx->blocksize,
> - io_ptr, ret_fs);
> - } else if (ctx->superblock) {
> - int blocksize;
> - for (blocksize = EXT2_MIN_BLOCK_SIZE;
> - blocksize <= EXT2_MAX_BLOCK_SIZE; blocksize *= 2) {
> - if (*ret_fs) {
> - ext2fs_free(*ret_fs);
> - *ret_fs = NULL;
> +
> + if (ctx->superblock) {
> + unsigned long blocksize = ctx->blocksize;
> +
> + if (!blocksize) {
> + for (blocksize = EXT2_MIN_BLOCK_SIZE;
> + blocksize <= EXT2_MAX_BLOCK_SIZE; blocksize *= 2) {
> +
> + retval = ext2fs_open2(ctx->filesystem_name,
> + ctx->io_options, flags,
> + ctx->superblock, blocksize,
> + unix_io_manager, ret_fs);
> + if (*ret_fs) {
> + ext2fs_free(*ret_fs);
> + *ret_fs = NULL;
> + }
> + if (!retval)
> + break;
> }
> - retval = ext2fs_open2(ctx->filesystem_name,
> - ctx->io_options, flags,
> - ctx->superblock, blocksize,
> - io_ptr, ret_fs);
> - if (!retval)
> - break;
> + if (retval)
> + return retval;
> }
> +
> + retval = ext2fs_open2(ctx->filesystem_name, ctx->io_options,
> + flags, ctx->superblock, blocksize,
> + io_ptr, ret_fs);
> } else
> retval = ext2fs_open2(ctx->filesystem_name, ctx->io_options,
> flags, 0, 0, io_ptr, ret_fs);
--
Gabriel Krisman Bertazi
