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"));
   }
 
   return TRUE;
@@ -214,6 +212,11 @@ Ext4OpenSuperblock (
     return EFI_UNSUPPORTED;
   }
 
+  if ((Partition->FeaturesIncompat & EXT4_FEATURE_INCOMPAT_RECOVER) == EXT4_FEATURE_INCOMPAT_RECOVER) {
+    DEBUG ((DEBUG_WARN, "[ext4] Needs journal recovery mount read-only\n"));
+    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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#80450): https://edk2.groups.io/g/devel/message/80450
Mute This Topic: https://groups.io/mt/85494673/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-