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

[Javier Martinez Canillas]

Hello Daniel,

On 5/17/21 4:09 PM, Daniel Kiper wrote:
> 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/?
>

Ok.
 
>> 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...
>

Yes, sorry about that. Originally Carlos' series had this patch before 1/4
and I re-arranged them in the series because made more sense to first add
the 64-bit support in GRUB core and then the XFS support. I'll drop this.

[snip]

>>
>>  static grub_dl_t my_mod;
>>
>> -
>> -
>

Ok.
 
> 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...
>

Ok.
 
>>  /* 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...
>

Oh, I did indeed miss these comments... sorry, I'll fix them in v3.

Best regards, -- 
Javier Martinez Canillas
Software Engineer
New Platform Technologies Enablement team
RHEL Engineering