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

[Daniel Kiper]

On Wed, May 12, 2021 at 05:46:16PM +0200, Javier Martinez Canillas wrote:
> The 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.
>
> The GRUB doesn't have the concept of mounting filesystems and just attempt

s/attempt/attempts/

> to read the files. But it does some sanity checking before attempting to
> read from a filesystem.
>
> Among the things which are tested, is if the super block only has set the
> 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 the GRUB doesn't attempt to detect if the filesystem is inconsistent
> nor replays the journal, just ignore if a filesystem needs to be repaired.

This sentence does not parse well...

> For this reason, make it a best effort but print a debug message when the

s/but/and/

> XFS filesystem is accessed if should be repaired. That way, if the reading

s/is accessed if/accessed/

> or later booting fail, the user can figure out that may be related to this.

s/if the reading or later booting fail/if later reading or booting fails/

> Suggested-by: Eric Sandeen <esandeen@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> Changes in v2:
> - Improve commit message of patch #4 (dkiper).
> - Fix typo in debug message of patch #4 (dkiper).
>
>  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 bcf281cf80d..6056bf44e32 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", "XFS filesystem needs repair, can lead to 
> failures\n");

s/can lead to failures/boot may fail/

Daniel