Re: [2.06 RELEASE PATCH 3/3] fs/xfs: Add needsrepair incompat feature support

[Daniel Kiper]

On Wed, May 05, 2021 at 10:32:33AM +0200, Javier Martinez Canillas wrote:
> XFS now has an incompat feature flag to indicate that the filesystem needs

s/XFS/The XFS/

> 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.
>
> The GRUB doesn't have the concept of mounting filesystems and just attempt
> to read the files. But it does some sanity checking, before attempting to

s/,//

> read from a filesystem.
>
> Among the things that are tested, is if the super block only has set the

s/that are/which are/
s/,//

> incompatible features flags that are supported by GRUB. If it contains any
> flags that are not listed as supported, reading the XFS filesystem fails.
>
> Since GRUB doesn't attempt to detect if the filesystem is inconsistent nor

s/GRUB/the GRUB/

> replays the journal, just ignore if a filesystem needs to be repaired too.
>
> But print a message if grub_xfs_mount() fails, to give more information to
> the user about why that could had been the case.

This statement is not inline with the logic of the code.

> Suggested-by: Eric Sandeen <esandeen@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  grub-core/fs/xfs.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index 2ce76ec70f9..9e290a144a0 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -84,6 +84,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #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_BIGTIME (1 << 3)        /* large timestamps */
> +#define XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR (1 << 4)    /* needs xfs_repair */
>
>  /*
>   * Directory entries with ftype are explicitly handled by GRUB code.
> @@ -98,7 +99,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>       (XFS_SB_FEAT_INCOMPAT_FTYPE | \
>        XFS_SB_FEAT_INCOMPAT_SPINODES | \
>        XFS_SB_FEAT_INCOMPAT_META_UUID | \
> -      XFS_SB_FEAT_INCOMPAT_BIGTIME)
> +      XFS_SB_FEAT_INCOMPAT_BIGTIME | \
> +      XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR)
>
>  struct grub_xfs_sblock
>  {
> @@ -307,6 +309,16 @@ static int grub_xfs_sb_valid(struct grub_xfs_data *data)
>    return 0;
>  }
>
> +static int
> +grub_xfs_sb_needs_repair (struct grub_xfs_data *data)
> +{
> +  return ((data->sblock.version &
> +           grub_cpu_to_be16_compile_time (XFS_SB_VERSION_NUMBITS)) ==
> +          grub_cpu_to_be16_compile_time (XFS_SB_VERSION_5) &&
> +          (data->sblock.sb_features_incompat &
> +           grub_cpu_to_be32_compile_time 
> (XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR)));
> +}
> +
>  /* Filetype information as used in inodes.  */
>  #define FILETYPE_INO_MASK    0170000
>  #define FILETYPE_INO_REG     0100000
> @@ -922,6 +934,9 @@ grub_xfs_mount (grub_disk_t disk)
>    if (!grub_xfs_sb_valid(data))
>      goto fail;
>
> +  if (grub_xfs_sb_needs_repair (data))
> +    grub_dprintf ("xfs", "Filesystem needs repair, could cause reads to 
> fail\n");

s/Filesystem/XFS filesystem/

Daniel