| Message ID | 159847954327.2601708.9783406435973854389.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: widen timestamps to deal with y2038 | expand |
> + */ > +static inline xfs_ictimestamp_t > +xfs_inode_to_log_dinode_ts( > + const struct timespec64 tv) > +{ > + uint64_t t; > + > +#ifdef __LITTLE_ENDIAN > + t = ((uint64_t)tv.tv_nsec << 32) | ((uint64_t)tv.tv_sec & 0xffffffff); > +#elif __BIG_ENDIAN > + t = ((int64_t)tv.tv_sec << 32) | ((uint64_t)tv.tv_nsec & 0xffffffff); > +#else > +# error System is neither little nor big endian? > +#endif > + return t; Looking at this I wonder if we should just keep the struct and cast to it locally in the conversion functions, as that should take care of everything. Or just keep the union from the previous version, sorry..
On Thu, Aug 27, 2020 at 9:51 AM Christoph Hellwig <hch@infradead.org> wrote: > > > + */ > > +static inline xfs_ictimestamp_t > > +xfs_inode_to_log_dinode_ts( > > + const struct timespec64 tv) > > +{ > > + uint64_t t; > > + > > +#ifdef __LITTLE_ENDIAN > > + t = ((uint64_t)tv.tv_nsec << 32) | ((uint64_t)tv.tv_sec & 0xffffffff); > > +#elif __BIG_ENDIAN > > + t = ((int64_t)tv.tv_sec << 32) | ((uint64_t)tv.tv_nsec & 0xffffffff); > > +#else > > +# error System is neither little nor big endian? > > +#endif > > + return t; > > Looking at this I wonder if we should just keep the struct and cast > to it locally in the conversion functions, as that should take > care of everything. Or just keep the union from the previous version, > sorry.. Looking at this my eyes pop out. I realize that maintaining on-disk format of the log is challenging, so if there is no other technical solution that will be easier for humans to review and maintain going forward, I will step back and let others review this code. But it bears the question: do we have to support replaying on BE a log that was recorded on LE? Especially with so little BE machines around these days, this sounds like over design to me. Wouldn't it be better just to keep a bit in the log if it is LE or BE and refuse to replay it on the wrong architecture? Sure, we need to support whatever we supported up to now, but "bigtime" can require a new incompat log feature "host order" (or something). Anyway, I am probably mumbling utter garbage, do feel free to ignore or set me straight. Thanks, Amir.
On Thu, Aug 27, 2020 at 11:17:34AM +0300, Amir Goldstein wrote: > Looking at this my eyes pop out. > I realize that maintaining on-disk format of the log is challenging, > so if there is no other technical solution that will be easier for humans > to review and maintain going forward, I will step back and let others > review this code. > > But it bears the question: do we have to support replaying on BE a > log that was recorded on LE? Especially with so little BE machines > around these days, this sounds like over design to me. > Wouldn't it be better just to keep a bit in the log if it is LE or BE and refuse > to replay it on the wrong architecture? XFS has never supported replaying a BE log on LE machine or vice versa.
On Thu, Aug 27, 2020 at 11:18 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Aug 27, 2020 at 11:17:34AM +0300, Amir Goldstein wrote: > > Looking at this my eyes pop out. > > I realize that maintaining on-disk format of the log is challenging, > > so if there is no other technical solution that will be easier for humans > > to review and maintain going forward, I will step back and let others > > review this code. > > > > But it bears the question: do we have to support replaying on BE a > > log that was recorded on LE? Especially with so little BE machines > > around these days, this sounds like over design to me. > > Wouldn't it be better just to keep a bit in the log if it is LE or BE and refuse > > to replay it on the wrong architecture? > > XFS has never supported replaying a BE log on LE machine or vice versa. Right. Makes sense. I guess I read this backwards: https://lore.kernel.org/linux-xfs/20200825003945.GA6096@magnolia/T/#u Sorry for the noise. Thanks, Amir.
On Thu, Aug 27, 2020 at 07:51:14AM +0100, Christoph Hellwig wrote: > > + */ > > +static inline xfs_ictimestamp_t > > +xfs_inode_to_log_dinode_ts( > > + const struct timespec64 tv) > > +{ > > + uint64_t t; > > + > > +#ifdef __LITTLE_ENDIAN > > + t = ((uint64_t)tv.tv_nsec << 32) | ((uint64_t)tv.tv_sec & 0xffffffff); > > +#elif __BIG_ENDIAN > > + t = ((int64_t)tv.tv_sec << 32) | ((uint64_t)tv.tv_nsec & 0xffffffff); > > +#else > > +# error System is neither little nor big endian? > > +#endif > > + return t; > > Looking at this I wonder if we should just keep the struct and cast > to it locally in the conversion functions, as that should take > care of everything. Or just keep the union from the previous version, > sorry.. Yeah, thinking about this ugliness some more I think I'd rather just use a pointer cast here since the ifdef stuff is gross. --D
diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h index e3400c9c71cd..86640f8cd66a 100644 --- a/fs/xfs/libxfs/xfs_log_format.h +++ b/fs/xfs/libxfs/xfs_log_format.h @@ -368,10 +368,7 @@ static inline int xfs_ilog_fdata(int w) * directly mirrors the xfs_dinode structure as it must contain all the same * information. */ -typedef struct xfs_ictimestamp { - int32_t t_sec; /* timestamp seconds */ - int32_t t_nsec; /* timestamp nanoseconds */ -} xfs_ictimestamp_t; +typedef uint64_t xfs_ictimestamp_t; /* * Define the format of the inode core that is logged. This structure must be diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c index 6c65938cee1c..6ebc332ae446 100644 --- a/fs/xfs/xfs_inode_item.c +++ b/fs/xfs/xfs_inode_item.c @@ -295,6 +295,33 @@ xfs_inode_item_format_attr_fork( } } +/* + * Convert an incore timestamp to a log timestamp. Note that the log format + * specifies host endian format! + * + * For traditional timestamps, the log format specifies that the seconds are + * stored in the first four bytes and the nanoseconds in the second four. On a + * little-endian system, this means that we shift tv_nsec to put it in the + * upper four bytes and mask tv_sec to put it in the lower four bytes. On + * big-endian systems, we shift tv_sec to put it in the upper four bytes and + * mask tv_nsec to put it in the lower four bytes. + */ +static inline xfs_ictimestamp_t +xfs_inode_to_log_dinode_ts( + const struct timespec64 tv) +{ + uint64_t t; + +#ifdef __LITTLE_ENDIAN + t = ((uint64_t)tv.tv_nsec << 32) | ((uint64_t)tv.tv_sec & 0xffffffff); +#elif __BIG_ENDIAN + t = ((int64_t)tv.tv_sec << 32) | ((uint64_t)tv.tv_nsec & 0xffffffff); +#else +# error System is neither little nor big endian? +#endif + return t; +} + static void xfs_inode_to_log_dinode( struct xfs_inode *ip, @@ -313,12 +340,9 @@ xfs_inode_to_log_dinode( memset(to->di_pad, 0, sizeof(to->di_pad)); memset(to->di_pad3, 0, sizeof(to->di_pad3)); - to->di_atime.t_sec = inode->i_atime.tv_sec; - to->di_atime.t_nsec = inode->i_atime.tv_nsec; - to->di_mtime.t_sec = inode->i_mtime.tv_sec; - to->di_mtime.t_nsec = inode->i_mtime.tv_nsec; - to->di_ctime.t_sec = inode->i_ctime.tv_sec; - to->di_ctime.t_nsec = inode->i_ctime.tv_nsec; + to->di_atime = xfs_inode_to_log_dinode_ts(inode->i_atime); + to->di_mtime = xfs_inode_to_log_dinode_ts(inode->i_mtime); + to->di_ctime = xfs_inode_to_log_dinode_ts(inode->i_ctime); to->di_nlink = inode->i_nlink; to->di_gen = inode->i_generation; to->di_mode = inode->i_mode; @@ -340,8 +364,7 @@ xfs_inode_to_log_dinode( if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) { to->di_version = 3; to->di_changecount = inode_peek_iversion(inode); - to->di_crtime.t_sec = from->di_crtime.tv_sec; - to->di_crtime.t_nsec = from->di_crtime.tv_nsec; + to->di_crtime = xfs_inode_to_log_dinode_ts(from->di_crtime); to->di_flags2 = from->di_flags2; to->di_cowextsize = from->di_cowextsize; to->di_ino = ip->i_ino; diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c index e7c6f2b95e17..bbb820579ea2 100644 --- a/fs/xfs/xfs_inode_item_recover.c +++ b/fs/xfs/xfs_inode_item_recover.c @@ -117,15 +117,34 @@ xfs_recover_inode_owner_change( /* * Convert a log timestamp to an ondisk timestamp. See notes about the ondisk - * encoding in the comments for xfs_inode_to_disk_ts. + * encoding in the comments for xfs_inode_to_disk_ts. Note that the log format + * specifies host endian format! + * + * For traditional timestamps, the log format specifies that the seconds are + * stored in the first four bytes and the nanoseconds in the second four. On a + * little-endian system, this means that we shift tv_nsec to extract it from + * the upper four bytes and mask tv_sec to extract it from the lower four + * bytes. On big-endian systems, we shift tv_sec to extract it from the upper + * four bytes and mask tv_nsec to extract it from the lower four bytes. */ static inline xfs_timestamp_t xfs_log_dinode_to_disk_ts( const xfs_ictimestamp_t its) { + struct timespec64 tv; uint64_t t; - t = ((int64_t)its.t_sec << 32) | (its.t_nsec & 0xffffffff); +#ifdef __LITTLE_ENDIAN + tv.tv_sec = (time64_t)its & 0xffffffff; + tv.tv_nsec = (int64_t)its >> 32; +#elif __BIG_ENDIAN + tv.tv_sec = (time64_t)its >> 32; + tv.tv_nsec = (int64_t)its & 0xffffffff; +#else +# error System is neither little nor big endian? +#endif + + t = ((int64_t)tv.tv_sec << 32) | (tv.tv_nsec & 0xffffffff); return cpu_to_be64(t); } diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h index 2f11a6f9e005..42b940e9b2b3 100644 --- a/fs/xfs/xfs_ondisk.h +++ b/fs/xfs/xfs_ondisk.h @@ -121,7 +121,7 @@ xfs_check_ondisk_structs(void) XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64, 16); XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode, 176); XFS_CHECK_STRUCT_SIZE(struct xfs_icreate_log, 28); - XFS_CHECK_STRUCT_SIZE(struct xfs_ictimestamp, 8); + XFS_CHECK_STRUCT_SIZE(xfs_ictimestamp_t, 8); XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format_32, 52); XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format, 56); XFS_CHECK_STRUCT_SIZE(struct xfs_qoff_logformat, 20);