All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] xfsdump: update to use fallocate
@ 2019-02-22 16:47 Darrick J. Wong
  2019-02-22 16:47 ` [PATCH 1/4] xfs_restore: refactor open-coded file creation code Darrick J. Wong
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Darrick J. Wong @ 2019-02-22 16:47 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs

Hi all,

This is a series to update the "create file and preallocate blocks"
code in xfs_restore.

The first patch refactors the existing copy-pasta into a single helper
function to create and preallocate space in files, as well as removing
the totally broken ALLOCSP code.

The next two patches fix unchecked return values and insufficient
unsupported-ioctl code.

The final patch upgrades xfs_restore to try fallocate before RESVSP
simply because it is the newer API.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/4] xfs_restore: refactor open-coded file creation code
  2019-02-22 16:47 [PATCH 0/4] xfsdump: update to use fallocate Darrick J. Wong
@ 2019-02-22 16:47 ` Darrick J. Wong
  2019-02-22 19:20   ` Andre Noll
  2019-05-07  0:11   ` Allison Collins
  2019-02-22 16:47 ` [PATCH 2/4] xfs_restore: check return value Darrick J. Wong
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: Darrick J. Wong @ 2019-02-22 16:47 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a helper to unlink, recreate, and reserve space in a file so that
we don't have two open-coded versions.  We lose the broken ALLOCSP code
since it never worked anyway.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 restore/dirattr.c |   97 ++++++++++++++++++-----------------------------------
 restore/dirattr.h |    2 +
 restore/namreg.c  |   70 +++-----------------------------------
 3 files changed, 41 insertions(+), 128 deletions(-)


diff --git a/restore/dirattr.c b/restore/dirattr.c
index 5368664..0fb2877 100644
--- a/restore/dirattr.c
+++ b/restore/dirattr.c
@@ -55,6 +55,37 @@
 #include "openutil.h"
 #include "mmap.h"
 
+/* Create a file, try to reserve space for it, and return the fd. */
+int
+create_filled_file(
+	const char	*pathname,
+	off64_t		size)
+{
+	struct flock64	fl = {
+		.l_len = size,
+	};
+	int		fd;
+	int		ret;
+
+	(void)unlink(pathname);
+
+	fd = open(pathname, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
+	if (fd < 0)
+		return fd;
+
+	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
+	if (ret && errno != ENOTTY)
+		mlog(MLOG_VERBOSE | MLOG_NOTE,
+_("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
+				size, pathname, "XFS_IOC_RESVSP64",
+				strerror(errno), errno);
+	if (ret == 0)
+		goto done;
+
+done:
+	return fd;
+}
+
 /* structure definitions used locally ****************************************/
 
 /* node handle limits
@@ -238,13 +269,8 @@ dirattr_init(char *hkdir, bool_t resume, uint64_t dircnt)
 			return BOOL_FALSE;
 		}
 	} else {
-		/* create the dirattr file, first unlinking any older version
-		 * laying around
-		 */
-		(void)unlink(dtp->dt_pathname);
-		dtp->dt_fd = open(dtp->dt_pathname,
-				   O_RDWR | O_CREAT | O_EXCL,
-				   S_IRUSR | S_IWUSR);
+		dtp->dt_fd = create_filled_file(dtp->dt_pathname,
+			DIRATTR_PERS_SZ + (dircnt * sizeof(struct dirattr)));
 		if (dtp->dt_fd < 0) {
 			mlog(MLOG_NORMAL | MLOG_ERROR, _(
 			      "could not create directory attributes file %s: "
@@ -253,63 +279,6 @@ dirattr_init(char *hkdir, bool_t resume, uint64_t dircnt)
 			      strerror(errno));
 			return BOOL_FALSE;
 		}
-
-		/* reserve space for the backing store. try to use RESVSP64.
-		 * if doesn't work, try ALLOCSP64. the former is faster, as
-		 * it does not zero the space.
-		 */
-		{
-		bool_t successpr;
-		unsigned int ioctlcmd;
-		int loglevel;
-		size_t trycnt;
-
-		for (trycnt = 0,
-		      successpr = BOOL_FALSE,
-		      ioctlcmd = XFS_IOC_RESVSP64,
-		      loglevel = MLOG_VERBOSE
-		      ;
-		      ! successpr && trycnt < 2
-		      ;
-		      trycnt++,
-		      ioctlcmd = XFS_IOC_ALLOCSP64,
-		      loglevel = max(MLOG_NORMAL, loglevel - 1)) {
-			off64_t initsz;
-			struct flock64 flock64;
-			int rval;
-
-			if (! ioctlcmd) {
-				continue;
-			}
-
-			initsz = (off64_t)DIRATTR_PERS_SZ
-				 +
-				 ((off64_t)dircnt * sizeof(dirattr_t));
-			flock64.l_whence = 0;
-			flock64.l_start = 0;
-			flock64.l_len = initsz;
-			rval = ioctl(dtp->dt_fd, ioctlcmd, &flock64);
-			if (rval) {
-				if (errno != ENOTTY) {
-					mlog(loglevel | MLOG_NOTE, _(
-					      "attempt to reserve %lld bytes for %s "
-					      "using %s "
-					      "failed: %s (%d)\n"),
-					      initsz,
-					      dtp->dt_pathname,
-					      ioctlcmd == XFS_IOC_RESVSP64
-					      ?
-					      "XFS_IOC_RESVSP64"
-					      :
-					      "XFS_IOC_ALLOCSP64",
-					      strerror(errno),
-					      errno);
-				}
-			} else {
-				successpr = BOOL_TRUE;
-			}
-		}
-		}
 	}
 
 	/* mmap the persistent descriptor
diff --git a/restore/dirattr.h b/restore/dirattr.h
index dd37a98..e81e69c 100644
--- a/restore/dirattr.h
+++ b/restore/dirattr.h
@@ -88,4 +88,6 @@ extern bool_t dirattr_cb_extattr(dah_t dah,
 				  extattrhdr_t *ahdrp,
 				  void *ctxp);
 
+int create_filled_file(const char *pathname, off64_t size);
+
 #endif /* DIRATTR_H */
diff --git a/restore/namreg.c b/restore/namreg.c
index 89fa5ef..d0d5e89 100644
--- a/restore/namreg.c
+++ b/restore/namreg.c
@@ -37,6 +37,10 @@
 #include "namreg.h"
 #include "openutil.h"
 #include "mmap.h"
+#include "global.h"
+#include "content.h"
+#include "content_inode.h"
+#include "dirattr.h"
 
 /* structure definitions used locally ****************************************/
 
@@ -153,13 +157,8 @@ namreg_init(char *hkdir, bool_t resume, uint64_t inocnt)
 			return BOOL_FALSE;
 		}
 	} else {
-		/* create the namreg file, first unlinking any older version
-		 * laying around
-		 */
-		(void)unlink(ntp->nt_pathname);
-		ntp->nt_fd = open(ntp->nt_pathname,
-				   O_RDWR | O_CREAT | O_EXCL,
-				   S_IRUSR | S_IWUSR);
+		ntp->nt_fd = create_filled_file(ntp->nt_pathname,
+			NAMREG_PERS_SZ + (inocnt * NAMREG_AVGLEN));
 		if (ntp->nt_fd < 0) {
 			mlog(MLOG_NORMAL | MLOG_ERROR, _(
 			      "could not create name registry file %s: "
@@ -168,63 +167,6 @@ namreg_init(char *hkdir, bool_t resume, uint64_t inocnt)
 			      strerror(errno));
 			return BOOL_FALSE;
 		}
-
-		/* reserve space for the backing store. try to use RESVSP64.
-		 * if doesn't work, try ALLOCSP64. the former is faster, as
-		 * it does not zero the space.
-		 */
-		{
-		bool_t successpr;
-		unsigned int ioctlcmd;
-		int loglevel;
-		size_t trycnt;
-
-		for (trycnt = 0,
-		      successpr = BOOL_FALSE,
-		      ioctlcmd = XFS_IOC_RESVSP64,
-		      loglevel = MLOG_VERBOSE
-		      ;
-		      ! successpr && trycnt < 2
-		      ;
-		      trycnt++,
-		      ioctlcmd = XFS_IOC_ALLOCSP64,
-		      loglevel = max(MLOG_NORMAL, loglevel - 1)) {
-			off64_t initsz;
-			struct flock64 flock64;
-			int rval;
-
-			if (! ioctlcmd) {
-				continue;
-			}
-
-			initsz = (off64_t)NAMREG_PERS_SZ
-				 +
-				 ((off64_t)inocnt * NAMREG_AVGLEN);
-			flock64.l_whence = 0;
-			flock64.l_start = 0;
-			flock64.l_len = initsz;
-			rval = ioctl(ntp->nt_fd, ioctlcmd, &flock64);
-			if (rval) {
-				if (errno != ENOTTY) {
-					mlog(loglevel | MLOG_NOTE, _(
-					      "attempt to reserve %lld bytes for %s "
-					      "using %s "
-					      "failed: %s (%d)\n"),
-					      initsz,
-					      ntp->nt_pathname,
-					      ioctlcmd == XFS_IOC_RESVSP64
-					      ?
-					      "XFS_IOC_RESVSP64"
-					      :
-					      "XFS_IOC_ALLOCSP64",
-					      strerror(errno),
-					      errno);
-				}
-			} else {
-				successpr = BOOL_TRUE;
-			}
-		}
-		}
 	}
 
 	/* mmap the persistent descriptor

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/4] xfs_restore: check return value
  2019-02-22 16:47 [PATCH 0/4] xfsdump: update to use fallocate Darrick J. Wong
  2019-02-22 16:47 ` [PATCH 1/4] xfs_restore: refactor open-coded file creation code Darrick J. Wong
@ 2019-02-22 16:47 ` Darrick J. Wong
  2019-05-07  0:11   ` Allison Collins
  2019-02-22 16:47 ` [PATCH 3/4] xfs_restore: fix unsupported ioctl detection Darrick J. Wong
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2019-02-22 16:47 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Check the return value of the unlink call when creating a new file.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 restore/dirattr.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


diff --git a/restore/dirattr.c b/restore/dirattr.c
index 0fb2877..4257a1b 100644
--- a/restore/dirattr.c
+++ b/restore/dirattr.c
@@ -67,7 +67,9 @@ create_filled_file(
 	int		fd;
 	int		ret;
 
-	(void)unlink(pathname);
+	ret = unlink(pathname);
+	if (ret && errno != ENOENT)
+		return ret;
 
 	fd = open(pathname, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
 	if (fd < 0)

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/4] xfs_restore: fix unsupported ioctl detection
  2019-02-22 16:47 [PATCH 0/4] xfsdump: update to use fallocate Darrick J. Wong
  2019-02-22 16:47 ` [PATCH 1/4] xfs_restore: refactor open-coded file creation code Darrick J. Wong
  2019-02-22 16:47 ` [PATCH 2/4] xfs_restore: check return value Darrick J. Wong
@ 2019-02-22 16:47 ` Darrick J. Wong
  2019-05-07  0:11   ` Allison Collins
  2019-02-22 16:47 ` [PATCH 4/4] xfs_restore: support fallocate when reserving space for a file Darrick J. Wong
  2019-05-06 18:24 ` [PATCH 0/4] xfsdump: update to use fallocate Darrick J. Wong
  4 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2019-02-22 16:47 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Linux ioctls can return ENOTTY or EOPNOTSUPP, so filter both of them
when logging reservation failure.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 restore/dirattr.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/restore/dirattr.c b/restore/dirattr.c
index 4257a1b..3fa8fb6 100644
--- a/restore/dirattr.c
+++ b/restore/dirattr.c
@@ -76,7 +76,7 @@ create_filled_file(
 		return fd;
 
 	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
-	if (ret && errno != ENOTTY)
+	if (ret && (errno != EOPNOTSUPP && errno != ENOTTY))
 		mlog(MLOG_VERBOSE | MLOG_NOTE,
 _("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
 				size, pathname, "XFS_IOC_RESVSP64",

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/4] xfs_restore: support fallocate when reserving space for a file
  2019-02-22 16:47 [PATCH 0/4] xfsdump: update to use fallocate Darrick J. Wong
                   ` (2 preceding siblings ...)
  2019-02-22 16:47 ` [PATCH 3/4] xfs_restore: fix unsupported ioctl detection Darrick J. Wong
@ 2019-02-22 16:47 ` Darrick J. Wong
  2019-05-07  0:11   ` Allison Collins
  2019-05-06 18:24 ` [PATCH 0/4] xfsdump: update to use fallocate Darrick J. Wong
  4 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2019-02-22 16:47 UTC (permalink / raw)
  To: darrick.wong; +Cc: linux-xfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Update the file creation helper to try fallocate when restoring a
filesystem before it tries RESVSP.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 configure.ac          |    2 ++
 include/builddefs.in  |    1 +
 m4/Makefile           |    1 +
 m4/package_libcdev.m4 |   15 +++++++++++++++
 restore/Makefile      |    4 ++++
 restore/dirattr.c     |   11 +++++++++++
 6 files changed, 34 insertions(+)
 create mode 100644 m4/package_libcdev.m4


diff --git a/configure.ac b/configure.ac
index a77054c..73dedd7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -84,6 +84,8 @@ AC_PACKAGE_NEED_ATTRIBUTES_H
 AC_PACKAGE_NEED_ATTRIBUTES_MACROS
 AC_PACKAGE_NEED_ATTRGET_LIBATTR
 
+AC_HAVE_FALLOCATE
+
 AC_MANUAL_FORMAT
 
 AC_CONFIG_FILES([include/builddefs])
diff --git a/include/builddefs.in b/include/builddefs.in
index 269c928..1c7e12f 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_FALLOCATE = @have_fallocate@
 
 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..050f82c
--- /dev/null
+++ b/m4/package_libcdev.m4
@@ -0,0 +1,15 @@
+#
+# Check if we have a fallocate libc call (Linux)
+#
+AC_DEFUN([AC_HAVE_FALLOCATE],
+  [ AC_MSG_CHECKING([for fallocate])
+    AC_TRY_LINK([
+#include <fcntl.h>
+#include <linux/falloc.h>
+    ], [
+         fallocate(0, 0, 0, 0);
+    ], have_fallocate=yes
+       AC_MSG_RESULT(yes),
+       AC_MSG_RESULT(no))
+    AC_SUBST(have_fallocate)
+  ])
diff --git a/restore/Makefile b/restore/Makefile
index 20c870a..ac3f8c8 100644
--- a/restore/Makefile
+++ b/restore/Makefile
@@ -102,6 +102,10 @@ LTDEPENDENCIES = $(LIBRMT)
 
 LCFLAGS = -DRESTORE
 
+ifeq ($(HAVE_FALLOCATE),yes)
+LCFLAGS += -DHAVE_FALLOCATE
+endif
+
 default: depend $(LTCOMMAND)
 
 include $(BUILDRULES)
diff --git a/restore/dirattr.c b/restore/dirattr.c
index 3fa8fb6..82eb1de 100644
--- a/restore/dirattr.c
+++ b/restore/dirattr.c
@@ -75,6 +75,17 @@ create_filled_file(
 	if (fd < 0)
 		return fd;
 
+#ifdef HAVE_FALLOCATE
+	ret = fallocate(fd, 0, 0, size);
+	if (ret && (errno != EOPNOTSUPP && errno != ENOTTY))
+		mlog(MLOG_VERBOSE | MLOG_NOTE,
+_("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
+				size, pathname, "fallocate",
+				strerror(errno), errno);
+	if (ret == 0)
+		goto done;
+#endif
+
 	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
 	if (ret && (errno != EOPNOTSUPP && errno != ENOTTY))
 		mlog(MLOG_VERBOSE | MLOG_NOTE,

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code
  2019-02-22 16:47 ` [PATCH 1/4] xfs_restore: refactor open-coded file creation code Darrick J. Wong
@ 2019-02-22 19:20   ` Andre Noll
  2019-02-22 19:28     ` Darrick J. Wong
  2019-05-07  0:11   ` Allison Collins
  1 sibling, 1 reply; 15+ messages in thread
From: Andre Noll @ 2019-02-22 19:20 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Fri, Feb 22, 08:47, Darrick J. Wong wrote
> +	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
> +	if (ret && errno != ENOTTY)
> +		mlog(MLOG_VERBOSE | MLOG_NOTE,
> +_("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
> +				size, pathname, "XFS_IOC_RESVSP64",
> +				strerror(errno), errno);

The "XFS_IOC_RESVSP64" string literal can be included in the format string.

> +	if (ret == 0)
> +		goto done;
> +
> +done:

LOL

> +	return fd;

If you really want to return success even if the ioctl() failed, this might
deserve a comment.

Andre
-- 
Max Planck Institute for Developmental Biology
Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code
  2019-02-22 19:20   ` Andre Noll
@ 2019-02-22 19:28     ` Darrick J. Wong
  2019-02-22 19:55       ` Andre Noll
  0 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2019-02-22 19:28 UTC (permalink / raw)
  To: Andre Noll; +Cc: linux-xfs

On Fri, Feb 22, 2019 at 08:20:53PM +0100, Andre Noll wrote:
> On Fri, Feb 22, 08:47, Darrick J. Wong wrote
> > +	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
> > +	if (ret && errno != ENOTTY)
> > +		mlog(MLOG_VERBOSE | MLOG_NOTE,
> > +_("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
> > +				size, pathname, "XFS_IOC_RESVSP64",
> > +				strerror(errno), errno);
> 
> The "XFS_IOC_RESVSP64" string literal can be included in the format string.

I was trying not to force an update of the i18n message catalogs.

> > +	if (ret == 0)
> > +		goto done;
> > +
> > +done:
> 
> LOL

Yes, it's silly, but gcc will complain if the label doesn't get
used, which it won't if you apply the rest of the series and build
xfsdump on a system that (somehow) doesn't know about fallocate.

> > +	return fd;
> 
> If you really want to return success even if the ioctl() failed, this might
> deserve a comment.

That's what the old code did, hence "try" in "try to reserve space" in
the comment describing what this function does.

--D

> 
> Andre
> -- 
> Max Planck Institute for Developmental Biology
> Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
> http://people.tuebingen.mpg.de/maan/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code
  2019-02-22 19:28     ` Darrick J. Wong
@ 2019-02-22 19:55       ` Andre Noll
  0 siblings, 0 replies; 15+ messages in thread
From: Andre Noll @ 2019-02-22 19:55 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On Fri, Feb 22, 11:28, Darrick J. Wong wrote
> > > +_("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
> > > +				size, pathname, "XFS_IOC_RESVSP64",
> > > +				strerror(errno), errno);
> > 
> > The "XFS_IOC_RESVSP64" string literal can be included in the format string.
> 
> I was trying not to force an update of the i18n message catalogs.

Point.

> > > +	if (ret == 0)
> > > +		goto done;
> > > +
> > > +done:
> > 
> > LOL
> 
> Yes, it's silly, but gcc will complain if the label doesn't get
> used, which it won't if you apply the rest of the series and build
> xfsdump on a system that (somehow) doesn't know about fallocate.

True. It does trigger a WTF event in wetware though, at least if you
read the patch without context of 4/4.

Anyway, thanks for the explanations
Andre
-- 
Max Planck Institute for Developmental Biology
Max-Planck-Ring 5, 72076 Tübingen, Germany. Phone: (+49) 7071 601 829
http://people.tuebingen.mpg.de/maan/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] xfsdump: update to use fallocate
  2019-02-22 16:47 [PATCH 0/4] xfsdump: update to use fallocate Darrick J. Wong
                   ` (3 preceding siblings ...)
  2019-02-22 16:47 ` [PATCH 4/4] xfs_restore: support fallocate when reserving space for a file Darrick J. Wong
@ 2019-05-06 18:24 ` Darrick J. Wong
  4 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2019-05-06 18:24 UTC (permalink / raw)
  To: linux-xfs; +Cc: Eric Sandeen

Uh, ping? :)

--D

On Fri, Feb 22, 2019 at 08:47:18AM -0800, Darrick J. Wong wrote:
> Hi all,
> 
> This is a series to update the "create file and preallocate blocks"
> code in xfs_restore.
> 
> The first patch refactors the existing copy-pasta into a single helper
> function to create and preallocate space in files, as well as removing
> the totally broken ALLOCSP code.
> 
> The next two patches fix unchecked return values and insufficient
> unsupported-ioctl code.
> 
> The final patch upgrades xfs_restore to try fallocate before RESVSP
> simply because it is the newer API.
> 
> If you're going to start using this mess, you probably ought to just
> pull from my git trees, which are linked below.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code
  2019-02-22 16:47 ` [PATCH 1/4] xfs_restore: refactor open-coded file creation code Darrick J. Wong
  2019-02-22 19:20   ` Andre Noll
@ 2019-05-07  0:11   ` Allison Collins
  2019-05-20 21:05     ` Darrick J. Wong
  1 sibling, 1 reply; 15+ messages in thread
From: Allison Collins @ 2019-05-07  0:11 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

On 2/22/19 9:47 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a helper to unlink, recreate, and reserve space in a file so that
> we don't have two open-coded versions.  We lose the broken ALLOCSP code
> since it never worked anyway.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   restore/dirattr.c |   97 ++++++++++++++++++-----------------------------------
>   restore/dirattr.h |    2 +
>   restore/namreg.c  |   70 +++-----------------------------------
>   3 files changed, 41 insertions(+), 128 deletions(-)
> 
> 
> diff --git a/restore/dirattr.c b/restore/dirattr.c
> index 5368664..0fb2877 100644
> --- a/restore/dirattr.c
> +++ b/restore/dirattr.c
> @@ -55,6 +55,37 @@
>   #include "openutil.h"
>   #include "mmap.h"
>   
> +/* Create a file, try to reserve space for it, and return the fd. */
> +int
> +create_filled_file(
> +	const char	*pathname,
> +	off64_t		size)
> +{
> +	struct flock64	fl = {
> +		.l_len = size,
> +	};
> +	int		fd;
> +	int		ret;
> +
> +	(void)unlink(pathname);
> +
> +	fd = open(pathname, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
> +	if (fd < 0)
> +		return fd;

Just a nit: I think if you goto done here instead of return, you can 
remove the extra goto below since it's not having much effect.  I sort 
of figured people like gotos because they like having one exit point to 
the function.  Alternatively, if you don't mind having multiple exit 
points, you can simply return early in patch 4, and avoid the goto all 
together.

Allison

> +
> +	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
> +	if (ret && errno != ENOTTY)
> +		mlog(MLOG_VERBOSE | MLOG_NOTE,
> +_("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
> +				size, pathname, "XFS_IOC_RESVSP64",
> +				strerror(errno), errno);
> +	if (ret == 0)
> +		goto done;
> +
> +done:
> +	return fd;
> +}
> +
>   /* structure definitions used locally ****************************************/
>   
>   /* node handle limits
> @@ -238,13 +269,8 @@ dirattr_init(char *hkdir, bool_t resume, uint64_t dircnt)
>   			return BOOL_FALSE;
>   		}
>   	} else {
> -		/* create the dirattr file, first unlinking any older version
> -		 * laying around
> -		 */
> -		(void)unlink(dtp->dt_pathname);
> -		dtp->dt_fd = open(dtp->dt_pathname,
> -				   O_RDWR | O_CREAT | O_EXCL,
> -				   S_IRUSR | S_IWUSR);
> +		dtp->dt_fd = create_filled_file(dtp->dt_pathname,
> +			DIRATTR_PERS_SZ + (dircnt * sizeof(struct dirattr)));
>   		if (dtp->dt_fd < 0) {
>   			mlog(MLOG_NORMAL | MLOG_ERROR, _(
>   			      "could not create directory attributes file %s: "
> @@ -253,63 +279,6 @@ dirattr_init(char *hkdir, bool_t resume, uint64_t dircnt)
>   			      strerror(errno));
>   			return BOOL_FALSE;
>   		}
> -
> -		/* reserve space for the backing store. try to use RESVSP64.
> -		 * if doesn't work, try ALLOCSP64. the former is faster, as
> -		 * it does not zero the space.
> -		 */
> -		{
> -		bool_t successpr;
> -		unsigned int ioctlcmd;
> -		int loglevel;
> -		size_t trycnt;
> -
> -		for (trycnt = 0,
> -		      successpr = BOOL_FALSE,
> -		      ioctlcmd = XFS_IOC_RESVSP64,
> -		      loglevel = MLOG_VERBOSE
> -		      ;
> -		      ! successpr && trycnt < 2
> -		      ;
> -		      trycnt++,
> -		      ioctlcmd = XFS_IOC_ALLOCSP64,
> -		      loglevel = max(MLOG_NORMAL, loglevel - 1)) {
> -			off64_t initsz;
> -			struct flock64 flock64;
> -			int rval;
> -
> -			if (! ioctlcmd) {
> -				continue;
> -			}
> -
> -			initsz = (off64_t)DIRATTR_PERS_SZ
> -				 +
> -				 ((off64_t)dircnt * sizeof(dirattr_t));
> -			flock64.l_whence = 0;
> -			flock64.l_start = 0;
> -			flock64.l_len = initsz;
> -			rval = ioctl(dtp->dt_fd, ioctlcmd, &flock64);
> -			if (rval) {
> -				if (errno != ENOTTY) {
> -					mlog(loglevel | MLOG_NOTE, _(
> -					      "attempt to reserve %lld bytes for %s "
> -					      "using %s "
> -					      "failed: %s (%d)\n"),
> -					      initsz,
> -					      dtp->dt_pathname,
> -					      ioctlcmd == XFS_IOC_RESVSP64
> -					      ?
> -					      "XFS_IOC_RESVSP64"
> -					      :
> -					      "XFS_IOC_ALLOCSP64",
> -					      strerror(errno),
> -					      errno);
> -				}
> -			} else {
> -				successpr = BOOL_TRUE;
> -			}
> -		}
> -		}
>   	}
>   
>   	/* mmap the persistent descriptor
> diff --git a/restore/dirattr.h b/restore/dirattr.h
> index dd37a98..e81e69c 100644
> --- a/restore/dirattr.h
> +++ b/restore/dirattr.h
> @@ -88,4 +88,6 @@ extern bool_t dirattr_cb_extattr(dah_t dah,
>   				  extattrhdr_t *ahdrp,
>   				  void *ctxp);
>   
> +int create_filled_file(const char *pathname, off64_t size);
> +
>   #endif /* DIRATTR_H */
> diff --git a/restore/namreg.c b/restore/namreg.c
> index 89fa5ef..d0d5e89 100644
> --- a/restore/namreg.c
> +++ b/restore/namreg.c
> @@ -37,6 +37,10 @@
>   #include "namreg.h"
>   #include "openutil.h"
>   #include "mmap.h"
> +#include "global.h"
> +#include "content.h"
> +#include "content_inode.h"
> +#include "dirattr.h"
>   
>   /* structure definitions used locally ****************************************/
>   
> @@ -153,13 +157,8 @@ namreg_init(char *hkdir, bool_t resume, uint64_t inocnt)
>   			return BOOL_FALSE;
>   		}
>   	} else {
> -		/* create the namreg file, first unlinking any older version
> -		 * laying around
> -		 */
> -		(void)unlink(ntp->nt_pathname);
> -		ntp->nt_fd = open(ntp->nt_pathname,
> -				   O_RDWR | O_CREAT | O_EXCL,
> -				   S_IRUSR | S_IWUSR);
> +		ntp->nt_fd = create_filled_file(ntp->nt_pathname,
> +			NAMREG_PERS_SZ + (inocnt * NAMREG_AVGLEN));
>   		if (ntp->nt_fd < 0) {
>   			mlog(MLOG_NORMAL | MLOG_ERROR, _(
>   			      "could not create name registry file %s: "
> @@ -168,63 +167,6 @@ namreg_init(char *hkdir, bool_t resume, uint64_t inocnt)
>   			      strerror(errno));
>   			return BOOL_FALSE;
>   		}
> -
> -		/* reserve space for the backing store. try to use RESVSP64.
> -		 * if doesn't work, try ALLOCSP64. the former is faster, as
> -		 * it does not zero the space.
> -		 */
> -		{
> -		bool_t successpr;
> -		unsigned int ioctlcmd;
> -		int loglevel;
> -		size_t trycnt;
> -
> -		for (trycnt = 0,
> -		      successpr = BOOL_FALSE,
> -		      ioctlcmd = XFS_IOC_RESVSP64,
> -		      loglevel = MLOG_VERBOSE
> -		      ;
> -		      ! successpr && trycnt < 2
> -		      ;
> -		      trycnt++,
> -		      ioctlcmd = XFS_IOC_ALLOCSP64,
> -		      loglevel = max(MLOG_NORMAL, loglevel - 1)) {
> -			off64_t initsz;
> -			struct flock64 flock64;
> -			int rval;
> -
> -			if (! ioctlcmd) {
> -				continue;
> -			}
> -
> -			initsz = (off64_t)NAMREG_PERS_SZ
> -				 +
> -				 ((off64_t)inocnt * NAMREG_AVGLEN);
> -			flock64.l_whence = 0;
> -			flock64.l_start = 0;
> -			flock64.l_len = initsz;
> -			rval = ioctl(ntp->nt_fd, ioctlcmd, &flock64);
> -			if (rval) {
> -				if (errno != ENOTTY) {
> -					mlog(loglevel | MLOG_NOTE, _(
> -					      "attempt to reserve %lld bytes for %s "
> -					      "using %s "
> -					      "failed: %s (%d)\n"),
> -					      initsz,
> -					      ntp->nt_pathname,
> -					      ioctlcmd == XFS_IOC_RESVSP64
> -					      ?
> -					      "XFS_IOC_RESVSP64"
> -					      :
> -					      "XFS_IOC_ALLOCSP64",
> -					      strerror(errno),
> -					      errno);
> -				}
> -			} else {
> -				successpr = BOOL_TRUE;
> -			}
> -		}
> -		}
>   	}
>   
>   	/* mmap the persistent descriptor
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] xfs_restore: check return value
  2019-02-22 16:47 ` [PATCH 2/4] xfs_restore: check return value Darrick J. Wong
@ 2019-05-07  0:11   ` Allison Collins
  0 siblings, 0 replies; 15+ messages in thread
From: Allison Collins @ 2019-05-07  0:11 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

Looks ok.
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

On 2/22/19 9:47 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Check the return value of the unlink call when creating a new file.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   restore/dirattr.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/restore/dirattr.c b/restore/dirattr.c
> index 0fb2877..4257a1b 100644
> --- a/restore/dirattr.c
> +++ b/restore/dirattr.c
> @@ -67,7 +67,9 @@ create_filled_file(
>   	int		fd;
>   	int		ret;
>   
> -	(void)unlink(pathname);
> +	ret = unlink(pathname);
> +	if (ret && errno != ENOENT)
> +		return ret;
>   
>   	fd = open(pathname, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
>   	if (fd < 0)
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] xfs_restore: fix unsupported ioctl detection
  2019-02-22 16:47 ` [PATCH 3/4] xfs_restore: fix unsupported ioctl detection Darrick J. Wong
@ 2019-05-07  0:11   ` Allison Collins
  0 siblings, 0 replies; 15+ messages in thread
From: Allison Collins @ 2019-05-07  0:11 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

Looks ok to me
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

On 2/22/19 9:47 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Linux ioctls can return ENOTTY or EOPNOTSUPP, so filter both of them
> when logging reservation failure.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   restore/dirattr.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/restore/dirattr.c b/restore/dirattr.c
> index 4257a1b..3fa8fb6 100644
> --- a/restore/dirattr.c
> +++ b/restore/dirattr.c
> @@ -76,7 +76,7 @@ create_filled_file(
>   		return fd;
>   
>   	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
> -	if (ret && errno != ENOTTY)
> +	if (ret && (errno != EOPNOTSUPP && errno != ENOTTY))
>   		mlog(MLOG_VERBOSE | MLOG_NOTE,
>   _("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
>   				size, pathname, "XFS_IOC_RESVSP64",
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] xfs_restore: support fallocate when reserving space for a file
  2019-02-22 16:47 ` [PATCH 4/4] xfs_restore: support fallocate when reserving space for a file Darrick J. Wong
@ 2019-05-07  0:11   ` Allison Collins
  0 siblings, 0 replies; 15+ messages in thread
From: Allison Collins @ 2019-05-07  0:11 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-xfs

On 2/22/19 9:47 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Update the file creation helper to try fallocate when restoring a
> filesystem before it tries RESVSP.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   configure.ac          |    2 ++
>   include/builddefs.in  |    1 +
>   m4/Makefile           |    1 +
>   m4/package_libcdev.m4 |   15 +++++++++++++++
>   restore/Makefile      |    4 ++++
>   restore/dirattr.c     |   11 +++++++++++
>   6 files changed, 34 insertions(+)
>   create mode 100644 m4/package_libcdev.m4
> 
> 
> diff --git a/configure.ac b/configure.ac
> index a77054c..73dedd7 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -84,6 +84,8 @@ AC_PACKAGE_NEED_ATTRIBUTES_H
>   AC_PACKAGE_NEED_ATTRIBUTES_MACROS
>   AC_PACKAGE_NEED_ATTRGET_LIBATTR
>   
> +AC_HAVE_FALLOCATE
> +
>   AC_MANUAL_FORMAT
>   
>   AC_CONFIG_FILES([include/builddefs])
> diff --git a/include/builddefs.in b/include/builddefs.in
> index 269c928..1c7e12f 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_FALLOCATE = @have_fallocate@
>   
>   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..050f82c
> --- /dev/null
> +++ b/m4/package_libcdev.m4
> @@ -0,0 +1,15 @@
> +#
> +# Check if we have a fallocate libc call (Linux)
> +#
> +AC_DEFUN([AC_HAVE_FALLOCATE],
> +  [ AC_MSG_CHECKING([for fallocate])
> +    AC_TRY_LINK([
> +#include <fcntl.h>
> +#include <linux/falloc.h>
> +    ], [
> +         fallocate(0, 0, 0, 0);
> +    ], have_fallocate=yes
> +       AC_MSG_RESULT(yes),
> +       AC_MSG_RESULT(no))
> +    AC_SUBST(have_fallocate)
> +  ])
> diff --git a/restore/Makefile b/restore/Makefile
> index 20c870a..ac3f8c8 100644
> --- a/restore/Makefile
> +++ b/restore/Makefile
> @@ -102,6 +102,10 @@ LTDEPENDENCIES = $(LIBRMT)
>   
>   LCFLAGS = -DRESTORE
>   
> +ifeq ($(HAVE_FALLOCATE),yes)
> +LCFLAGS += -DHAVE_FALLOCATE
> +endif
> +
>   default: depend $(LTCOMMAND)
>   
>   include $(BUILDRULES)
> diff --git a/restore/dirattr.c b/restore/dirattr.c
> index 3fa8fb6..82eb1de 100644
> --- a/restore/dirattr.c
> +++ b/restore/dirattr.c
> @@ -75,6 +75,17 @@ create_filled_file(
>   	if (fd < 0)
>   		return fd;
>   
> +#ifdef HAVE_FALLOCATE
> +	ret = fallocate(fd, 0, 0, size);
> +	if (ret && (errno != EOPNOTSUPP && errno != ENOTTY))
> +		mlog(MLOG_VERBOSE | MLOG_NOTE,
> +_("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
> +				size, pathname, "fallocate",
> +				strerror(errno), errno);
> +	if (ret == 0)
> +		goto done;
This is the goto I referenced in patch 1.  Otherwise I think the rest is ok.

Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> +#endif
> +
>   	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
>   	if (ret && (errno != EOPNOTSUPP && errno != ENOTTY))
>   		mlog(MLOG_VERBOSE | MLOG_NOTE,
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code
  2019-05-07  0:11   ` Allison Collins
@ 2019-05-20 21:05     ` Darrick J. Wong
  0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2019-05-20 21:05 UTC (permalink / raw)
  To: Allison Collins; +Cc: linux-xfs

On Mon, May 06, 2019 at 05:11:23PM -0700, Allison Collins wrote:
> On 2/22/19 9:47 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Create a helper to unlink, recreate, and reserve space in a file so that
> > we don't have two open-coded versions.  We lose the broken ALLOCSP code
> > since it never worked anyway.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >   restore/dirattr.c |   97 ++++++++++++++++++-----------------------------------
> >   restore/dirattr.h |    2 +
> >   restore/namreg.c  |   70 +++-----------------------------------
> >   3 files changed, 41 insertions(+), 128 deletions(-)
> > 
> > 
> > diff --git a/restore/dirattr.c b/restore/dirattr.c
> > index 5368664..0fb2877 100644
> > --- a/restore/dirattr.c
> > +++ b/restore/dirattr.c
> > @@ -55,6 +55,37 @@
> >   #include "openutil.h"
> >   #include "mmap.h"
> > +/* Create a file, try to reserve space for it, and return the fd. */
> > +int
> > +create_filled_file(
> > +	const char	*pathname,
> > +	off64_t		size)
> > +{
> > +	struct flock64	fl = {
> > +		.l_len = size,
> > +	};
> > +	int		fd;
> > +	int		ret;
> > +
> > +	(void)unlink(pathname);
> > +
> > +	fd = open(pathname, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
> > +	if (fd < 0)
> > +		return fd;
> 
> Just a nit: I think if you goto done here instead of return, you can remove
> the extra goto below since it's not having much effect.  I sort of figured
> people like gotos because they like having one exit point to the function.
> Alternatively, if you don't mind having multiple exit points, you can simply
> return early in patch 4, and avoid the goto all together.

Hmm, you're right, let's just return fd directly instead of all this
goto nonsense.

--D

> Allison
> 
> > +
> > +	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
> > +	if (ret && errno != ENOTTY)
> > +		mlog(MLOG_VERBOSE | MLOG_NOTE,
> > +_("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
> > +				size, pathname, "XFS_IOC_RESVSP64",
> > +				strerror(errno), errno);
> > +	if (ret == 0)
> > +		goto done;
> > +
> > +done:
> > +	return fd;
> > +}
> > +
> >   /* structure definitions used locally ****************************************/
> >   /* node handle limits
> > @@ -238,13 +269,8 @@ dirattr_init(char *hkdir, bool_t resume, uint64_t dircnt)
> >   			return BOOL_FALSE;
> >   		}
> >   	} else {
> > -		/* create the dirattr file, first unlinking any older version
> > -		 * laying around
> > -		 */
> > -		(void)unlink(dtp->dt_pathname);
> > -		dtp->dt_fd = open(dtp->dt_pathname,
> > -				   O_RDWR | O_CREAT | O_EXCL,
> > -				   S_IRUSR | S_IWUSR);
> > +		dtp->dt_fd = create_filled_file(dtp->dt_pathname,
> > +			DIRATTR_PERS_SZ + (dircnt * sizeof(struct dirattr)));
> >   		if (dtp->dt_fd < 0) {
> >   			mlog(MLOG_NORMAL | MLOG_ERROR, _(
> >   			      "could not create directory attributes file %s: "
> > @@ -253,63 +279,6 @@ dirattr_init(char *hkdir, bool_t resume, uint64_t dircnt)
> >   			      strerror(errno));
> >   			return BOOL_FALSE;
> >   		}
> > -
> > -		/* reserve space for the backing store. try to use RESVSP64.
> > -		 * if doesn't work, try ALLOCSP64. the former is faster, as
> > -		 * it does not zero the space.
> > -		 */
> > -		{
> > -		bool_t successpr;
> > -		unsigned int ioctlcmd;
> > -		int loglevel;
> > -		size_t trycnt;
> > -
> > -		for (trycnt = 0,
> > -		      successpr = BOOL_FALSE,
> > -		      ioctlcmd = XFS_IOC_RESVSP64,
> > -		      loglevel = MLOG_VERBOSE
> > -		      ;
> > -		      ! successpr && trycnt < 2
> > -		      ;
> > -		      trycnt++,
> > -		      ioctlcmd = XFS_IOC_ALLOCSP64,
> > -		      loglevel = max(MLOG_NORMAL, loglevel - 1)) {
> > -			off64_t initsz;
> > -			struct flock64 flock64;
> > -			int rval;
> > -
> > -			if (! ioctlcmd) {
> > -				continue;
> > -			}
> > -
> > -			initsz = (off64_t)DIRATTR_PERS_SZ
> > -				 +
> > -				 ((off64_t)dircnt * sizeof(dirattr_t));
> > -			flock64.l_whence = 0;
> > -			flock64.l_start = 0;
> > -			flock64.l_len = initsz;
> > -			rval = ioctl(dtp->dt_fd, ioctlcmd, &flock64);
> > -			if (rval) {
> > -				if (errno != ENOTTY) {
> > -					mlog(loglevel | MLOG_NOTE, _(
> > -					      "attempt to reserve %lld bytes for %s "
> > -					      "using %s "
> > -					      "failed: %s (%d)\n"),
> > -					      initsz,
> > -					      dtp->dt_pathname,
> > -					      ioctlcmd == XFS_IOC_RESVSP64
> > -					      ?
> > -					      "XFS_IOC_RESVSP64"
> > -					      :
> > -					      "XFS_IOC_ALLOCSP64",
> > -					      strerror(errno),
> > -					      errno);
> > -				}
> > -			} else {
> > -				successpr = BOOL_TRUE;
> > -			}
> > -		}
> > -		}
> >   	}
> >   	/* mmap the persistent descriptor
> > diff --git a/restore/dirattr.h b/restore/dirattr.h
> > index dd37a98..e81e69c 100644
> > --- a/restore/dirattr.h
> > +++ b/restore/dirattr.h
> > @@ -88,4 +88,6 @@ extern bool_t dirattr_cb_extattr(dah_t dah,
> >   				  extattrhdr_t *ahdrp,
> >   				  void *ctxp);
> > +int create_filled_file(const char *pathname, off64_t size);
> > +
> >   #endif /* DIRATTR_H */
> > diff --git a/restore/namreg.c b/restore/namreg.c
> > index 89fa5ef..d0d5e89 100644
> > --- a/restore/namreg.c
> > +++ b/restore/namreg.c
> > @@ -37,6 +37,10 @@
> >   #include "namreg.h"
> >   #include "openutil.h"
> >   #include "mmap.h"
> > +#include "global.h"
> > +#include "content.h"
> > +#include "content_inode.h"
> > +#include "dirattr.h"
> >   /* structure definitions used locally ****************************************/
> > @@ -153,13 +157,8 @@ namreg_init(char *hkdir, bool_t resume, uint64_t inocnt)
> >   			return BOOL_FALSE;
> >   		}
> >   	} else {
> > -		/* create the namreg file, first unlinking any older version
> > -		 * laying around
> > -		 */
> > -		(void)unlink(ntp->nt_pathname);
> > -		ntp->nt_fd = open(ntp->nt_pathname,
> > -				   O_RDWR | O_CREAT | O_EXCL,
> > -				   S_IRUSR | S_IWUSR);
> > +		ntp->nt_fd = create_filled_file(ntp->nt_pathname,
> > +			NAMREG_PERS_SZ + (inocnt * NAMREG_AVGLEN));
> >   		if (ntp->nt_fd < 0) {
> >   			mlog(MLOG_NORMAL | MLOG_ERROR, _(
> >   			      "could not create name registry file %s: "
> > @@ -168,63 +167,6 @@ namreg_init(char *hkdir, bool_t resume, uint64_t inocnt)
> >   			      strerror(errno));
> >   			return BOOL_FALSE;
> >   		}
> > -
> > -		/* reserve space for the backing store. try to use RESVSP64.
> > -		 * if doesn't work, try ALLOCSP64. the former is faster, as
> > -		 * it does not zero the space.
> > -		 */
> > -		{
> > -		bool_t successpr;
> > -		unsigned int ioctlcmd;
> > -		int loglevel;
> > -		size_t trycnt;
> > -
> > -		for (trycnt = 0,
> > -		      successpr = BOOL_FALSE,
> > -		      ioctlcmd = XFS_IOC_RESVSP64,
> > -		      loglevel = MLOG_VERBOSE
> > -		      ;
> > -		      ! successpr && trycnt < 2
> > -		      ;
> > -		      trycnt++,
> > -		      ioctlcmd = XFS_IOC_ALLOCSP64,
> > -		      loglevel = max(MLOG_NORMAL, loglevel - 1)) {
> > -			off64_t initsz;
> > -			struct flock64 flock64;
> > -			int rval;
> > -
> > -			if (! ioctlcmd) {
> > -				continue;
> > -			}
> > -
> > -			initsz = (off64_t)NAMREG_PERS_SZ
> > -				 +
> > -				 ((off64_t)inocnt * NAMREG_AVGLEN);
> > -			flock64.l_whence = 0;
> > -			flock64.l_start = 0;
> > -			flock64.l_len = initsz;
> > -			rval = ioctl(ntp->nt_fd, ioctlcmd, &flock64);
> > -			if (rval) {
> > -				if (errno != ENOTTY) {
> > -					mlog(loglevel | MLOG_NOTE, _(
> > -					      "attempt to reserve %lld bytes for %s "
> > -					      "using %s "
> > -					      "failed: %s (%d)\n"),
> > -					      initsz,
> > -					      ntp->nt_pathname,
> > -					      ioctlcmd == XFS_IOC_RESVSP64
> > -					      ?
> > -					      "XFS_IOC_RESVSP64"
> > -					      :
> > -					      "XFS_IOC_ALLOCSP64",
> > -					      strerror(errno),
> > -					      errno);
> > -				}
> > -			} else {
> > -				successpr = BOOL_TRUE;
> > -			}
> > -		}
> > -		}
> >   	}
> >   	/* mmap the persistent descriptor
> > 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/4] xfs_restore: refactor open-coded file creation code
  2019-08-20 20:21 Darrick J. Wong
@ 2019-08-20 20:21 ` Darrick J. Wong
  0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2019-08-20 20:21 UTC (permalink / raw)
  To: sandeen, darrick.wong; +Cc: linux-xfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a helper to unlink, recreate, and reserve space in a file so that
we don't have two open-coded versions.  We lose the broken ALLOCSP code
since it never worked anyway.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 restore/dirattr.c |   97 ++++++++++++++++++-----------------------------------
 restore/dirattr.h |    2 +
 restore/namreg.c  |   70 +++-----------------------------------
 3 files changed, 41 insertions(+), 128 deletions(-)


diff --git a/restore/dirattr.c b/restore/dirattr.c
index 806f282..5cd22a8 100644
--- a/restore/dirattr.c
+++ b/restore/dirattr.c
@@ -55,6 +55,37 @@
 #include "openutil.h"
 #include "mmap.h"
 
+/* Create a file, try to reserve space for it, and return the fd. */
+int
+create_filled_file(
+	const char	*pathname,
+	off64_t		size)
+{
+	struct flock64	fl = {
+		.l_len = size,
+	};
+	int		fd;
+	int		ret;
+
+	(void)unlink(pathname);
+
+	fd = open(pathname, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
+	if (fd < 0)
+		return fd;
+
+	ret = ioctl(fd, XFS_IOC_RESVSP64, &fl);
+	if (ret && errno != ENOTTY)
+		mlog(MLOG_VERBOSE | MLOG_NOTE,
+_("attempt to reserve %lld bytes for %s using %s failed: %s (%d)\n"),
+				size, pathname, "XFS_IOC_RESVSP64",
+				strerror(errno), errno);
+	if (ret == 0)
+		goto done;
+
+done:
+	return fd;
+}
+
 /* structure definitions used locally ****************************************/
 
 /* node handle limits
@@ -238,13 +269,8 @@ dirattr_init(char *hkdir, bool_t resume, uint64_t dircnt)
 			return BOOL_FALSE;
 		}
 	} else {
-		/* create the dirattr file, first unlinking any older version
-		 * laying around
-		 */
-		(void)unlink(dtp->dt_pathname);
-		dtp->dt_fd = open(dtp->dt_pathname,
-				   O_RDWR | O_CREAT | O_EXCL,
-				   S_IRUSR | S_IWUSR);
+		dtp->dt_fd = create_filled_file(dtp->dt_pathname,
+			DIRATTR_PERS_SZ + (dircnt * sizeof(struct dirattr)));
 		if (dtp->dt_fd < 0) {
 			mlog(MLOG_NORMAL | MLOG_ERROR, _(
 			      "could not create directory attributes file %s: "
@@ -253,63 +279,6 @@ dirattr_init(char *hkdir, bool_t resume, uint64_t dircnt)
 			      strerror(errno));
 			return BOOL_FALSE;
 		}
-
-		/* reserve space for the backing store. try to use RESVSP64.
-		 * if doesn't work, try ALLOCSP64. the former is faster, as
-		 * it does not zero the space.
-		 */
-		{
-		bool_t successpr;
-		unsigned int ioctlcmd;
-		int loglevel;
-		size_t trycnt;
-
-		for (trycnt = 0,
-		      successpr = BOOL_FALSE,
-		      ioctlcmd = XFS_IOC_RESVSP64,
-		      loglevel = MLOG_VERBOSE
-		      ;
-		      !successpr && trycnt < 2
-		      ;
-		      trycnt++,
-		      ioctlcmd = XFS_IOC_ALLOCSP64,
-		      loglevel = max(MLOG_NORMAL, loglevel - 1)) {
-			off64_t initsz;
-			struct flock64 flock64;
-			int rval;
-
-			if (!ioctlcmd) {
-				continue;
-			}
-
-			initsz = (off64_t)DIRATTR_PERS_SZ
-				 +
-				 ((off64_t)dircnt * sizeof(dirattr_t));
-			flock64.l_whence = 0;
-			flock64.l_start = 0;
-			flock64.l_len = initsz;
-			rval = ioctl(dtp->dt_fd, ioctlcmd, &flock64);
-			if (rval) {
-				if (errno != ENOTTY) {
-					mlog(loglevel | MLOG_NOTE, _(
-					      "attempt to reserve %lld bytes for %s "
-					      "using %s "
-					      "failed: %s (%d)\n"),
-					      initsz,
-					      dtp->dt_pathname,
-					      ioctlcmd == XFS_IOC_RESVSP64
-					      ?
-					      "XFS_IOC_RESVSP64"
-					      :
-					      "XFS_IOC_ALLOCSP64",
-					      strerror(errno),
-					      errno);
-				}
-			} else {
-				successpr = BOOL_TRUE;
-			}
-		}
-		}
 	}
 
 	/* mmap the persistent descriptor
diff --git a/restore/dirattr.h b/restore/dirattr.h
index 232822e..cdfa4fc 100644
--- a/restore/dirattr.h
+++ b/restore/dirattr.h
@@ -88,4 +88,6 @@ extern bool_t dirattr_cb_extattr(dah_t dah,
 				  extattrhdr_t *ahdrp,
 				  void *ctxp);
 
+int create_filled_file(const char *pathname, off64_t size);
+
 #endif /* DIRATTR_H */
diff --git a/restore/namreg.c b/restore/namreg.c
index fe159e4..594e325 100644
--- a/restore/namreg.c
+++ b/restore/namreg.c
@@ -37,6 +37,10 @@
 #include "namreg.h"
 #include "openutil.h"
 #include "mmap.h"
+#include "global.h"
+#include "content.h"
+#include "content_inode.h"
+#include "dirattr.h"
 
 /* structure definitions used locally ****************************************/
 
@@ -153,13 +157,8 @@ namreg_init(char *hkdir, bool_t resume, uint64_t inocnt)
 			return BOOL_FALSE;
 		}
 	} else {
-		/* create the namreg file, first unlinking any older version
-		 * laying around
-		 */
-		(void)unlink(ntp->nt_pathname);
-		ntp->nt_fd = open(ntp->nt_pathname,
-				   O_RDWR | O_CREAT | O_EXCL,
-				   S_IRUSR | S_IWUSR);
+		ntp->nt_fd = create_filled_file(ntp->nt_pathname,
+			NAMREG_PERS_SZ + (inocnt * NAMREG_AVGLEN));
 		if (ntp->nt_fd < 0) {
 			mlog(MLOG_NORMAL | MLOG_ERROR, _(
 			      "could not create name registry file %s: "
@@ -168,63 +167,6 @@ namreg_init(char *hkdir, bool_t resume, uint64_t inocnt)
 			      strerror(errno));
 			return BOOL_FALSE;
 		}
-
-		/* reserve space for the backing store. try to use RESVSP64.
-		 * if doesn't work, try ALLOCSP64. the former is faster, as
-		 * it does not zero the space.
-		 */
-		{
-		bool_t successpr;
-		unsigned int ioctlcmd;
-		int loglevel;
-		size_t trycnt;
-
-		for (trycnt = 0,
-		      successpr = BOOL_FALSE,
-		      ioctlcmd = XFS_IOC_RESVSP64,
-		      loglevel = MLOG_VERBOSE
-		      ;
-		      !successpr && trycnt < 2
-		      ;
-		      trycnt++,
-		      ioctlcmd = XFS_IOC_ALLOCSP64,
-		      loglevel = max(MLOG_NORMAL, loglevel - 1)) {
-			off64_t initsz;
-			struct flock64 flock64;
-			int rval;
-
-			if (!ioctlcmd) {
-				continue;
-			}
-
-			initsz = (off64_t)NAMREG_PERS_SZ
-				 +
-				 ((off64_t)inocnt * NAMREG_AVGLEN);
-			flock64.l_whence = 0;
-			flock64.l_start = 0;
-			flock64.l_len = initsz;
-			rval = ioctl(ntp->nt_fd, ioctlcmd, &flock64);
-			if (rval) {
-				if (errno != ENOTTY) {
-					mlog(loglevel | MLOG_NOTE, _(
-					      "attempt to reserve %lld bytes for %s "
-					      "using %s "
-					      "failed: %s (%d)\n"),
-					      initsz,
-					      ntp->nt_pathname,
-					      ioctlcmd == XFS_IOC_RESVSP64
-					      ?
-					      "XFS_IOC_RESVSP64"
-					      :
-					      "XFS_IOC_ALLOCSP64",
-					      strerror(errno),
-					      errno);
-				}
-			} else {
-				successpr = BOOL_TRUE;
-			}
-		}
-		}
 	}
 
 	/* mmap the persistent descriptor


^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-08-20 20:21 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-22 16:47 [PATCH 0/4] xfsdump: update to use fallocate Darrick J. Wong
2019-02-22 16:47 ` [PATCH 1/4] xfs_restore: refactor open-coded file creation code Darrick J. Wong
2019-02-22 19:20   ` Andre Noll
2019-02-22 19:28     ` Darrick J. Wong
2019-02-22 19:55       ` Andre Noll
2019-05-07  0:11   ` Allison Collins
2019-05-20 21:05     ` Darrick J. Wong
2019-02-22 16:47 ` [PATCH 2/4] xfs_restore: check return value Darrick J. Wong
2019-05-07  0:11   ` Allison Collins
2019-02-22 16:47 ` [PATCH 3/4] xfs_restore: fix unsupported ioctl detection Darrick J. Wong
2019-05-07  0:11   ` Allison Collins
2019-02-22 16:47 ` [PATCH 4/4] xfs_restore: support fallocate when reserving space for a file Darrick J. Wong
2019-05-07  0:11   ` Allison Collins
2019-05-06 18:24 ` [PATCH 0/4] xfsdump: update to use fallocate Darrick J. Wong
2019-08-20 20:21 Darrick J. Wong
2019-08-20 20:21 ` [PATCH 1/4] xfs_restore: refactor open-coded file creation code Darrick J. Wong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.