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

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

[Javier Martinez Canillas]

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
later due the inconsistencies when traversing the filesystem, the user can
understand why that and take any needed action.

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 */
 
 /*
  * 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)
+{
+  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
@@ -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"));
+    }
+
   if (grub_add (grub_xfs_inode_size (data),
       sizeof (struct grub_xfs_data) - sizeof (struct grub_xfs_inode) + 1, &sz))
     goto fail;
-- 
2.31.1

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

[Carlos Maiolino]

> 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 */
>  
>  /*
>   * 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)
> +{
> +  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
> @@ -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"));
> +    }
> +

I'm not a grub developer, but from the XFS point of view, this looks fine.

Feel free to add my reviewed tag if it makes sense to you:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


-- 
Carlos

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

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

[Carlos Maiolino]

Hi Daniel

> > --- 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/

I believe Javier got this from kernel, I wonder if it doesn't make sense to keep
the flags names consistent with kernel?


> 
> > +          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?

No, only xfs V5 superblock supports needsrepair flag.

Hope it helps somehow.

Cheers.

-- 
Carlos

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

[Javier Martinez Canillas]

On 4/22/21 11:20 AM, Carlos Maiolino wrote:
> Hi Daniel
> 
>>> --- 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/
> 
> I believe Javier got this from kernel, I wonder if it doesn't make sense to keep
> the flags names consistent with kernel?

That's correct, I copied the macro name from the kernel and agree
with Carlos that's better to keep it the same. It makes much easier
to grep the kernel source code when looking for something:

#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_BIGTIME	(1 << 3)	/* large timestamps */
#define XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR (1 << 4)	/* needs xfs_repair */

https://elixir.bootlin.com/linux/latest/source/fs/xfs/libxfs/xfs_format.h#L471

> 
> 
>>
>>> +          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?
> 
> No, only xfs V5 superblock supports needsrepair flag.
> 

Indeed, and there's no plan for a XFS v5+ in the near future as was
explained to me by Carlos. I also copied the logic to check from the
Linux kernel which uses '==' for all the flags supported only in v5.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

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

[Daniel Kiper]

On Thu, Apr 22, 2021 at 12:15:18PM +0200, Javier Martinez Canillas wrote:
> On 4/22/21 11:20 AM, Carlos Maiolino wrote:
> > Hi Daniel
> >
> >>> --- 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/
> >
> > I believe Javier got this from kernel, I wonder if it doesn't make sense to keep
> > the flags names consistent with kernel?
>
> That's correct, I copied the macro name from the kernel and agree
> with Carlos that's better to keep it the same. It makes much easier
> to grep the kernel source code when looking for something:
>
> #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_BIGTIME	(1 << 3)	/* large timestamps */
> #define XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR (1 << 4)	/* needs xfs_repair */
>
> https://elixir.bootlin.com/linux/latest/source/fs/xfs/libxfs/xfs_format.h#L471

If this comes from the kernel then I am OK with it.

> >>> +          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?
> >
> > No, only xfs V5 superblock supports needsrepair flag.
>
> Indeed, and there's no plan for a XFS v5+ in the near future as was
> explained to me by Carlos. I also copied the logic to check from the
> Linux kernel which uses '==' for all the flags supported only in v5.

OK, then just repost the patch with minor fixes...

Daniel

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

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3461 bytes --]

Filesystems are not mounted in GRUB. This patch will result in GRUB
outputting a warning on every access to any XFS that has this flag set,
even if it is not related to further booting. It will create an excessive
noise. FS drivers shouldn't print anything except dprintf's

Le lun. 19 avr. 2021 à 17:22, Javier Martinez Canillas <javierm@redhat.com>
a écrit :

> 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
> later due the inconsistencies when traversing the filesystem, the user can
> understand why that and take any needed action.
>
> 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 */
>
>  /*
>   * 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)
> +{
> +  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
> @@ -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"));
> +    }
> +
>    if (grub_add (grub_xfs_inode_size (data),
>        sizeof (struct grub_xfs_data) - sizeof (struct grub_xfs_inode) + 1,
> &sz))
>      goto fail;
> --
> 2.31.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 4371 bytes --]

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

[Javier Martinez Canillas]

Hello Vladimir,

On 4/22/21 4:56 PM, Vladimir 'phcoder' Serbinenko wrote:
> Filesystems are not mounted in GRUB. This patch will result in GRUB
> outputting a warning on every access to any XFS that has this flag set,
> even if it is not related to further booting. It will create an excessive
> noise. FS drivers shouldn't print anything except dprintf's
>

I see, you are right. There's a grub_xfs_mount() function in grub-core/fs/xfs.c
but it's misleading indeed since is just called from the .fs_* handlers to read
the data.

Do you think we should just ignore the flag then and drop this patch or it is
worth to have some debugging printouts with grub_dprintf("xfs", ... ) ?
 
Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

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

[Carlos Maiolino]

On Thu, Apr 22, 2021 at 05:15:31PM +0200, Javier Martinez Canillas wrote:
> Hello Vladimir,
> 
> On 4/22/21 4:56 PM, Vladimir 'phcoder' Serbinenko wrote:
> > Filesystems are not mounted in GRUB. This patch will result in GRUB
> > outputting a warning on every access to any XFS that has this flag set,
> > even if it is not related to further booting. It will create an excessive
> > noise. FS drivers shouldn't print anything except dprintf's

I see. Good to know, thanks for the information.


> Do you think we should just ignore the flag then and drop this patch or it is
> worth to have some debugging printouts with grub_dprintf("xfs", ... ) ?

If I understand this correctly, grub can simply ignore the flag, which will make
it attempt to read the filesystem anyway.

Although, if it fails to read the FS for 'some reason', maybe it can print out
a debug message then saying such flag was set in the filesystem? This may give
some extra information to the user?!

Cheers.


-- 
Carlos

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

[Daniel Kiper]

On Thu, Apr 29, 2021 at 11:05:20AM +0200, Carlos Maiolino wrote:
> On Thu, Apr 22, 2021 at 05:15:31PM +0200, Javier Martinez Canillas wrote:
> > Hello Vladimir,
> >
> > On 4/22/21 4:56 PM, Vladimir 'phcoder' Serbinenko wrote:
> > > Filesystems are not mounted in GRUB. This patch will result in GRUB
> > > outputting a warning on every access to any XFS that has this flag set,
> > > even if it is not related to further booting. It will create an excessive
> > > noise. FS drivers shouldn't print anything except dprintf's
>
> I see. Good to know, thanks for the information.
>
> > Do you think we should just ignore the flag then and drop this patch or it is
> > worth to have some debugging printouts with grub_dprintf("xfs", ... ) ?
>
> If I understand this correctly, grub can simply ignore the flag, which will make
> it attempt to read the filesystem anyway.
>
> Although, if it fails to read the FS for 'some reason', maybe it can print out
> a debug message then saying such flag was set in the filesystem? This may give
> some extra information to the user?!

I think it is good idea. Could you prepare a patch?

Daniel

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

[Javier Martinez Canillas]

Hello Daniel,

On 4/29/21 4:32 PM, Daniel Kiper wrote:
> On Thu, Apr 29, 2021 at 11:05:20AM +0200, Carlos Maiolino wrote:
>> On Thu, Apr 22, 2021 at 05:15:31PM +0200, Javier Martinez Canillas wrote:
>>> Hello Vladimir,
>>>
>>> On 4/22/21 4:56 PM, Vladimir 'phcoder' Serbinenko wrote:
>>>> Filesystems are not mounted in GRUB. This patch will result in GRUB
>>>> outputting a warning on every access to any XFS that has this flag set,
>>>> even if it is not related to further booting. It will create an excessive
>>>> noise. FS drivers shouldn't print anything except dprintf's
>>
>> I see. Good to know, thanks for the information.
>>
>>> Do you think we should just ignore the flag then and drop this patch or it is
>>> worth to have some debugging printouts with grub_dprintf("xfs", ... ) ?
>>
>> If I understand this correctly, grub can simply ignore the flag, which will make
>> it attempt to read the filesystem anyway.
>>
>> Although, if it fails to read the FS for 'some reason', maybe it can print out
>> a debug message then saying such flag was set in the filesystem? This may give
>> some extra information to the user?!
> 
> I think it is good idea. Could you prepare a patch?
>

Yes, I'll post a new version today. Also, it was pointed out to me that even if
GRUB will mostly ignore this feature flag (besides printing a message if fails
to read something), we should at least include XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR
in the XFS_SB_FEAT_INCOMPAT_SUPPORTED list.

Otherwise GRUB will just fail to read anything if grub_xfs_sb_valid() finds that
an unsupported incompatible feature is enabled in the super block.
 
> Daniel

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat