Re: ext4 regression in v5.9-rc2 from e7bfb5c9bb3d on ro fs with overlapped bitmaps

[Jan Kara]

Hi Josh!

On Mon 05-10-20 01:14:54, Josh Triplett wrote:
> Ran into an ext4 regression when testing upgrades to 5.9-rc kernels:
> 
> Commit e7bfb5c9bb3d ("ext4: handle add_system_zone() failure in
> ext4_setup_system_zone()") breaks mounting of read-only ext4 filesystems
> with intentionally overlapping bitmap blocks.
> 
> On an always-read-only filesystem explicitly marked with
> EXT4_FEATURE_RO_COMPAT_SHARED_BLOCKS, prior to that commit, it's safe to
> point all the block and inode bitmaps to a single block of all 1s,
> because a read-only filesystem will never allocate or free any blocks or
> inodes.
> However, after that commit, the block validity check rejects such
> filesystems with -EUCLEAN and "failed to initialize system zone (-117)".
> This causes systems that previously worked correctly to fail when
> upgrading to v5.9-rc2 or later.
> 
> This was obviously a bugfix, and I'm not suggesting that it should be
> reverted; it looks like this effectively worked by accident before,
> because the block_validity check wasn't fully functional. However, this
> does break real systems, and I'd like to get some kind of regression fix
> in before 5.9 final if possible. I think it would suffice to make
> block_validity default to false if and only if
> EXT4_FEATURE_RO_COMPAT_SHARED_BLOCKS is set.
> 
> Does that seem like a reasonable fix?

Well, but EXT4_FEATURE_RO_COMPAT_SHARED_BLOCKS is your internal feature
that's not present in current upstream kernel AFAICS. So if you have out of
tree (even disk format incompatible) filesystem feature and upstream kernel
changes in a way that breaks you, I'd say it's your problem to keep the
pieces together?

So IMO you need to change implementation of your
EXT4_FEATURE_RO_COMPAT_SHARED_BLOCKS feature to work with more paranoid
block validity checking. Whether you'll do that by disabling block validity
or by properly teaching block validity code about that feature is up to you
but I don't think that belongs to the upstream kernel since that is correct
as is...

								Honza

-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR