[PATCH] xfsrestore: use utimensat() to provide atime/mtime with ns resolution

Discussion:

Brian Foster

2014-09-04 16:38:28 UTC

xfsdump encodes and stores the full atime and mtime for each file with
nanosecond resolution. xfsrestore uses utime() to set the times of each
file that is restored. The latter supports resolution of 1 second, thus
sub-second timestamp data is lost on restore.

Add the associated configure checks for and use utimensat() when
available to restore timestamps with nanosecond resolution. Create a new
helper to facilitate conditional support for utimensat().

Signed-off-by: Brian Foster <***@redhat.com>
---

I'm not totally sure on the configure bits here... it's mostly copied
from xfsprogs and tweaked appropriately. It seems to work, at least. ;)

Brian

configure.ac | 2 ++
include/builddefs.in | 1 +
m4/Makefile | 1 +
m4/package_libcdev.m4 | 16 ++++++++++++++++
restore/Makefile | 4 ++++
restore/content.c | 38 ++++++++++++++++++++++++++++++--------
6 files changed, 54 insertions(+), 8 deletions(-)
create mode 100644 m4/package_libcdev.m4

diff --git a/configure.ac b/configure.ac
index 59f9564..28e604e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -82,6 +82,8 @@ AC_PACKAGE_NEED_ATTRIBUTES_H
AC_PACKAGE_NEED_ATTRIBUTES_MACROS
AC_PACKAGE_NEED_ATTRGET_LIBATTR

+AC_HAVE_UTIMENSAT
+
AC_MANUAL_FORMAT

AC_CONFIG_FILES([include/builddefs])
diff --git a/include/builddefs.in b/include/builddefs.in
index 269c928..bdf0ede 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -69,6 +69,7 @@ ENABLE_SHARED = @enable_shared@
ENABLE_GETTEXT = @enable_gettext@

HAVE_ZIPPED_MANPAGES = @have_zipped_manpages@
+HAVE_UTIMENSAT = @have_utimensat@

GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall
# -Wbitwise -Wno-transparent-union -Wno-old-initializer -Wno-decl
diff --git a/m4/Makefile b/m4/Makefile
index 9a35056..ae452f7 100644
--- a/m4/Makefile
+++ b/m4/Makefile
@@ -16,6 +16,7 @@ LSRCFILES = \
manual_format.m4 \
package_attrdev.m4 \
package_globals.m4 \
+ package_libcdev.m4 \
package_ncurses.m4 \
package_pthread.m4 \
package_utilies.m4 \
diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
new file mode 100644
index 0000000..6fcbdba
--- /dev/null
+++ b/m4/package_libcdev.m4
@@ -0,0 +1,16 @@
+#
+# Check if we have a utimensat libc call
+#
+AC_DEFUN([AC_HAVE_UTIMENSAT],
+ [ AC_MSG_CHECKING([for utimensat])
+ AC_TRY_COMPILE([
+#define _ATFILE_SOURCE
+#include <fcntl.h>
+#include <sys/stat.h>
+ ], [
+ utimensat(AT_FDCWD, 0, 0, AT_SYMLINK_NOFOLLOW);
+ ], have_utimensat=yes
+ AC_MSG_RESULT(yes),
+ AC_MSG_RESULT(no))
+ AC_SUBST(have_utimensat)
+ ])
diff --git a/restore/Makefile b/restore/Makefile
index c6f3f25..c5cf925 100644
--- a/restore/Makefile
+++ b/restore/Makefile
@@ -102,6 +102,10 @@ LTDEPENDENCIES = $(LIBRMT)

LCFLAGS = -DRESTORE

+ifeq ($(HAVE_UTIMENSAT),yes)
+LCFLAGS += -DHAVE_UTIMENSAT
+endif
+
default: depend $(LTCOMMAND)

include $(BUILDRULES)
diff --git a/restore/content.c b/restore/content.c
index cfcf94d..65dc2f5 100644
--- a/restore/content.c
+++ b/restore/content.c
@@ -7418,6 +7418,34 @@ done:
return 0;
}

+/*
+ * Set the access and modification times for a file.
+ */
+static int
+restore_file_amtime(
+ const char *path,
+ struct bstat *bstatp)
+{
+ int rval;
+
+#ifdef HAVE_UTIMENSAT
+ struct timespec times[2];
+
+ times[0].tv_sec = bstatp->bs_atime.tv_sec;
+ times[0].tv_nsec = bstatp->bs_atime.tv_nsec;
+ times[1].tv_sec = bstatp->bs_mtime.tv_sec;
+ times[1].tv_nsec = bstatp->bs_mtime.tv_nsec;
+ rval = utimensat(AT_FDCWD, path, times, 0);
+#else
+ struct utimbuf utimbuf;
+
+ utimbuf.actime = ( time32_t )bstatp->bs_atime.tv_sec;
+ utimbuf.modtime = ( time32_t )bstatp->bs_mtime.tv_sec;
+ rval = utime(path, &utimbuf);
+#endif
+ return rval;
+}
+
/* called to begin a regular file. if no path given, or if just toc,
* don't actually write, just read. also get into that situation if
* cannot prepare destination. fd == -1 signifies no write. *statp
@@ -7671,7 +7699,6 @@ restore_complete_reg(stream_context_t *strcxtp)
bstat_t *bstatp = &strcxtp->sc_bstat;
char *path = strcxtp->sc_path;
intgen_t fd = strcxtp->sc_fd;
- struct utimbuf utimbuf;
intgen_t rval;

// only applies to regular files
@@ -7688,9 +7715,7 @@ restore_complete_reg(stream_context_t *strcxtp)

/* set the access and modification times
*/
- utimbuf.actime = ( time32_t )bstatp->bs_atime.tv_sec;
- utimbuf.modtime = ( time32_t )bstatp->bs_mtime.tv_sec;
- rval = utime( path, &utimbuf );
+ rval = restore_file_amtime(path, bstatp);
if ( rval ) {
mlog( MLOG_VERBOSE | MLOG_WARNING, _(
"unable to set access and modification "
@@ -7770,7 +7795,6 @@ static bool_t
restore_spec( filehdr_t *fhdrp, rv_t *rvp, char *path )
{
bstat_t *bstatp = &fhdrp->fh_stat;
- struct utimbuf utimbuf;
char *printstr;
intgen_t rval;

@@ -7913,9 +7937,7 @@ restore_spec( filehdr_t *fhdrp, rv_t *rvp, char *path )

/* set the access and modification times
*/
- utimbuf.actime = ( time32_t )bstatp->bs_atime.tv_sec;
- utimbuf.modtime = ( time32_t )bstatp->bs_mtime.tv_sec;
- rval = utime( path, &utimbuf );
+ rval = restore_file_amtime(path, bstatp);
if ( rval ) {
mlog( MLOG_VERBOSE | MLOG_WARNING, _(
"unable to set access and modification "

--
1.8.3.1

Eric Sandeen

2014-09-04 19:47:00 UTC

Permalink

Post by Brian Foster
xfsdump encodes and stores the full atime and mtime for each file with
nanosecond resolution. xfsrestore uses utime() to set the times of each
file that is restored. The latter supports resolution of 1 second, thus
sub-second timestamp data is lost on restore.
Add the associated configure checks for and use utimensat() when
available to restore timestamps with nanosecond resolution. Create a new
helper to facilitate conditional support for utimensat().
---
I'm not totally sure on the configure bits here... it's mostly copied
from xfsprogs and tweaked appropriately. It seems to work, at least. ;)
Brian
configure.ac | 2 ++
include/builddefs.in | 1 +
m4/Makefile | 1 +
m4/package_libcdev.m4 | 16 ++++++++++++++++
restore/Makefile | 4 ++++
restore/content.c | 38 ++++++++++++++++++++++++++++++--------
6 files changed, 54 insertions(+), 8 deletions(-)
create mode 100644 m4/package_libcdev.m4
diff --git a/configure.ac b/configure.ac
index 59f9564..28e604e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -82,6 +82,8 @@ AC_PACKAGE_NEED_ATTRIBUTES_H
AC_PACKAGE_NEED_ATTRIBUTES_MACROS
AC_PACKAGE_NEED_ATTRGET_LIBATTR
+AC_HAVE_UTIMENSAT
+
AC_MANUAL_FORMAT
AC_CONFIG_FILES([include/builddefs])
diff --git a/include/builddefs.in b/include/builddefs.in
index 269c928..bdf0ede 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -69,6 +69,7 @@ ENABLE_SHARED = @enable_shared@
GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall
# -Wbitwise -Wno-transparent-union -Wno-old-initializer -Wno-decl
diff --git a/m4/Makefile b/m4/Makefile
index 9a35056..ae452f7 100644
--- a/m4/Makefile
+++ b/m4/Makefile
@@ -16,6 +16,7 @@ LSRCFILES = \
manual_format.m4 \
package_attrdev.m4 \
package_globals.m4 \
+ package_libcdev.m4 \
package_ncurses.m4 \
package_pthread.m4 \
package_utilies.m4 \
diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
new file mode 100644
index 0000000..6fcbdba
--- /dev/null
+++ b/m4/package_libcdev.m4
@@ -0,0 +1,16 @@
+#
+# Check if we have a utimensat libc call
+#
+AC_DEFUN([AC_HAVE_UTIMENSAT],
+ [ AC_MSG_CHECKING([for utimensat])
+ AC_TRY_COMPILE([

(as discussed on IRC...)

I think you need AC_TRY_LINK here; changing utimensat() to utimensatFOO() still
passes. ;)

-Eric

Brian Foster

2014-09-04 21:35:12 UTC

Permalink

Post by Eric Sandeen

(as discussed on IRC...)
I think you need AC_TRY_LINK here; changing utimensat() to utimensatFOO() still
passes. ;)

Yeah... I tested that by breaking the function signature. I'm guessing
this does a 'gcc -c,' which lets me compile pretty much any kind of call
if no function declaration is found in a header. AC_TRY_LINK() does what
we want. Thanks for catching that.

Brian

Post by Eric Sandeen
-Eric

Dave Chinner

2014-09-05 00:45:01 UTC

Permalink

That doesn't seem like a big deal. What sort of problems does this
actually cause?

FYI, many linux filesystems only have second resolution timestamps
and hence applications can't rely on sub-second timestamp resolution
to actually mean anything useful....

Cheers,

Dave.

--
Dave Chinner
***@fromorbit.com

Eric Sandeen

2014-09-05 01:04:51 UTC

Permalink

Post by Dave Chinner

That doesn't seem like a big deal. What sort of problems does this
actually cause?
FYI, many linux filesystems only have second resolution timestamps
and hence applications can't rely on sub-second timestamp resolution
to actually mean anything useful....

But why not restore the same resolution as is actually stored in the dump?
Throwing it away seems odd, and restoring it looks easy enough.

In any case, there was a user who noticed & complained. Seems like a
very reasonable thing to fix, to me.

-Eric

Dave Chinner

2014-09-05 01:24:04 UTC

Permalink

Post by Eric Sandeen

Post by Dave Chinner

That doesn't seem like a big deal. What sort of problems does this
actually cause?
FYI, many linux filesystems only have second resolution timestamps
and hence applications can't rely on sub-second timestamp resolution
to actually mean anything useful....

But why not restore the same resolution as is actually stored in the dump?
Throwing it away seems odd, and restoring it looks easy enough.

Comes from a time when we couldn't restore what was in the dump. :/

Post by Eric Sandeen
In any case, there was a user who noticed & complained. Seems like a
very reasonable thing to fix, to me.

Sure, but we don't make changes with the justification "just
because". xfsrestore has had this behaviour since dump/restore was
first introduced, so first we need to understand what the actual
problem is. Was the user complaining because they noticed they were
"different" in passing, or was it noticed because the difference is
the root cause of some other problem?

Cheers,

Dave.

--
Dave Chinner
***@fromorbit.com

Brian Foster

2014-09-05 11:02:12 UTC

Permalink

Post by Dave Chinner

Post by Eric Sandeen

Post by Dave Chinner

That doesn't seem like a big deal. What sort of problems does this
actually cause?
FYI, many linux filesystems only have second resolution timestamps
and hence applications can't rely on sub-second timestamp resolution
to actually mean anything useful....

But why not restore the same resolution as is actually stored in the dump?
Throwing it away seems odd, and restoring it looks easy enough.

Comes from a time when we couldn't restore what was in the dump. :/

Post by Eric Sandeen
In any case, there was a user who noticed & complained. Seems like a
very reasonable thing to fix, to me.

No problems that I'm aware of. As Eric mentioned, it was noticed during
an evaluation of possible data transfer mechanisms for a glusterfs
setup. The user had to evaluate whether it would lead to any issues (a
geo-replication tracking thing I suspect) for a customer, but I hadn't
heard anything that suggested it was. The utime() call appears to be
obsolete as well, for whatever that's worth.

Brian

Post by Dave Chinner
Cheers,
Dave.
--
Dave Chinner

Greg Freemyer

2014-09-05 11:19:29 UTC

Permalink

This post might be inappropriate. Click to display it.