| Message ID | E1ieGtm-0004ZY-DD@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/adfs updates for 5.6 | expand |
On Mon, 2019-12-09 at 11:08 +0000, Russell King wrote: > Despite ADFS timestamps having centi-second granularity, and Linux > gaining fine-grained timestamp support in v2.5.48, fs/adfs was never > updated. > > Update fs/adfs to centi-second support, and ensure that the inode > ctime > always reflects what is written in underlying media. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > fs/adfs/inode.c | 40 ++++++++++++++++++++-------------------- > fs/adfs/super.c | 2 ++ > 2 files changed, 22 insertions(+), 20 deletions(-) > > diff --git a/fs/adfs/inode.c b/fs/adfs/inode.c > index 124de75413a5..18a1d478669b 100644 > --- a/fs/adfs/inode.c > +++ b/fs/adfs/inode.c > @@ -158,6 +158,8 @@ adfs_mode2atts(struct super_block *sb, struct > inode *inode) > return attr; > } > > +static const s64 nsec_unix_epoch_diff_risc_os_epoch = > 2208988800000000000LL; > + > /* > * Convert an ADFS time to Unix time. ADFS has a 40-bit centi- > second time > * referenced to 1 Jan 1900 (til 2248) so we need to discard > 2208988800 seconds > @@ -170,8 +172,6 @@ adfs_adfs2unix_time(struct timespec64 *tv, struct > inode *inode) > /* 01 Jan 1970 00:00:00 (Unix epoch) as nanoseconds since > * 01 Jan 1900 00:00:00 (RISC OS epoch) > */ > - static const s64 nsec_unix_epoch_diff_risc_os_epoch = > - 220898880000000 > 0000LL; > s64 nsec; > > if (!adfs_inode_is_stamped(inode)) > @@ -204,24 +204,23 @@ adfs_adfs2unix_time(struct timespec64 *tv, > struct inode *inode) > return; > } > > -/* > - * Convert an Unix time to ADFS time. We only do this if the entry > has a > - * time/date stamp already. > - */ > -static void > -adfs_unix2adfs_time(struct inode *inode, unsigned int secs) > +/* Convert an Unix time to ADFS time for an entry that is already > stamped. */ > +static void adfs_unix2adfs_time(struct inode *inode, > + const struct timespec64 *ts) > { > - unsigned int high, low; > + s64 cs, nsec = timespec64_to_ns(ts); > > - if (adfs_inode_is_stamped(inode)) { > - /* convert 32-bit seconds to 40-bit centi-seconds */ > - low = (secs & 255) * 100; > - high = (secs / 256) * 100 + (low >> 8) + 0x336e996a; > + /* convert from Unix to RISC OS epoch */ > + nsec += nsec_unix_epoch_diff_risc_os_epoch; > > - ADFS_I(inode)->loadaddr = (high >> 24) | > - (ADFS_I(inode)->loadaddr & ~0xff); > - ADFS_I(inode)->execaddr = (low & 255) | (high << 8); > - } > + /* convert from nanoseconds to centiseconds */ > + cs = div_s64(nsec, 10000000); > + > + cs = clamp_t(s64, cs, 0, 0xffffffffff); > + > + ADFS_I(inode)->loadaddr &= ~0xff; > + ADFS_I(inode)->loadaddr |= (cs >> 32) & 0xff; > + ADFS_I(inode)->execaddr = cs; > } > > /* > @@ -315,10 +314,11 @@ adfs_notify_change(struct dentry *dentry, > struct iattr *attr) > if (ia_valid & ATTR_SIZE) > truncate_setsize(inode, attr->ia_size); > > - if (ia_valid & ATTR_MTIME) { > - inode->i_mtime = attr->ia_mtime; > - adfs_unix2adfs_time(inode, attr->ia_mtime.tv_sec); > + if (ia_valid & ATTR_MTIME && adfs_inode_is_stamped(inode)) { > + adfs_unix2adfs_time(inode, &attr->ia_mtime); > + adfs_adfs2unix_time(&inode->i_mtime, inode); > } > + > /* > * FIXME: should we make these == to i_mtime since we don't > * have the ability to represent them in our filesystem? > diff --git a/fs/adfs/super.c b/fs/adfs/super.c > index 65b04ebb51c3..e0eea9adb4e6 100644 > --- a/fs/adfs/super.c > +++ b/fs/adfs/super.c > @@ -391,7 +391,9 @@ static int adfs_fill_super(struct super_block > *sb, void *data, int silent) > asb = kzalloc(sizeof(*asb), GFP_KERNEL); > if (!asb) > return -ENOMEM; > + > sb->s_fs_info = asb; > + sb->s_time_gran = 10000000; I believe it's not easy to follow what this granularity means. Maybe, it makes sense to introduce some constant and to add some comment? Thanks, Viacheslav Dubeyko. > > /* set default options */ > asb->s_uid = GLOBAL_ROOT_UID;
On Mon, Dec 09, 2019 at 04:54:55PM +0300, Vyacheslav Dubeyko wrote: > On Mon, 2019-12-09 at 11:08 +0000, Russell King wrote: > > Despite ADFS timestamps having centi-second granularity, and Linux > > gaining fine-grained timestamp support in v2.5.48, fs/adfs was never > > updated. > > > > Update fs/adfs to centi-second support, and ensure that the inode > > ctime > > always reflects what is written in underlying media. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > --- > > fs/adfs/inode.c | 40 ++++++++++++++++++++-------------------- > > fs/adfs/super.c | 2 ++ > > 2 files changed, 22 insertions(+), 20 deletions(-) > > > > diff --git a/fs/adfs/inode.c b/fs/adfs/inode.c > > index 124de75413a5..18a1d478669b 100644 > > --- a/fs/adfs/inode.c > > +++ b/fs/adfs/inode.c > > @@ -158,6 +158,8 @@ adfs_mode2atts(struct super_block *sb, struct > > inode *inode) > > return attr; > > } > > > > +static const s64 nsec_unix_epoch_diff_risc_os_epoch = > > 2208988800000000000LL; > > + > > /* > > * Convert an ADFS time to Unix time. ADFS has a 40-bit centi- > > second time > > * referenced to 1 Jan 1900 (til 2248) so we need to discard > > 2208988800 seconds > > @@ -170,8 +172,6 @@ adfs_adfs2unix_time(struct timespec64 *tv, struct > > inode *inode) > > /* 01 Jan 1970 00:00:00 (Unix epoch) as nanoseconds since > > * 01 Jan 1900 00:00:00 (RISC OS epoch) > > */ > > - static const s64 nsec_unix_epoch_diff_risc_os_epoch = > > - 220898880000000 > > 0000LL; > > s64 nsec; > > > > if (!adfs_inode_is_stamped(inode)) > > @@ -204,24 +204,23 @@ adfs_adfs2unix_time(struct timespec64 *tv, > > struct inode *inode) > > return; > > } > > > > -/* > > - * Convert an Unix time to ADFS time. We only do this if the entry > > has a > > - * time/date stamp already. > > - */ > > -static void > > -adfs_unix2adfs_time(struct inode *inode, unsigned int secs) > > +/* Convert an Unix time to ADFS time for an entry that is already > > stamped. */ > > +static void adfs_unix2adfs_time(struct inode *inode, > > + const struct timespec64 *ts) > > { > > - unsigned int high, low; > > + s64 cs, nsec = timespec64_to_ns(ts); > > > > - if (adfs_inode_is_stamped(inode)) { > > - /* convert 32-bit seconds to 40-bit centi-seconds */ > > - low = (secs & 255) * 100; > > - high = (secs / 256) * 100 + (low >> 8) + 0x336e996a; > > + /* convert from Unix to RISC OS epoch */ > > + nsec += nsec_unix_epoch_diff_risc_os_epoch; > > > > - ADFS_I(inode)->loadaddr = (high >> 24) | > > - (ADFS_I(inode)->loadaddr & ~0xff); > > - ADFS_I(inode)->execaddr = (low & 255) | (high << 8); > > - } > > + /* convert from nanoseconds to centiseconds */ > > + cs = div_s64(nsec, 10000000); > > + > > + cs = clamp_t(s64, cs, 0, 0xffffffffff); > > + > > + ADFS_I(inode)->loadaddr &= ~0xff; > > + ADFS_I(inode)->loadaddr |= (cs >> 32) & 0xff; > > + ADFS_I(inode)->execaddr = cs; > > } > > > > /* > > @@ -315,10 +314,11 @@ adfs_notify_change(struct dentry *dentry, > > struct iattr *attr) > > if (ia_valid & ATTR_SIZE) > > truncate_setsize(inode, attr->ia_size); > > > > - if (ia_valid & ATTR_MTIME) { > > - inode->i_mtime = attr->ia_mtime; > > - adfs_unix2adfs_time(inode, attr->ia_mtime.tv_sec); > > + if (ia_valid & ATTR_MTIME && adfs_inode_is_stamped(inode)) { > > + adfs_unix2adfs_time(inode, &attr->ia_mtime); > > + adfs_adfs2unix_time(&inode->i_mtime, inode); > > } > > + > > /* > > * FIXME: should we make these == to i_mtime since we don't > > * have the ability to represent them in our filesystem? > > diff --git a/fs/adfs/super.c b/fs/adfs/super.c > > index 65b04ebb51c3..e0eea9adb4e6 100644 > > --- a/fs/adfs/super.c > > +++ b/fs/adfs/super.c > > @@ -391,7 +391,9 @@ static int adfs_fill_super(struct super_block > > *sb, void *data, int silent) > > asb = kzalloc(sizeof(*asb), GFP_KERNEL); > > if (!asb) > > return -ENOMEM; > > + > > sb->s_fs_info = asb; > > + sb->s_time_gran = 10000000; > > I believe it's not easy to follow what this granularity means. Maybe, > it makes sense to introduce some constant and to add some comment? Or simply name it "s_time_gran_ns" so the units are in the name.
On Mon, 2019-12-09 at 14:03 +0000, Russell King - ARM Linux admin wrote: > > > > On Mon, Dec 09, 2019 at 04:54:55PM +0300, Vyacheslav Dubeyko wrote: > > On Mon, 2019-12-09 at 11:08 +0000, Russell King wrote: <snipped> > > > sb->s_fs_info = asb; > > > + sb->s_time_gran = 10000000; > > > > I believe it's not easy to follow what this granularity means. > > Maybe, > > it makes sense to introduce some constant and to add some comment? > > Or simply name it "s_time_gran_ns" so the units are in the name. > Sounds good. :) But why namely 10000000? Thanks, Viacheslav Dubeyko.
On Mon, Dec 09, 2019 at 05:34:05PM +0300, Vyacheslav Dubeyko wrote: > On Mon, 2019-12-09 at 14:03 +0000, Russell King - ARM Linux admin > wrote: > > > > > > On Mon, Dec 09, 2019 at 04:54:55PM +0300, Vyacheslav Dubeyko wrote: > > > On Mon, 2019-12-09 at 11:08 +0000, Russell King wrote: > > <snipped> > > > > > sb->s_fs_info = asb; > > > > + sb->s_time_gran = 10000000; > > > > > > I believe it's not easy to follow what this granularity means. > > > Maybe, > > > it makes sense to introduce some constant and to add some comment? > > > > Or simply name it "s_time_gran_ns" so the units are in the name. > > > > Sounds good. :) But why namely 10000000? I don't know what you mean. If you're asking, why "10000000", isn't it obvious if you read the commit message? adfs has "centi-second" granularity. s_time_gran is in nanoseconds. There are 10000000 nanoseconds in a centisecond. What do you expect? #define ADFS_TIME_GRAN 10000000 sb->s_time_gran = ADFS_TIME_GRAN; ? How does that help - it just stupidly and needlessly obfuscates the code. The whole "use definitions for constants" is idiotic when a constant is only used in one place - when it means you have to search through more source code to find it's single definition. Sorry, I'm not doing that and make readability *worse*.
On Mon, Dec 09, 2019 at 02:40:00PM +0000, Russell King - ARM Linux admin wrote: > > Sounds good. :) But why namely 10000000? > > I don't know what you mean. > > If you're asking, why "10000000", isn't it obvious if you read the > commit message? adfs has "centi-second" granularity. s_time_gran > is in nanoseconds. There are 10000000 nanoseconds in a centisecond. > > What do you expect? > > #define ADFS_TIME_GRAN 10000000 > > sb->s_time_gran = ADFS_TIME_GRAN; > > ? > > How does that help - it just stupidly and needlessly obfuscates the > code. > > The whole "use definitions for constants" is idiotic when a constant > is only used in one place - when it means you have to search through > more source code to find it's single definition. Sorry, I'm not > doing that and make readability *worse*. I'd find it more readable if you wrote it as 10 * 1000 * 1000. Saves trying to count zeroes. I know C added the ability to spell that as 10'000'000, but I don't think all compiler versions support that yet. Maybe this would be cleanest: sb->s_time_gran = NSEC_PER_SEC / 100; This is definitely how not to do it: include/acpi/actypes.h:#define ACPI_100NSEC_PER_SEC 10000000L
diff --git a/fs/adfs/inode.c b/fs/adfs/inode.c index 124de75413a5..18a1d478669b 100644 --- a/fs/adfs/inode.c +++ b/fs/adfs/inode.c @@ -158,6 +158,8 @@ adfs_mode2atts(struct super_block *sb, struct inode *inode) return attr; } +static const s64 nsec_unix_epoch_diff_risc_os_epoch = 2208988800000000000LL; + /* * Convert an ADFS time to Unix time. ADFS has a 40-bit centi-second time * referenced to 1 Jan 1900 (til 2248) so we need to discard 2208988800 seconds @@ -170,8 +172,6 @@ adfs_adfs2unix_time(struct timespec64 *tv, struct inode *inode) /* 01 Jan 1970 00:00:00 (Unix epoch) as nanoseconds since * 01 Jan 1900 00:00:00 (RISC OS epoch) */ - static const s64 nsec_unix_epoch_diff_risc_os_epoch = - 2208988800000000000LL; s64 nsec; if (!adfs_inode_is_stamped(inode)) @@ -204,24 +204,23 @@ adfs_adfs2unix_time(struct timespec64 *tv, struct inode *inode) return; } -/* - * Convert an Unix time to ADFS time. We only do this if the entry has a - * time/date stamp already. - */ -static void -adfs_unix2adfs_time(struct inode *inode, unsigned int secs) +/* Convert an Unix time to ADFS time for an entry that is already stamped. */ +static void adfs_unix2adfs_time(struct inode *inode, + const struct timespec64 *ts) { - unsigned int high, low; + s64 cs, nsec = timespec64_to_ns(ts); - if (adfs_inode_is_stamped(inode)) { - /* convert 32-bit seconds to 40-bit centi-seconds */ - low = (secs & 255) * 100; - high = (secs / 256) * 100 + (low >> 8) + 0x336e996a; + /* convert from Unix to RISC OS epoch */ + nsec += nsec_unix_epoch_diff_risc_os_epoch; - ADFS_I(inode)->loadaddr = (high >> 24) | - (ADFS_I(inode)->loadaddr & ~0xff); - ADFS_I(inode)->execaddr = (low & 255) | (high << 8); - } + /* convert from nanoseconds to centiseconds */ + cs = div_s64(nsec, 10000000); + + cs = clamp_t(s64, cs, 0, 0xffffffffff); + + ADFS_I(inode)->loadaddr &= ~0xff; + ADFS_I(inode)->loadaddr |= (cs >> 32) & 0xff; + ADFS_I(inode)->execaddr = cs; } /* @@ -315,10 +314,11 @@ adfs_notify_change(struct dentry *dentry, struct iattr *attr) if (ia_valid & ATTR_SIZE) truncate_setsize(inode, attr->ia_size); - if (ia_valid & ATTR_MTIME) { - inode->i_mtime = attr->ia_mtime; - adfs_unix2adfs_time(inode, attr->ia_mtime.tv_sec); + if (ia_valid & ATTR_MTIME && adfs_inode_is_stamped(inode)) { + adfs_unix2adfs_time(inode, &attr->ia_mtime); + adfs_adfs2unix_time(&inode->i_mtime, inode); } + /* * FIXME: should we make these == to i_mtime since we don't * have the ability to represent them in our filesystem? diff --git a/fs/adfs/super.c b/fs/adfs/super.c index 65b04ebb51c3..e0eea9adb4e6 100644 --- a/fs/adfs/super.c +++ b/fs/adfs/super.c @@ -391,7 +391,9 @@ static int adfs_fill_super(struct super_block *sb, void *data, int silent) asb = kzalloc(sizeof(*asb), GFP_KERNEL); if (!asb) return -ENOMEM; + sb->s_fs_info = asb; + sb->s_time_gran = 10000000; /* set default options */ asb->s_uid = GLOBAL_ROOT_UID;
Despite ADFS timestamps having centi-second granularity, and Linux gaining fine-grained timestamp support in v2.5.48, fs/adfs was never updated. Update fs/adfs to centi-second support, and ensure that the inode ctime always reflects what is written in underlying media. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- fs/adfs/inode.c | 40 ++++++++++++++++++++-------------------- fs/adfs/super.c | 2 ++ 2 files changed, 22 insertions(+), 20 deletions(-)