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

[Daniel Kiper]

On Wed, May 12, 2021 at 05:46:15PM +0200, Javier Martinez Canillas wrote:
> From: Carlos Maiolino <cmaiolino@redhat.com>
>
> The XFS filesystem supports a bigtime feature to overcome y2038 problem.
> This patch makes the GRUB able to support the XFS filesystems with this
> feature enabled.
>
> The XFS counter for the bigtime enabled timestamps starts on 0, which
> translates to GRUB_INT32_MIN (Dec 31 20:45:52 UTC 1901) in the legacy
> timestamps. The conversion to unix timestamps is made before passing the
> value to other GRUB functions.
>
> For this to work properly, GRUB requires to access flags2 field in the
> XFS ondisk inode. So, the grub_xfs_inode structure has been updated to
> the full ondisk inode size.

s/the full ondisk inode size/cover full ondisk inode/?

> This patch is enough to make the GRUB work properly with files that have
> timestamps with values up to GRUB_INT32_MAX (Jan 19 03:14:07 UTC 2038).

This paragraph contradicts a bit what was said in the first paragraph...

> Any file with a timestamps bigger than this will overflow the counter,
> causing GRUB to show wrong timestamps. This is not really different than
> the current situation, since the timestamps in GRUB are 32-bit values.

Err... I think this is incorrect right now because the patch before this
one fixed the issue. So, this paragraph should be dropped. There is only
one question: are the changes here sufficient to have full XFS bigtime
support in the GRUB? I think they are...

> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> Changes in v2:
> - Improve the commit message of patch #3 (dkiper).
> - Drop Reviewed-by tag from patch #3 (dkiper).
> _ Drop unrelated changes in patch #3 (dkiper).
>
>  grub-core/fs/xfs.c | 63 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index 43023e03fb3..bcf281cf80d 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -75,10 +75,15 @@ GRUB_MOD_LICENSE ("GPLv3+");
>        XFS_SB_VERSION2_PROJID32BIT | \
>        XFS_SB_VERSION2_FTYPE)
>
> +/* Inode flags2 flags */
> +#define XFS_DIFLAG2_BIGTIME_BIT      3
> +#define XFS_DIFLAG2_BIGTIME  (1 << XFS_DIFLAG2_BIGTIME_BIT)
> +
>  /* incompat feature flags */
>  #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 
> */
>
>  /*
>   * Directory entries with ftype are explicitly handled by GRUB code.
> @@ -92,7 +97,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define XFS_SB_FEAT_INCOMPAT_SUPPORTED \
>       (XFS_SB_FEAT_INCOMPAT_FTYPE | \
>        XFS_SB_FEAT_INCOMPAT_SPINODES | \
> -      XFS_SB_FEAT_INCOMPAT_META_UUID)
> +      XFS_SB_FEAT_INCOMPAT_META_UUID | \
> +      XFS_SB_FEAT_INCOMPAT_BIGTIME)
>
>  struct grub_xfs_sblock
>  {
> @@ -177,7 +183,7 @@ struct grub_xfs_btree_root
>    grub_uint64_t keys[1];
>  } GRUB_PACKED;
>
> -struct grub_xfs_time
> +struct grub_xfs_time_legacy
>  {
>    grub_uint32_t sec;
>    grub_uint32_t nanosec;
> @@ -190,20 +196,23 @@ struct grub_xfs_inode
>    grub_uint8_t version;
>    grub_uint8_t format;
>    grub_uint8_t unused2[26];
> -  struct grub_xfs_time atime;
> -  struct grub_xfs_time mtime;
> -  struct grub_xfs_time ctime;
> +  grub_uint64_t atime;
> +  grub_uint64_t mtime;
> +  grub_uint64_t ctime;
>    grub_uint64_t size;
>    grub_uint64_t nblocks;
>    grub_uint32_t extsize;
>    grub_uint32_t nextents;
>    grub_uint16_t unused3;
>    grub_uint8_t fork_offset;
> -  grub_uint8_t unused4[17];
> +  grub_uint8_t unused4[37];
> +  grub_uint64_t flags2;
> +  grub_uint8_t unused5[48];
>  } GRUB_PACKED;
>
> -#define XFS_V2_INODE_SIZE sizeof(struct grub_xfs_inode)
> -#define XFS_V3_INODE_SIZE (XFS_V2_INODE_SIZE + 76)
> +#define XFS_V3_INODE_SIZE sizeof(struct grub_xfs_inode)
> +/* Size of struct grub_xfs_inode until fork_offset (included)*/
> +#define XFS_V2_INODE_SIZE (XFS_V3_INODE_SIZE - 92)
>
>  struct grub_xfs_dirblock_tail
>  {
> @@ -233,8 +242,6 @@ struct grub_xfs_data
>
>  static grub_dl_t my_mod;
>
> -
> -

Could you drop this change?

>  static int grub_xfs_sb_hascrc(struct grub_xfs_data *data)
>  {
>    return (data->sblock.version & 
> grub_cpu_to_be16_compile_time(XFS_SB_VERSION_NUMBITS)) ==
> @@ -950,7 +957,6 @@ grub_xfs_mount (grub_disk_t disk)
>    return 0;
>  }
>
> -

Ditto...

>  /* Context for grub_xfs_dir.  */
>  struct grub_xfs_dir_ctx
>  {
> @@ -958,6 +964,39 @@ struct grub_xfs_dir_ctx
>    void *hook_data;
>  };
>
> +/* Bigtime inodes helpers */
> +
> +#define NSEC_PER_SEC 1000000000L
> +#define XFS_BIGTIME_EPOCH_OFFSET (-(grub_int64_t)GRUB_INT32_MIN)

I think starting from here you forgot to take into account my comments...

> +static int grub_xfs_inode_has_bigtime(const struct grub_xfs_inode *inode)
> +{
> +  return inode->version >= 3 &&
> +     (inode->flags2 & grub_cpu_to_be64_compile_time(XFS_DIFLAG2_BIGTIME));
> +}
> +
> +static grub_int64_t
> +grub_xfs_bigtime_to_unix(grub_uint64_t time)
> +{
> +  grub_uint64_t rem;
> +  grub_int64_t nsec = NSEC_PER_SEC;
> +  grub_int64_t seconds = grub_divmod64((grub_int64_t)time, nsec, &rem);
> +
> +  return seconds - XFS_BIGTIME_EPOCH_OFFSET;
> +}
> +
> +static grub_int64_t
> +grub_xfs_get_inode_time(struct grub_xfs_inode *inode)
> +{
> +  struct grub_xfs_time_legacy *lts;
> +
> +  if (grub_xfs_inode_has_bigtime(inode))
> +    return grub_xfs_bigtime_to_unix(grub_be_to_cpu64(inode->mtime));
> +
> +  lts = (struct grub_xfs_time_legacy *)&inode->mtime;
> +  return grub_be_to_cpu32(lts->sec);
> +}
> +
>  /* Helper for grub_xfs_dir.  */
>  static int
>  grub_xfs_dir_iter (const char *filename, enum grub_fshelp_filetype filetype,
> @@ -970,7 +1009,7 @@ grub_xfs_dir_iter (const char *filename, enum 
> grub_fshelp_filetype filetype,
>    if (node->inode_read)
>      {
>        info.mtimeset = 1;
> -      info.mtime = grub_be_to_cpu32 (node->inode.mtime.sec);
> +      info.mtime = grub_xfs_get_inode_time(&node->inode);
>      }
>    info.dir = ((filetype & GRUB_FSHELP_TYPE_MASK) == GRUB_FSHELP_DIR);
>    grub_free (node);

Daniel