[PATCH 0/4] xfsdump: update to use fallocate

[PATCH 0/4] xfsdump: update to use fallocate

[Darrick J. Wong]

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

[PATCH 1/4] xfs_restore: refactor open-coded file creation code

[Darrick J. Wong]

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

Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code

[Andre Noll]

[-- 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 --]

Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code

[Darrick J. Wong]

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/

Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code

[Andre Noll]

[-- 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 --]

Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code

[Allison Collins]

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
> 

Re: [PATCH 1/4] xfs_restore: refactor open-coded file creation code

[Darrick J. Wong]

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
> > 

[PATCH 2/4] xfs_restore: check return value

[Darrick J. Wong]

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)

Re: [PATCH 2/4] xfs_restore: check return value

[Allison Collins]

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)
> 

[PATCH 3/4] xfs_restore: fix unsupported ioctl detection

[Darrick J. Wong]

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",

Re: [PATCH 3/4] xfs_restore: fix unsupported ioctl detection

[Allison Collins]

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",
> 

[PATCH 4/4] xfs_restore: support fallocate when reserving space for a file

[Darrick J. Wong]

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,

Re: [PATCH 4/4] xfs_restore: support fallocate when reserving space for a file

[Allison Collins]

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,
> 

Re: [PATCH 0/4] xfsdump: update to use fallocate

[Darrick J. Wong]

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

[PATCH 0/4] xfsdump: update to use fallocate

[Darrick J. Wong]

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.

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

--D

Re: [PATCH 0/4] xfsdump: update to use fallocate

[Eric Sandeen]

thanks but I actually merged these back in May :D

https://git.kernel.org/pub/scm/fs/xfs/xfsdump-dev.git/commit/?id=36b5cf95733df250fc4401b6a336b7ffc7567b94

On 8/20/19 3:21 PM, 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.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D
>