Re: [PATCH 2/2] Ext4Pkg: Support non-cleanlty unmounted filesystems

["Pedro Falcato" <pedro.falcato@gmail.com>]

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 <jbrasen@nvidia.com> wrote:
>
> Support for uncleanly mounted filesystems, if there is a recovery
> journal mark filesystem as read-only.
>
> Signed-off-by: Jeff Brasen <jbrasen@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