diff mbox

duperemove: test presence of dedupe ioctl

Message ID 20161209175645.GB16813@birch.djwong.org
State Superseded, archived
Headers show

Commit Message

Darrick J. Wong Dec. 9, 2016, 5:56 p.m. UTC
Since a zero-length dedupe operation is guaranteed to succeed, use that
to test whether or not this filesystem supports dedupe.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 file_scan.c |   47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christoph Hellwig Dec. 14, 2016, 10:44 a.m. UTC | #1
On Fri, Dec 09, 2016 at 09:56:45AM -0800, Darrick J. Wong wrote:
> Since a zero-length dedupe operation is guaranteed to succeed, use that
> to test whether or not this filesystem supports dedupe.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  file_scan.c |   47 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/file_scan.c b/file_scan.c
> index 617f166..a34453e 100644
> --- a/file_scan.c
> +++ b/file_scan.c
> @@ -45,11 +45,7 @@
>  #include "file_scan.h"
>  #include "dbfile.h"
>  #include "util.h"
> -
> -/* This is not in linux/magic.h */
> -#ifndef	XFS_SB_MAGIC
> -#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
> -#endif
> +#include "btrfs-ioctl.h"
>  
>  static char path[PATH_MAX] = { 0, };
>  static char *pathp = path;
> @@ -189,6 +185,39 @@ static int walk_dir(const char *name)
>  	return ret;
>  }
>  
> +struct fake_btrfs_ioctl_same_args {
> +	struct btrfs_ioctl_same_args args;
> +	struct btrfs_ioctl_same_extent_info info;
> +};

Why does this need a fake structure here?
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Dec. 14, 2016, 6:38 p.m. UTC | #2
On Wed, Dec 14, 2016 at 02:44:36AM -0800, Christoph Hellwig wrote:
> On Fri, Dec 09, 2016 at 09:56:45AM -0800, Darrick J. Wong wrote:
> > Since a zero-length dedupe operation is guaranteed to succeed, use that
> > to test whether or not this filesystem supports dedupe.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  file_scan.c |   47 +++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 37 insertions(+), 10 deletions(-)
> > 
> > diff --git a/file_scan.c b/file_scan.c
> > index 617f166..a34453e 100644
> > --- a/file_scan.c
> > +++ b/file_scan.c
> > @@ -45,11 +45,7 @@
> >  #include "file_scan.h"
> >  #include "dbfile.h"
> >  #include "util.h"
> > -
> > -/* This is not in linux/magic.h */
> > -#ifndef	XFS_SB_MAGIC
> > -#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
> > -#endif
> > +#include "btrfs-ioctl.h"
> >  
> >  static char path[PATH_MAX] = { 0, };
> >  static char *pathp = path;
> > @@ -189,6 +185,39 @@ static int walk_dir(const char *name)
> >  	return ret;
> >  }
> >  
> > +struct fake_btrfs_ioctl_same_args {
> > +	struct btrfs_ioctl_same_args args;
> > +	struct btrfs_ioctl_same_extent_info info;
> > +};
> 
> Why does this need a fake structure here?

In order to test the ioctl we have to fill out at least one
btrfs_ioctl_same_extent_info so that we get far enough into the fs-specific
dedupe_range handler that we've verified that the fs is capable of dedupe and
that the fs is willing to try to satisfy the request.

We could just malloc sizeof(_same_args) + sizeof(_same_extent_info)...

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Dec. 14, 2016, 7:26 p.m. UTC | #3
On Wed, Dec 14, 2016 at 10:38:45AM -0800, Darrick J. Wong wrote:
> > > +struct fake_btrfs_ioctl_same_args {
> > > +	struct btrfs_ioctl_same_args args;
> > > +	struct btrfs_ioctl_same_extent_info info;
> > > +};
> > 
> > Why does this need a fake structure here?
> 
> In order to test the ioctl we have to fill out at least one
> btrfs_ioctl_same_extent_info so that we get far enough into the fs-specific
> dedupe_range handler that we've verified that the fs is capable of dedupe and
> that the fs is willing to try to satisfy the request.

Oh, got it, it's just the fake that tripped me up.

> We could just malloc sizeof(_same_args) + sizeof(_same_extent_info)...

Either that, or more simply just don't give the structure a name
by just declaring it locally on the stack:

	struct {
		struct btrfs_ioctl_same_args args;
		struct btrfs_ioctl_same_extent_info info;
	} sa = { 0 };

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Dec. 16, 2016, 1:20 a.m. UTC | #4
On Wed, Dec 14, 2016 at 11:26:07AM -0800, Christoph Hellwig wrote:
> On Wed, Dec 14, 2016 at 10:38:45AM -0800, Darrick J. Wong wrote:
> > > > +struct fake_btrfs_ioctl_same_args {
> > > > +	struct btrfs_ioctl_same_args args;
> > > > +	struct btrfs_ioctl_same_extent_info info;
> > > > +};
> > > 
> > > Why does this need a fake structure here?
> > 
> > In order to test the ioctl we have to fill out at least one
> > btrfs_ioctl_same_extent_info so that we get far enough into the fs-specific
> > dedupe_range handler that we've verified that the fs is capable of dedupe and
> > that the fs is willing to try to satisfy the request.
> 
> Oh, got it, it's just the fake that tripped me up.
> 
> > We could just malloc sizeof(_same_args) + sizeof(_same_extent_info)...
> 
> Either that, or more simply just don't give the structure a name
> by just declaring it locally on the stack:
> 
> 	struct {
> 		struct btrfs_ioctl_same_args args;
> 		struct btrfs_ioctl_same_extent_info info;
> 	} sa = { 0 };

Fair enough, no need to pollute the namespace.

--D

> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Dec. 16, 2016, 7:53 a.m. UTC | #5
On Thu, Dec 15, 2016 at 05:20:14PM -0800, Darrick J. Wong wrote:
> Fair enough, no need to pollute the namespace.

Or confuse poor readers with the 'fake' name :)
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/file_scan.c b/file_scan.c
index 617f166..a34453e 100644
--- a/file_scan.c
+++ b/file_scan.c
@@ -45,11 +45,7 @@ 
 #include "file_scan.h"
 #include "dbfile.h"
 #include "util.h"
-
-/* This is not in linux/magic.h */
-#ifndef	XFS_SB_MAGIC
-#define	XFS_SB_MAGIC		0x58465342	/* 'XFSB' */
-#endif
+#include "btrfs-ioctl.h"
 
 static char path[PATH_MAX] = { 0, };
 static char *pathp = path;
@@ -189,6 +185,39 @@  static int walk_dir(const char *name)
 	return ret;
 }
 
+struct fake_btrfs_ioctl_same_args {
+	struct btrfs_ioctl_same_args args;
+	struct btrfs_ioctl_same_extent_info info;
+};
+
+/*
+ * A zero-length dedupe between two files should always succeed,
+ * so we can use this to test the presence of dedupe functionality.
+ */
+static bool check_ioctl_works(int fd)
+{
+	struct fake_btrfs_ioctl_same_args sa = {0};
+	struct stat sb;
+	static int cached = -1;
+	int ret;
+
+	if (cached >= 0)
+		return cached != 0;
+
+	ret = fstat(fd, &sb);
+	if (ret)
+		return false;
+
+	sa.args.dest_count = 1;
+	sa.args.length = 0;
+	sa.info.fd = fd;
+	sa.info.logical_offset = 0;
+	errno = 0;
+	ret = btrfs_extent_same(fd, &sa.args);
+	cached = !ret && !errno && !sa.info.status;
+	return cached != 0;
+}
+
 static int __add_file(const char *name, struct stat *st,
 		      struct filerec **ret_file)
 {
@@ -235,12 +264,10 @@  static int __add_file(const char *name, struct stat *st,
 		goto out;
 	}
 
-	if (run_dedupe &&
-	    ((fs.f_type != BTRFS_SUPER_MAGIC &&
-	      fs.f_type != XFS_SB_MAGIC))) {
+	if (run_dedupe && !check_ioctl_works(fd)) {
 		close(fd);
-		fprintf(stderr,	"\"%s\": Can only dedupe files on btrfs or xfs "
-			"(experimental)\n", name);
+		fprintf(stderr,	"\"%s\": dedupe ioctl not supported on this "
+			"filesystem.\n", name);
 		return ENOSYS;
 	}