[01/41] fs/adfs: inode: update timestamps to centisecond precision
diff mbox series

Message ID E1ieGtm-0004ZY-DD@rmk-PC.armlinux.org.uk
State New
Headers show
Series
  • fs/adfs updates for 5.6
Related show

Commit Message

Russell King Dec. 9, 2019, 11:08 a.m. UTC
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(-)

Comments

Vyacheslav Dubeyko Dec. 9, 2019, 1:54 p.m. UTC | #1
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;
Russell King - ARM Linux admin Dec. 9, 2019, 2:03 p.m. UTC | #2
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.
Vyacheslav Dubeyko Dec. 9, 2019, 2:34 p.m. UTC | #3
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.
Russell King - ARM Linux admin Dec. 9, 2019, 2:40 p.m. UTC | #4
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*.
Matthew Wilcox Dec. 9, 2019, 5:15 p.m. UTC | #5
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

Patch
diff mbox series

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;