Comments below.
The patch itself also looks good.
Commit message issue: typo on "non-cleanlty".
On Thu, Sep 9, 2021 at 9:41 PM Jeff Brasen <jbra...@nvidia.com> wrote:
>
> Support for uncleanly mounted filesystems, if there is a recovery
> journal mark filesystem as read-only.
>
> Signed-off-by: Jeff Brasen <jbra...@nvidia.com>
> ---
> Features/Ext4Pkg/Ext4Dxe/Superblock.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> index 1f8cdd3705..444dd3ca97 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> @@ -18,7 +18,7 @@ STATIC CONST UINT32 gSupportedIncompatFeat =
> EXT4_FEATURE_INCOMPAT_64BIT | EXT4_FEATURE_INCOMPAT_DIRDATA |
> EXT4_FEATURE_INCOMPAT_FLEX_BG | EXT4_FEATURE_INCOMPAT_FILETYPE |
> EXT4_FEATURE_INCOMPAT_EXTENTS | EXT4_FEATURE_INCOMPAT_LARGEDIR |
> - EXT4_FEATURE_INCOMPAT_MMP;
> + EXT4_FEATURE_INCOMPAT_MMP | EXT4_FEATURE_INCOMPAT_RECOVER;
>
> // Future features that may be nice additions in the future:
> // 1) Btree support: Required for write support and would speed up lookups
> in large directories.
> @@ -88,10 +88,8 @@ Ext4SuperblockValidate (
> return FALSE;
> }
>
> - // Is this correct behaviour? Imagine the power cuts out, should the
> system fail to boot because
> - // we're scared of touching something corrupt?
> if ((Sb->s_state & EXT4_FS_STATE_UNMOUNTED) == 0) {
> - return FALSE;
> + DEBUG ((DEBUG_WARN, "[ext4] filesystem was not unmounted cleanly\n"));
Nitpick: filesystem should be capitalized.
> }
>
> return TRUE;
> @@ -214,6 +212,11 @@ Ext4OpenSuperblock (
> return EFI_UNSUPPORTED;
> }
>
> + if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_RECOVER) ==
> EXT4_FEATURE_INCOMPAT_RECOVER) {
New code that wants to test for features/feature-sets should use
EXT4_HAS_INCOMPAT, EXT4_HAS_COMPAT, EXT4_HAS_RO_COMPAT.
The code in this function that's testing for features using i.e:
FeaturesIncompat & FEATURE manually should likely be replaced by the
macros.
> + DEBUG ((DEBUG_WARN, "[ext4] Needs journal recovery mount read-only\n"));
The debug message looks poorly worded; something like "[ext4] Needs
journal recovery, mounting read-only\n" sounds good?
> + Partition->ReadOnly = TRUE;
> + }
> +
> // At the time of writing, it's the only supported checksum.
> if (Partition->FeaturesCompat & EXT4_FEATURE_RO_COMPAT_METADATA_CSUM &&
> Sb->s_checksum_type != EXT4_CHECKSUM_CRC32C) {
> --
> 2.17.1
>
--
Pedro Falcato
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80466): https://edk2.groups.io/g/devel/message/80466
Mute This Topic: https://groups.io/mt/85494673/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
