Re: [PATCH] xfs: Print a warning if the filesystem needs to be repaired

[Daniel Kiper]

On Mon, Apr 19, 2021 at 05:16:50PM +0200, Javier Martinez Canillas wrote:
> XFS now has an incompat feature flag to indicate that the filesystem needs
> to be repaired. The Linux kernel refuses to mount a filesystem that has it
> set and only the xfs_repair tool is able to clear that flag.
>
> One option is to make the GRUB behaviour consistent with the Linux kernel,
> and don't allow to mount a XFS filesystem that needs to be repaired. But
> that will prevent to even boot to a rescue environment for the OS in order
> to repair the filesystem.
>
> To prevent this situation, let's GRUB attempt to mount the filesystem even
> when needs to be repaired. Since after all, it currently attempts to mount
> even if is in an inconsistent state without trying to replay the jornal.
>
> For this reason, make it a best effort but print an error message when the
> filesystem is mounted if needs to be repaired. That way, if booting fails

s/filesystem is mounted if needs to be repaired/filesystem is mounted and 
should be repaired/

> later due the inconsistencies when traversing the filesystem, the user can
> understand why that and take any needed action.

s/that/that happened/

> Suggested-by: Eric Sandeen <esandeen@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  grub-core/fs/xfs.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index 43023e03fb3..22e7e61d574 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -79,6 +79,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define XFS_SB_FEAT_INCOMPAT_FTYPE      (1 << 0)        /* filetype in 
> dirent */
>  #define XFS_SB_FEAT_INCOMPAT_SPINODES   (1 << 1)        /* sparse inode 
> chunks */
>  #define XFS_SB_FEAT_INCOMPAT_META_UUID  (1 << 2)        /* metadata UUID */
> +#define XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR (1 << 4)        /* needs xfs_repair 
> */

s/XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR/XFS_SB_FEAT_INCOMPAT_NEEDS_REPAIR/

>  /*
>   * Directory entries with ftype are explicitly handled by GRUB code.
> @@ -300,6 +301,16 @@ static int grub_xfs_sb_valid(struct grub_xfs_data *data)
>    return 0;
>  }
>
> +static int
> +grub_xfs_sb_needsrepair(struct grub_xfs_data *data)

s/grub_xfs_sb_needsrepair/xfs_sb_needs_repair/

> +{
> +  return ((data->sblock.version &
> +           grub_cpu_to_be16_compile_time(XFS_SB_VERSION_NUMBITS)) ==

Missing space before "(" and below...

> +          grub_cpu_to_be16_compile_time(XFS_SB_VERSION_5) &&

Hmmm... Is XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR flag available for
XFS_SB_VERSION_5 only? Should not we use ">=" instead of "=="?
Does not older XFS versions support this flag?

> +          data->sblock.sb_features_incompat &
> +          grub_cpu_to_be32_compile_time(XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR));

I would add more parenthesis here...

> +}
> +
>  /* Filetype information as used in inodes.  */
>  #define FILETYPE_INO_MASK    0170000
>  #define FILETYPE_INO_REG     0100000
> @@ -915,6 +926,11 @@ grub_xfs_mount (grub_disk_t disk)
>    if (!grub_xfs_sb_valid(data))
>      goto fail;
>
> +  if (grub_xfs_sb_needsrepair(data))
> +    {
> +      grub_printf (N_("Filesystem needs repair. Please run a XFS repair 
> tool"));
> +    }

Please drop these curly brackets here...

Daniel