[PATCH v3 0/2] ExtPkg Updates

[PATCH v3 0/2] ExtPkg Updates

[Jeff Brasen]

I have been using the new Ext4Pkg and been pretty successful and it is solving a use case we had.

Had a couple updates to propose

1. Changed the implementation of the binding protocol to both check if the driver is already bound
to the partition as well as added a really quick check to validate the magic value in supported.
This improves performance when you have a large number of non-ext4 partitions on the system.

2. As we are planning on using this for boot support we want to support unclean filesystem states in
case the user doesn't reset cleanly. I added a check if the recovery journal is present and if so treat
the filesystem as read-only (I know the driver is only RO at this point, but figured if you added write
support prior to recovery journal support we would want that). With this everything seems to work great.
I can add this under a FeaturePcd if desired as well.

Change log

v3 - Removed goto flow on binding supported failures
     Minor code review comments
v2 - Minor code review comments
v1 - Initial revision

Jeff Brasen (2):
  Ext4Pkg: Improve Ext4IsBindingSupported() behavior
  Ext4Pkg: Support uncleanly unmounted filesystems

 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 54 +++++++++++++++++++++------
 Features/Ext4Pkg/Ext4Dxe/Superblock.c | 46 +++++++++++++++++++++--
 3 files changed, 99 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH v3 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

[Jeff Brasen]

A couple of improvements to improve performance.
Add check to return ACCESS_DENIED if already connected
Add check to verify superblock magic during supported to reduce start calls

Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
 Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 54 +++++++++++++++++++++------
 Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 +++++++++++++++++
 3 files changed, 92 insertions(+), 11 deletions(-)

diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
index 64eab455db..a9b932ed52 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
@@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
   OUT UINTN          *VolNameLen
   );
 
+/**
+   Checks the superblock's magic value.
+
+   @param[in] DiskIo      Pointer to the DiskIo.
+   @param[in] BlockIo     Pointer to the BlockIo.
+
+   @returns Whether the partition has a valid EXT4 superblock magic value.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
+  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
+  );
+
 #endif
diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
index ea2e048d77..d9fbe9ea78 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
@@ -631,7 +631,6 @@ Ext4Unload (
   @retval EFI_ACCESS_DENIED        The device specified by ControllerHandle and
                                    RemainingDevicePath is already being managed by a different
                                    driver or an application that requires exclusive access.
-                                   Currently not implemented.
   @retval EFI_UNSUPPORTED          The device specified by ControllerHandle and
                                    RemainingDevicePath is not supported by the driver specified by This.
 **/
@@ -643,32 +642,65 @@ Ext4IsBindingSupported (
   IN EFI_DEVICE_PATH *RemainingDevicePath  OPTIONAL
   )
 {
-  // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
-  // protocol and ignore the output argument entirely
+  EFI_STATUS            Status;
+  EFI_DISK_IO_PROTOCOL  *DiskIo;
+  EFI_BLOCK_IO_PROTOCOL *BlockIo;
 
-  EFI_STATUS  Status;
+  DiskIo = NULL;
+  BlockIo = NULL;
 
+  //
+  // Open the IO Abstraction(s) needed to perform the supported test
+  //
   Status = gBS->OpenProtocol (
                   ControllerHandle,
                   &gEfiDiskIoProtocolGuid,
-                  NULL,
-                  BindingProtocol->ImageHandle,
+                  (VOID **) &DiskIo,
+                  BindingProtocol->DriverBindingHandle,
                   ControllerHandle,
-                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+                  EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
 
   if (EFI_ERROR (Status)) {
     return Status;
   }
-
+  //
+  // Open the IO Abstraction(s) needed to perform the supported test
+  //
   Status = gBS->OpenProtocol (
                   ControllerHandle,
                   &gEfiBlockIoProtocolGuid,
-                  NULL,
-                  BindingProtocol->ImageHandle,
+                  (VOID **) &BlockIo,
+                  BindingProtocol->DriverBindingHandle,
                   ControllerHandle,
-                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
+                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
                   );
+
+  if (!EFI_ERROR (Status)) {
+    if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
+      Status = EFI_UNSUPPORTED;
+    }
+  }
+
+  //
+  // Close the I/O Abstraction(s) used to perform the supported test
+  //
+  if (DiskIo != NULL) {
+    gBS->CloseProtocol (
+          ControllerHandle,
+          &gEfiDiskIoProtocolGuid,
+          BindingProtocol->DriverBindingHandle,
+          ControllerHandle
+          );
+  }
+  if (BlockIo != NULL) {
+    gBS->CloseProtocol (
+          ControllerHandle,
+          &gEfiBlockIoProtocolGuid,
+          BindingProtocol->DriverBindingHandle,
+          ControllerHandle
+          );
+  }
   return Status;
 }
 
diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
index c321d8c3d8..0c965415c5 100644
--- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
+++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
@@ -34,6 +34,41 @@ STATIC CONST UINT32  gSupportedIncompatFeat =
 // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
 // references and add some Ext4SignalCorruption function + function call.
 
+/**
+   Checks the superblock's magic value.
+
+   @param[in] DiskIo      Pointer to the DiskIo.
+   @param[in] BlockIo     Pointer to the BlockIo.
+
+   @returns Whether the partition has a valid EXT4 superblock magic value.
+**/
+BOOLEAN
+Ext4SuperblockCheckMagic (
+  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
+  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
+  )
+{
+  UINT16      Magic;
+  EFI_STATUS  Status;
+
+  Status = DiskIo->ReadDisk (
+                     DiskIo,
+                     BlockIo->Media->MediaId,
+                     EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
+                     sizeof (Magic),
+                     &Magic
+                     );
+  if (EFI_ERROR (Status)) {
+    return FALSE;
+  }
+
+  if (Magic != EXT4_SIGNATURE) {
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
 /**
    Does brief validation of the ext4 superblock.
 
-- 
2.17.1

Re: [edk2-devel] [PATCH v3 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

[Marvin Häuser]

On 11/09/2021 00:11, Jeff Brasen via groups.io wrote:
> A couple of improvements to improve performance.
> Add check to return ACCESS_DENIED if already connected
> Add check to verify superblock magic during supported to reduce start calls
>
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> ---
>   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 54 +++++++++++++++++++++------
>   Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 +++++++++++++++++
>   3 files changed, 92 insertions(+), 11 deletions(-)
>
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> index 64eab455db..a9b932ed52 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
>     OUT UINTN          *VolNameLen
>     );
>   
> +/**
> +   Checks the superblock's magic value.
> +
> +   @param[in] DiskIo      Pointer to the DiskIo.
> +   @param[in] BlockIo     Pointer to the BlockIo.
> +
> +   @returns Whether the partition has a valid EXT4 superblock magic value.
> +**/
> +BOOLEAN
> +Ext4SuperblockCheckMagic (
> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
> +  );
> +
>   #endif
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> index ea2e048d77..d9fbe9ea78 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> @@ -631,7 +631,6 @@ Ext4Unload (
>     @retval EFI_ACCESS_DENIED        The device specified by ControllerHandle and
>                                      RemainingDevicePath is already being managed by a different
>                                      driver or an application that requires exclusive access.
> -                                   Currently not implemented.
>     @retval EFI_UNSUPPORTED          The device specified by ControllerHandle and
>                                      RemainingDevicePath is not supported by the driver specified by This.
>   **/
> @@ -643,32 +642,65 @@ Ext4IsBindingSupported (
>     IN EFI_DEVICE_PATH *RemainingDevicePath  OPTIONAL
>     )
>   {
> -  // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
> -  // protocol and ignore the output argument entirely
> +  EFI_STATUS            Status;
> +  EFI_DISK_IO_PROTOCOL  *DiskIo;
> +  EFI_BLOCK_IO_PROTOCOL *BlockIo;
>   
> -  EFI_STATUS  Status;
> +  DiskIo = NULL;
> +  BlockIo = NULL;
>   
> +  //
> +  // Open the IO Abstraction(s) needed to perform the supported test
> +  //
>     Status = gBS->OpenProtocol (
>                     ControllerHandle,
>                     &gEfiDiskIoProtocolGuid,
> -                  NULL,
> -                  BindingProtocol->ImageHandle,
> +                  (VOID **) &DiskIo,
> +                  BindingProtocol->DriverBindingHandle,
>                     ControllerHandle,
> -                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
> +                  EFI_OPEN_PROTOCOL_BY_DRIVER
>                     );
>   
>     if (EFI_ERROR (Status)) {
>       return Status;
>     }
> -
> +  //
> +  // Open the IO Abstraction(s) needed to perform the supported test
> +  //
>     Status = gBS->OpenProtocol (
>                     ControllerHandle,
>                     &gEfiBlockIoProtocolGuid,
> -                  NULL,
> -                  BindingProtocol->ImageHandle,
> +                  (VOID **) &BlockIo,
> +                  BindingProtocol->DriverBindingHandle,
>                     ControllerHandle,
> -                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
>                     );
> +
> +  if (!EFI_ERROR (Status)) {
> +    if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
> +      Status = EFI_UNSUPPORTED;
> +    }
> +  }
> +
> +  //
> +  // Close the I/O Abstraction(s) used to perform the supported test
> +  //
> +  if (DiskIo != NULL) {
> +    gBS->CloseProtocol (
> +          ControllerHandle,
> +          &gEfiDiskIoProtocolGuid,
> +          BindingProtocol->DriverBindingHandle,
> +          ControllerHandle
> +          );
> +  }
> +  if (BlockIo != NULL) {
> +    gBS->CloseProtocol (
> +          ControllerHandle,
> +          &gEfiBlockIoProtocolGuid,
> +          BindingProtocol->DriverBindingHandle,
> +          ControllerHandle
> +          );
> +  }

GET_PROTOCOL protocols are not to be closed, I guess this was missed 
when there was still the question whether to use BY_DRIVER for BlockIo.

Best regards,
Marvin

>     return Status;
>   }
>   
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> index c321d8c3d8..0c965415c5 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> @@ -34,6 +34,41 @@ STATIC CONST UINT32  gSupportedIncompatFeat =
>   // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
>   // references and add some Ext4SignalCorruption function + function call.
>   
> +/**
> +   Checks the superblock's magic value.
> +
> +   @param[in] DiskIo      Pointer to the DiskIo.
> +   @param[in] BlockIo     Pointer to the BlockIo.
> +
> +   @returns Whether the partition has a valid EXT4 superblock magic value.
> +**/
> +BOOLEAN
> +Ext4SuperblockCheckMagic (
> +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
> +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
> +  )
> +{
> +  UINT16      Magic;
> +  EFI_STATUS  Status;
> +
> +  Status = DiskIo->ReadDisk (
> +                     DiskIo,
> +                     BlockIo->Media->MediaId,
> +                     EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
> +                     sizeof (Magic),
> +                     &Magic
> +                     );
> +  if (EFI_ERROR (Status)) {
> +    return FALSE;
> +  }
> +
> +  if (Magic != EXT4_SIGNATURE) {
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
>   /**
>      Does brief validation of the ext4 superblock.
>   

Re: [edk2-devel] [PATCH v3 1/2] Ext4Pkg: Improve Ext4IsBindingSupported() behavior

[Pedro Falcato]

On BY_DRIVER, Jeff reached the conclusion that we can't open the
BLOCK_IO protocol with BY_DRIVER since it's already owned by
DiskIoDxe. The finding makes sense and is consistent
with FatPkg's behaviour.
As for the GET_PROTOCOL issue, feel free to send a patch ;)

On Sun, Sep 12, 2021 at 11:40 AM Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> On 11/09/2021 00:11, Jeff Brasen via groups.io wrote:
> > A couple of improvements to improve performance.
> > Add check to return ACCESS_DENIED if already connected
> > Add check to verify superblock magic during supported to reduce start calls
> >
> > Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> > ---
> >   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
> >   Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 54 +++++++++++++++++++++------
> >   Features/Ext4Pkg/Ext4Dxe/Superblock.c | 35 +++++++++++++++++
> >   3 files changed, 92 insertions(+), 11 deletions(-)
> >
> > diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> > index 64eab455db..a9b932ed52 100644
> > --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> > +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h
> > @@ -1117,4 +1117,18 @@ Ext4GetVolumeName (
> >     OUT UINTN          *VolNameLen
> >     );
> >
> > +/**
> > +   Checks the superblock's magic value.
> > +
> > +   @param[in] DiskIo      Pointer to the DiskIo.
> > +   @param[in] BlockIo     Pointer to the BlockIo.
> > +
> > +   @returns Whether the partition has a valid EXT4 superblock magic value.
> > +**/
> > +BOOLEAN
> > +Ext4SuperblockCheckMagic (
> > +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
> > +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
> > +  );
> > +
> >   #endif
> > diff --git a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> > index ea2e048d77..d9fbe9ea78 100644
> > --- a/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> > +++ b/Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c
> > @@ -631,7 +631,6 @@ Ext4Unload (
> >     @retval EFI_ACCESS_DENIED        The device specified by ControllerHandle and
> >                                      RemainingDevicePath is already being managed by a different
> >                                      driver or an application that requires exclusive access.
> > -                                   Currently not implemented.
> >     @retval EFI_UNSUPPORTED          The device specified by ControllerHandle and
> >                                      RemainingDevicePath is not supported by the driver specified by This.
> >   **/
> > @@ -643,32 +642,65 @@ Ext4IsBindingSupported (
> >     IN EFI_DEVICE_PATH *RemainingDevicePath  OPTIONAL
> >     )
> >   {
> > -  // Note to self: EFI_OPEN_PROTOCOL_TEST_PROTOCOL lets us not close the
> > -  // protocol and ignore the output argument entirely
> > +  EFI_STATUS            Status;
> > +  EFI_DISK_IO_PROTOCOL  *DiskIo;
> > +  EFI_BLOCK_IO_PROTOCOL *BlockIo;
> >
> > -  EFI_STATUS  Status;
> > +  DiskIo = NULL;
> > +  BlockIo = NULL;
> >
> > +  //
> > +  // Open the IO Abstraction(s) needed to perform the supported test
> > +  //
> >     Status = gBS->OpenProtocol (
> >                     ControllerHandle,
> >                     &gEfiDiskIoProtocolGuid,
> > -                  NULL,
> > -                  BindingProtocol->ImageHandle,
> > +                  (VOID **) &DiskIo,
> > +                  BindingProtocol->DriverBindingHandle,
> >                     ControllerHandle,
> > -                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
> > +                  EFI_OPEN_PROTOCOL_BY_DRIVER
> >                     );
> >
> >     if (EFI_ERROR (Status)) {
> >       return Status;
> >     }
> > -
> > +  //
> > +  // Open the IO Abstraction(s) needed to perform the supported test
> > +  //
> >     Status = gBS->OpenProtocol (
> >                     ControllerHandle,
> >                     &gEfiBlockIoProtocolGuid,
> > -                  NULL,
> > -                  BindingProtocol->ImageHandle,
> > +                  (VOID **) &BlockIo,
> > +                  BindingProtocol->DriverBindingHandle,
> >                     ControllerHandle,
> > -                  EFI_OPEN_PROTOCOL_TEST_PROTOCOL
> > +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
> >                     );
> > +
> > +  if (!EFI_ERROR (Status)) {
> > +    if (!Ext4SuperblockCheckMagic (DiskIo, BlockIo)) {
> > +      Status = EFI_UNSUPPORTED;
> > +    }
> > +  }
> > +
> > +  //
> > +  // Close the I/O Abstraction(s) used to perform the supported test
> > +  //
> > +  if (DiskIo != NULL) {
> > +    gBS->CloseProtocol (
> > +          ControllerHandle,
> > +          &gEfiDiskIoProtocolGuid,
> > +          BindingProtocol->DriverBindingHandle,
> > +          ControllerHandle
> > +          );
> > +  }
> > +  if (BlockIo != NULL) {
> > +    gBS->CloseProtocol (
> > +          ControllerHandle,
> > +          &gEfiBlockIoProtocolGuid,
> > +          BindingProtocol->DriverBindingHandle,
> > +          ControllerHandle
> > +          );
> > +  }
>
> GET_PROTOCOL protocols are not to be closed, I guess this was missed
> when there was still the question whether to use BY_DRIVER for BlockIo.
>
> Best regards,
> Marvin
>
> >     return Status;
> >   }
> >
> > diff --git a/Features/Ext4Pkg/Ext4Dxe/Superblock.c b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> > index c321d8c3d8..0c965415c5 100644
> > --- a/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> > +++ b/Features/Ext4Pkg/Ext4Dxe/Superblock.c
> > @@ -34,6 +34,41 @@ STATIC CONST UINT32  gSupportedIncompatFeat =
> >   // this is desired, it's fairly trivial to look for EFI_VOLUME_CORRUPTED
> >   // references and add some Ext4SignalCorruption function + function call.
> >
> > +/**
> > +   Checks the superblock's magic value.
> > +
> > +   @param[in] DiskIo      Pointer to the DiskIo.
> > +   @param[in] BlockIo     Pointer to the BlockIo.
> > +
> > +   @returns Whether the partition has a valid EXT4 superblock magic value.
> > +**/
> > +BOOLEAN
> > +Ext4SuperblockCheckMagic (
> > +  IN EFI_DISK_IO_PROTOCOL   *DiskIo,
> > +  IN EFI_BLOCK_IO_PROTOCOL  *BlockIo
> > +  )
> > +{
> > +  UINT16      Magic;
> > +  EFI_STATUS  Status;
> > +
> > +  Status = DiskIo->ReadDisk (
> > +                     DiskIo,
> > +                     BlockIo->Media->MediaId,
> > +                     EXT4_SUPERBLOCK_OFFSET + OFFSET_OF (EXT4_SUPERBLOCK, s_magic),
> > +                     sizeof (Magic),
> > +                     &Magic
> > +                     );
> > +  if (EFI_ERROR (Status)) {
> > +    return FALSE;
> > +  }
> > +
> > +  if (Magic != EXT4_SIGNATURE) {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> >   /**
> >      Does brief validation of the ext4 superblock.
> >
>


-- 
Pedro Falcato

[PATCH v3 2/2] Ext4Pkg: Support uncleanly unmounted filesystems

[Jeff Brasen]

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 0c965415c5..9c3f7a9e7b 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.
@@ -89,10 +89,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;
@@ -215,6 +213,11 @@ Ext4OpenSuperblock (
     return EFI_UNSUPPORTED;
   }
 
+  if (EXT4_HAS_INCOMPAT (Partition, EXT4_FEATURE_INCOMPAT_RECOVER)) {
+    DEBUG ((DEBUG_WARN, "[ext4] Needs journal recovery, mounting 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

Re: [PATCH v3 0/2] ExtPkg Updates

[Pedro Falcato]

Series Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

On Fri, Sep 10, 2021 at 11:11 PM Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> I have been using the new Ext4Pkg and been pretty successful and it is solving a use case we had.
>
> Had a couple updates to propose
>
> 1. Changed the implementation of the binding protocol to both check if the driver is already bound
> to the partition as well as added a really quick check to validate the magic value in supported.
> This improves performance when you have a large number of non-ext4 partitions on the system.
>
> 2. As we are planning on using this for boot support we want to support unclean filesystem states in
> case the user doesn't reset cleanly. I added a check if the recovery journal is present and if so treat
> the filesystem as read-only (I know the driver is only RO at this point, but figured if you added write
> support prior to recovery journal support we would want that). With this everything seems to work great.
> I can add this under a FeaturePcd if desired as well.
>
> Change log
>
> v3 - Removed goto flow on binding supported failures
>      Minor code review comments
> v2 - Minor code review comments
> v1 - Initial revision
>
> Jeff Brasen (2):
>   Ext4Pkg: Improve Ext4IsBindingSupported() behavior
>   Ext4Pkg: Support uncleanly unmounted filesystems
>
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 54 +++++++++++++++++++++------
>  Features/Ext4Pkg/Ext4Dxe/Superblock.c | 46 +++++++++++++++++++++--
>  3 files changed, 99 insertions(+), 15 deletions(-)
>
> --
> 2.17.1
>


-- 
Pedro Falcato

Re: [PATCH v3 0/2] ExtPkg Updates

[Pedro Falcato]

Pushed as to edk2-platforms as
71f334393361d53e805fe9a01e2cf7bc43e909ce and
7872c983ed45de4c9e4e9295aa249e1369913094, respectively.

On Fri, Sep 10, 2021 at 11:11 PM Jeff Brasen <jbrasen@nvidia.com> wrote:
>
> I have been using the new Ext4Pkg and been pretty successful and it is solving a use case we had.
>
> Had a couple updates to propose
>
> 1. Changed the implementation of the binding protocol to both check if the driver is already bound
> to the partition as well as added a really quick check to validate the magic value in supported.
> This improves performance when you have a large number of non-ext4 partitions on the system.
>
> 2. As we are planning on using this for boot support we want to support unclean filesystem states in
> case the user doesn't reset cleanly. I added a check if the recovery journal is present and if so treat
> the filesystem as read-only (I know the driver is only RO at this point, but figured if you added write
> support prior to recovery journal support we would want that). With this everything seems to work great.
> I can add this under a FeaturePcd if desired as well.
>
> Change log
>
> v3 - Removed goto flow on binding supported failures
>      Minor code review comments
> v2 - Minor code review comments
> v1 - Initial revision
>
> Jeff Brasen (2):
>   Ext4Pkg: Improve Ext4IsBindingSupported() behavior
>   Ext4Pkg: Support uncleanly unmounted filesystems
>
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.h    | 14 +++++++
>  Features/Ext4Pkg/Ext4Dxe/Ext4Dxe.c    | 54 +++++++++++++++++++++------
>  Features/Ext4Pkg/Ext4Dxe/Superblock.c | 46 +++++++++++++++++++++--
>  3 files changed, 99 insertions(+), 15 deletions(-)
>
> --
> 2.17.1
>


-- 
Pedro Falcato