On Mon, Nov 18, 2019 at 11:30:58AM +0200, Amir Goldstein wrote:
> On Mon, Nov 18, 2019 at 10:22 AM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> >
> > On Mon, Nov 18, 2019 at 06:52:39AM +0200, Amir Goldstein wrote:
> > > > >
> > > > > I wonder if your version has struct xfs_dinode_v3 or it could avoid it.
> > > > > There is a benefit in terms of code complexity and test coverage
> > > > > to keep the only difference between inode versions in the on-disk
> > > > > parsers, while reading into the same struct, the same way as
> > > > > old inode versions are read into struct xfs_dinode.
> > > > >
> > > > > Oh well, I can wait for tomorrow to see the polished version :-)
> > > >
> > > > Well now we noticed that Arnd also changed the disk quota structure
> > > > format too, so that'll slow things down as we try to figure out how to
> > > > reconcile 34-bit inode seconds vs. 40-bit quota timer seconds.
> > > >
> > > > (Or whatever happens with that)
> > > >
> > >
> > > Sigh. FWIW, I liked Arnd's 40-bit inode time patch because it
> > > keeps the patch LoC for this conversion minimal.
> >
> > We can extend the quota warning range without changing the on-disk
> > structures, and with much less code than changing the on-disk
> > structures.
> >
> > We only need a ~500 year range for the warning expiry timestamp, and
> > we don't really care about fine grained resolution of the timer
> > expiry.
> >
> > We've already got a 70 year range with the signed second counter. So
> > let's just redefine the timeout value on disk to use units of 10s
> > instead of 1s when the bigtime superblock feature bit is set. ANd
> > now we have our >500 year range requirement.
> >
> > That shouldn't need much more than 5-10 lines of new code
> > translating the units when we read/write them from/to disk....
> >
>
> Sounds good.
>
> What is your take on the issue of keeping struct xfs_dinode
> and struct xfs_log_dinode common to v3..v4?
>
> If we make struct xfs_timestamp_t/xfs_ictimestamp_t a union
> of {{t_sec32;t_nsec32}, {t_nsec64}} then xfs_log_dinode_to_disk()
> conversion code is conditional to di_version.
> If we store v4 on-disk as {t_nsec32_hi;t_nsec32_lo} then the
> conversion code from disk to log is unconditional to di_version.
>
> Am I overthinking this?
Probably not. I changed both disk and log inodes to a union of the old
timestamp struct and a t_nsec64 (as you put it) and it works fine.
The quota timers I've simply reduced the timer resolution from 1s to 4s
to get us an extra two bits, and added a full-size in-core dquot timer
so that we only lose resolution if the fs gets unmounted. It's kind of
a dirty trick, though.
> Darrick,
>
> I am assuming you are working on the patch.
> If you would like me to re-post my patch with the decided
> on-disk formats for inode and a quota patch let me know.
Still working on it. In the meantime, I have a question for you and
Arnd: I've started writing more fstests. How does userspace query the
kernel to find out the supported range of timestamps? fstests currently
hardcodes it, but yuck. XFS could find an ioctl/sysfs knob to export
that info, but it really belongs in statvfs or something.
--D
> Thanks,
> Amir.
