[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [2.06 RELEASE PATCH 2/3] fs/xfs: Add bigtime incompat feature suppor
|
From: |
Daniel Kiper |
|
Subject: |
Re: [2.06 RELEASE PATCH 2/3] fs/xfs: Add bigtime incompat feature support |
|
Date: |
Wed, 5 May 2021 17:03:39 +0200 |
|
User-agent: |
NeoMutt/20170113 (1.7.2) |
On Wed, May 05, 2021 at 10:32:32AM +0200, Javier Martinez Canillas wrote:
> From: Carlos Maiolino <cmaiolino@redhat.com>
>
> XFS filesystem now supports bigtime feature, to overcome y2038 problem.
s/XFS/The XFS/
> This patch makes grub able to support xfs filesystems with this feature
s/grub/the GRUB/
s/xfs/the XFS/
Same below please...
> enabled.
>
> xfs counter for bigtime enable timestamps starts on 0, which translates
s/enable/enabled/
> to INT32_MIN (Dec 31 20:45:52 UTC 1901) in the legacy timestamps. The
s/INT32_MIN/GRUB_INT32_MIN/
> conversion to unix timestamps is made before passing the value to
> grub-core.
s/grub-core/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
s/, so,/. So,/
> the full ondisk inode size.
>
> This patch is enough to make grub work properly with files with
> timestamps up to INT32_MAX (y2038), any file with timestamps bigger than
> this will overflow the counter, causing grub to show wrong timestamps
> (not really much difference on current situation).
I am not sure what you want say here...
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Please drop this line...
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> grub-core/fs/xfs.c | 69 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 54 insertions(+), 15 deletions(-)
>
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index 43023e03fb3..2ce76ec70f9 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_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 */
I think this change requires comment in the commit message.
> +#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;
>
> -
> -
Please 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 */
> +
Please drop this empty line.
> +#define NSEC_PER_SEC 1000000000L
#define NSEC_PER_SEC ((grub_int64_t) 1000000000)
Should not we define this in include/grub/time.h?
> +#define XFS_BIGTIME_EPOCH_OFFSET (-(grub_int64_t)GRUB_INT32_MIN)
(-(grub_int64_t) GRUB_INT32_MIN)
Missing space...
> +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));
grub_cpu_to_be64_compile_time (XFS_DIFLAG2_BIGTIME)
Missing space... Please fix similar issues below...
> +}
> +
> +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;
return grub_divmod64 (time, NSEC_PER_SEC, NULL) - XFS_BIGTIME_EPOCH_OFFSET;
And you can drop the rest of the body of this function...
> +}
> +
> +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;
lts = (struct grub_xfs_time_legacy *) &inode->mtime;
Missing space...
> + return grub_be_to_cpu32(lts->sec);
Ditto...
> +}
> +
> /* 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);
Missing space...
Daniel