[PATCH v2 1/3] btrfs-progs: fi defrag: clean up duplicate code if find errors

[PATCH v2 1/3] btrfs-progs: fi defrag: clean up duplicate code if find errors

[Su Yue]

In function cmd_filesystem_defrag(), lines of code for error handling
are duplicate and hard to expand in further.

Create a jump label for errors.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
Changelog:
v2:	Record -errno to return value if open() and fstat() failed.
	Move change "do no exit if defrag range ioctl is unsupported"
	to another patch.
	Suggested by David.
---
 cmds-filesystem.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 7728430f16a1..17d399d58adf 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -1029,23 +1029,22 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 		if (fd < 0) {
 			error("cannot open %s: %s", argv[i],
 					strerror(errno));
-			defrag_global_errors++;
-			close_file_or_dir(fd, dirstream);
-			continue;
+			ret = -errno;
+			goto next;
 		}
-		if (fstat(fd, &st)) {
+
+		ret = fstat(fd, &st);
+		if (ret) {
 			error("failed to stat %s: %s",
 					argv[i], strerror(errno));
-			defrag_global_errors++;
-			close_file_or_dir(fd, dirstream);
-			continue;
+			ret = -errno;
+			goto next;
 		}
 		if (!(S_ISDIR(st.st_mode) || S_ISREG(st.st_mode))) {
 			error("%s is not a directory or a regular file",
 					argv[i]);
-			defrag_global_errors++;
-			close_file_or_dir(fd, dirstream);
-			continue;
+			ret = -EINVAL;
+			goto next;
 		}
 		if (recursive && S_ISDIR(st.st_mode)) {
 			ret = nftw(argv[i], defrag_callback, 10,
@@ -1060,20 +1059,25 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 			ret = ioctl(fd, BTRFS_IOC_DEFRAG_RANGE,
 					&defrag_global_range);
 			defrag_err = errno;
-		}
-		close_file_or_dir(fd, dirstream);
-		if (ret && defrag_err == ENOTTY) {
-			error(
+			if (ret && defrag_err == ENOTTY) {
+				error(
 "defrag range ioctl not supported in this kernel version, 2.6.33 and newer is required");
-			defrag_global_errors++;
-			break;
+				defrag_global_errors++;
+				close_file_or_dir(fd, dirstream);
+				break;
+			}
+			if (ret) {
+				error("defrag failed on %s: %s", argv[i],
+				      strerror(defrag_err));
+				goto next;
+			}
 		}
-		if (ret) {
-			error("defrag failed on %s: %s", argv[i],
-					strerror(defrag_err));
+next:
+		if (ret)
 			defrag_global_errors++;
-		}
+		close_file_or_dir(fd, dirstream);
 	}
+
 	if (defrag_global_errors)
 		fprintf(stderr, "total %d failures\n", defrag_global_errors);
 
-- 
2.15.0

[PATCH v2 2/3] btrfs-progs: fi defrag: do not exit if defrag range ioctl is unsupported

[Su Yue]

If ioctl of defrag range is unsupported, defrag will exit
immediately.

Since caller can handle the error, let cmd_filesystem_defrag()
break the loop and return error instead of calling exit(1).

Suggested-by: David Sterba <dsterba@suse.com>
Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
Changelog:
v2:	Separate the patch from commit 6e991b9161fa ("btrfs-progs: fi
	defrag: clean up duplicate code if find errors").
---
 cmds-filesystem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 17d399d58adf..3931333f76c6 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -1050,7 +1050,7 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 			ret = nftw(argv[i], defrag_callback, 10,
 						FTW_MOUNT | FTW_PHYS);
 			if (ret == ENOTTY)
-				exit(1);
+				break;
 			/* errors are handled in the callback */
 			ret = 0;
 		} else {
-- 
2.15.0

Re: [PATCH v2 2/3] btrfs-progs: fi defrag: do not exit if defrag range ioctl is unsupported

[David Sterba]

On Tue, Nov 28, 2017 at 05:14:49PM +0800, Su Yue wrote:
> If ioctl of defrag range is unsupported, defrag will exit
> immediately.
> 
> Since caller can handle the error, let cmd_filesystem_defrag()
> break the loop and return error instead of calling exit(1).
> 
> Suggested-by: David Sterba <dsterba@suse.com>
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
> Changelog:
> v2:	Separate the patch from commit 6e991b9161fa ("btrfs-progs: fi
> 	defrag: clean up duplicate code if find errors").
> ---
>  cmds-filesystem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 17d399d58adf..3931333f76c6 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -1050,7 +1050,7 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  			ret = nftw(argv[i], defrag_callback, 10,
>  						FTW_MOUNT | FTW_PHYS);
>  			if (ret == ENOTTY)
> -				exit(1);
> +				break;

And we still need to call close_file_or_dir.

>  			/* errors are handled in the callback */
>  			ret = 0;
>  		} else {
> -- 
> 2.15.0
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] btrfs-progs: fi defrag: extend -c to drop nocompress flag on files

[Su Yue]

Now, files which have nocompress flag also will be defraged
with compression. However, nocompress flag is still existed
and have to be cleared manually.

So add an option '--clear-nocompress' to extend -c to drop
nocompress flag after defragement.

Suggested-by: David Sterba <dsterba@suse.com>
Suggested-by: Anand Jain <anand.jain@oracle.com>
Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
Changelog:
v2:	Remove change about indentation of defrag_callback().

---
 cmds-filesystem.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 3931333f76c6..84242814798e 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -27,6 +27,7 @@
 #include <mntent.h>
 #include <linux/limits.h>
 #include <getopt.h>
+#include <linux/fs.h>
 
 #include "kerncompat.h"
 #include "ctree.h"
@@ -867,6 +868,8 @@ static const char * const cmd_filesystem_defrag_usage[] = {
 	"-l len              defragment only up to len bytes",
 	"-t size             target extent size hint (default: 32M)",
 	"",
+	"--compress-force    clear nocompress flag on files after defragment, only work with option -c",
+	"",
 	"Warning: most Linux kernels will break up the ref-links of COW data",
 	"(e.g., files copied with 'cp --reflink', snapshots) which may cause",
 	"considerable increase of space usage. See btrfs-filesystem(8) for",
@@ -874,9 +877,39 @@ static const char * const cmd_filesystem_defrag_usage[] = {
 	NULL
 };
 
+static int clear_nocompress_flag(int fd)
+{
+	unsigned int flags;
+	int ret = 0;
+
+	ret = ioctl(fd, FS_IOC_GETFLAGS, &flags);
+	if (ret < 0) {
+		ret = -errno;
+		error("failed to get flags: %s", strerror(-ret));
+		goto out;
+	}
+
+	if (!(flags & FS_NOCOMP_FL)) {
+		ret = 0;
+		goto out;
+	}
+	flags &= ~FS_NOCOMP_FL;
+	ret = ioctl(fd, FS_IOC_SETFLAGS, &flags);
+	if (ret < 0) {
+		ret = -errno;
+		error("failed to set flags: %s", strerror(-ret));
+		goto out;
+	}
+
+	ret = 0;
+out:
+	return ret;
+}
+
 static struct btrfs_ioctl_defrag_range_args defrag_global_range;
 static int defrag_global_verbose;
 static int defrag_global_errors;
+static int defrag_global_clear_nocompress;
 static int defrag_callback(const char *fpath, const struct stat *sb,
 		int typeflag, struct FTW *ftwbuf)
 {
@@ -904,6 +937,14 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
 			err = errno;
 			goto error;
 		}
+
+		if (defrag_global_clear_nocompress) {
+			ret = clear_nocompress_flag(fd);
+			if (ret) {
+				err = -ret;
+				goto error;
+			}
+		}
 	}
 	return 0;
 
@@ -926,6 +967,12 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 	int compress_type = BTRFS_COMPRESS_NONE;
 	DIR *dirstream;
 
+	enum { GETOPT_VAL_CLEAR_NOCOMPRESS = 257};
+	static const struct option long_options[] = {
+		{ "clear-nocompress", no_argument, NULL,
+		  GETOPT_VAL_CLEAR_NOCOMPRESS},
+		{ NULL, 0, NULL, 0}
+	};
 	/*
 	 * Kernel has a different default (256K) that is supposed to be safe,
 	 * but it does not defragment very well. The 32M will likely lead to
@@ -937,8 +984,10 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 	defrag_global_errors = 0;
 	defrag_global_verbose = 0;
 	defrag_global_errors = 0;
+	defrag_global_clear_nocompress = 0;
 	while(1) {
-		int c = getopt(argc, argv, "vrc::fs:l:t:");
+		int c = getopt_long(argc, argv, "vrc::fs:l:t:", long_options,
+				    NULL);
 		if (c < 0)
 			break;
 
@@ -972,6 +1021,9 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 		case 'r':
 			recursive = 1;
 			break;
+		case GETOPT_VAL_CLEAR_NOCOMPRESS:
+			defrag_global_clear_nocompress = 1;
+			break;
 		default:
 			usage(cmd_filesystem_defrag_usage);
 		}
@@ -987,6 +1039,8 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 	if (compress_type) {
 		defrag_global_range.flags |= BTRFS_DEFRAG_RANGE_COMPRESS;
 		defrag_global_range.compress_type = compress_type;
+	} else if (defrag_global_clear_nocompress) {
+		warning("Option --clear-nocompress only works for -c");
 	}
 	if (flush)
 		defrag_global_range.flags |= BTRFS_DEFRAG_RANGE_START_IO;
@@ -1071,6 +1125,15 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 				      strerror(defrag_err));
 				goto next;
 			}
+
+			if (defrag_global_clear_nocompress)
+				ret = clear_nocompress_flag(fd);
+			if (ret) {
+				error(
+				"failed to drop nocompress flag on %s: %s",
+				argv[i], strerror(-ret));
+				goto next;
+			}
 		}
 next:
 		if (ret)
-- 
2.15.0

Re: [PATCH v2 3/3] btrfs-progs: fi defrag: extend -c to drop nocompress flag on files

[David Sterba]

On Tue, Nov 28, 2017 at 05:14:50PM +0800, Su Yue wrote:
> Now, files which have nocompress flag also will be defraged
> with compression. However, nocompress flag is still existed
> and have to be cleared manually.
> 
> So add an option '--clear-nocompress' to extend -c to drop
> nocompress flag after defragement.

> Suggested-by: David Sterba <dsterba@suse.com>
> Suggested-by: Anand Jain <anand.jain@oracle.com>

Do you have the pointer to the discussion? The whole idea sounds
familiar and seeing my name here means I must have been involved, but I
have only vague memories.

> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
> Changelog:
> v2:	Remove change about indentation of defrag_callback().
> 
> ---
>  cmds-filesystem.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 3931333f76c6..84242814798e 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -27,6 +27,7 @@
>  #include <mntent.h>
>  #include <linux/limits.h>
>  #include <getopt.h>
> +#include <linux/fs.h>
>  
>  #include "kerncompat.h"
>  #include "ctree.h"
> @@ -867,6 +868,8 @@ static const char * const cmd_filesystem_defrag_usage[] = {
>  	"-l len              defragment only up to len bytes",
>  	"-t size             target extent size hint (default: 32M)",
>  	"",
> +	"--compress-force    clear nocompress flag on files after defragment, only work with option -c",

This is probably --clear-nocompress .

The "force compress" semantics is up to kernel, so I think naming the
options 'clear-nocompress' is the right thing, as it's what it really
does.

> +	"",
>  	"Warning: most Linux kernels will break up the ref-links of COW data",
>  	"(e.g., files copied with 'cp --reflink', snapshots) which may cause",
>  	"considerable increase of space usage. See btrfs-filesystem(8) for",
> @@ -874,9 +877,39 @@ static const char * const cmd_filesystem_defrag_usage[] = {
>  	NULL
>  };
>  
> +static int clear_nocompress_flag(int fd)
> +{
> +	unsigned int flags;
> +	int ret = 0;
> +
> +	ret = ioctl(fd, FS_IOC_GETFLAGS, &flags);
> +	if (ret < 0) {
> +		ret = -errno;
> +		error("failed to get flags: %s", strerror(-ret));
> +		goto out;
> +	}
> +
> +	if (!(flags & FS_NOCOMP_FL)) {
> +		ret = 0;
> +		goto out;
> +	}
> +	flags &= ~FS_NOCOMP_FL;
> +	ret = ioctl(fd, FS_IOC_SETFLAGS, &flags);

This is inherently racy, but the inode flags do not change that often so
concurrent change is unlikely.

> +	if (ret < 0) {
> +		ret = -errno;
> +		error("failed to set flags: %s", strerror(-ret));
> +		goto out;
> +	}
> +
> +	ret = 0;
> +out:
> +	return ret;
> +}
> +
>  static struct btrfs_ioctl_defrag_range_args defrag_global_range;
>  static int defrag_global_verbose;
>  static int defrag_global_errors;
> +static int defrag_global_clear_nocompress;
>  static int defrag_callback(const char *fpath, const struct stat *sb,
>  		int typeflag, struct FTW *ftwbuf)
>  {
> @@ -904,6 +937,14 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
>  			err = errno;
>  			goto error;
>  		}
> +
> +		if (defrag_global_clear_nocompress) {
> +			ret = clear_nocompress_flag(fd);
> +			if (ret) {
> +				err = -ret;
> +				goto error;
> +			}
> +		}
>  	}
>  	return 0;
>  
> @@ -926,6 +967,12 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  	int compress_type = BTRFS_COMPRESS_NONE;
>  	DIR *dirstream;
>  
> +	enum { GETOPT_VAL_CLEAR_NOCOMPRESS = 257};
> +	static const struct option long_options[] = {
> +		{ "clear-nocompress", no_argument, NULL,
> +		  GETOPT_VAL_CLEAR_NOCOMPRESS},
> +		{ NULL, 0, NULL, 0}
> +	};
>  	/*
>  	 * Kernel has a different default (256K) that is supposed to be safe,
>  	 * but it does not defragment very well. The 32M will likely lead to
> @@ -937,8 +984,10 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  	defrag_global_errors = 0;
>  	defrag_global_verbose = 0;
>  	defrag_global_errors = 0;
> +	defrag_global_clear_nocompress = 0;
>  	while(1) {
> -		int c = getopt(argc, argv, "vrc::fs:l:t:");
> +		int c = getopt_long(argc, argv, "vrc::fs:l:t:", long_options,
> +				    NULL);
>  		if (c < 0)
>  			break;
>  
> @@ -972,6 +1021,9 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  		case 'r':
>  			recursive = 1;
>  			break;
> +		case GETOPT_VAL_CLEAR_NOCOMPRESS:
> +			defrag_global_clear_nocompress = 1;
> +			break;
>  		default:
>  			usage(cmd_filesystem_defrag_usage);
>  		}
> @@ -987,6 +1039,8 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  	if (compress_type) {
>  		defrag_global_range.flags |= BTRFS_DEFRAG_RANGE_COMPRESS;
>  		defrag_global_range.compress_type = compress_type;
> +	} else if (defrag_global_clear_nocompress) {
> +		warning("Option --clear-nocompress only works for -c");
>  	}
>  	if (flush)
>  		defrag_global_range.flags |= BTRFS_DEFRAG_RANGE_START_IO;
> @@ -1071,6 +1125,15 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  				      strerror(defrag_err));
>  				goto next;
>  			}
> +
> +			if (defrag_global_clear_nocompress)
> +				ret = clear_nocompress_flag(fd);
> +			if (ret) {
> +				error(
> +				"failed to drop nocompress flag on %s: %s",
> +				argv[i], strerror(-ret));
> +				goto next;
> +			}
>  		}
>  next:
>  		if (ret)
> -- 
> 2.15.0
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] btrfs-progs: fi defrag: extend -c to drop nocompress flag on files

[Su Yue]

On 11/29/2017 12:07 AM, David Sterba wrote:
> On Tue, Nov 28, 2017 at 05:14:50PM +0800, Su Yue wrote:
>> Now, files which have nocompress flag also will be defraged
>> with compression. However, nocompress flag is still existed
>> and have to be cleared manually.
>>
>> So add an option '--clear-nocompress' to extend -c to drop
>> nocompress flag after defragement.
> 
>> Suggested-by: David Sterba <dsterba@suse.com>
>> Suggested-by: Anand Jain <anand.jain@oracle.com>
> 
> Do you have the pointer to the discussion? The whole idea sounds
> familiar and seeing my name here means I must have been involved, but I
> have only vague memories.
> 

Here is the link:
https://www.spinics.net/lists/linux-btrfs/msg67529.html

>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
>> ---
>> Changelog:
>> v2:	Remove change about indentation of defrag_callback().
>>
>> ---
>>   cmds-filesystem.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
>> index 3931333f76c6..84242814798e 100644
>> --- a/cmds-filesystem.c
>> +++ b/cmds-filesystem.c
>> @@ -27,6 +27,7 @@
>>   #include <mntent.h>
>>   #include <linux/limits.h>
>>   #include <getopt.h>
>> +#include <linux/fs.h>
>>   
>>   #include "kerncompat.h"
>>   #include "ctree.h"
>> @@ -867,6 +868,8 @@ static const char * const cmd_filesystem_defrag_usage[] = {
>>   	"-l len              defragment only up to len bytes",
>>   	"-t size             target extent size hint (default: 32M)",
>>   	"",
>> +	"--compress-force    clear nocompress flag on files after defragment, only work with option -c",
> 
> This is probably --clear-nocompress .
> 

Oh... I forgot to change the usage info here.

> The "force compress" semantics is up to kernel, so I think naming the
> options 'clear-nocompress' is the right thing, as it's what it really
> does.
> 
>> +	"",
>>   	"Warning: most Linux kernels will break up the ref-links of COW data",
>>   	"(e.g., files copied with 'cp --reflink', snapshots) which may cause",
>>   	"considerable increase of space usage. See btrfs-filesystem(8) for",
>> @@ -874,9 +877,39 @@ static const char * const cmd_filesystem_defrag_usage[] = {
>>   	NULL
>>   };
>>   
>> +static int clear_nocompress_flag(int fd)
>> +{
>> +	unsigned int flags;
>> +	int ret = 0;
>> +
>> +	ret = ioctl(fd, FS_IOC_GETFLAGS, &flags);
>> +	if (ret < 0) {
>> +		ret = -errno;
>> +		error("failed to get flags: %s", strerror(-ret));
>> +		goto out;
>> +	}
>> +
>> +	if (!(flags & FS_NOCOMP_FL)) {
>> +		ret = 0;
>> +		goto out;
>> +	}
>> +	flags &= ~FS_NOCOMP_FL;
>> +	ret = ioctl(fd, FS_IOC_SETFLAGS, &flags);
> 
> This is inherently racy, but the inode flags do not change that often so
> concurrent change is unlikely.
> 

I don't quite understand what you mean.
Could you explain some details?

Thanks,
Su
>> +	if (ret < 0) {
>> +		ret = -errno;
>> +		error("failed to set flags: %s", strerror(-ret));
>> +		goto out;
>> +	}
>> +
>> +	ret = 0;
>> +out:
>> +	return ret;
>> +}
>> +
>>   static struct btrfs_ioctl_defrag_range_args defrag_global_range;
>>   static int defrag_global_verbose;
>>   static int defrag_global_errors;
>> +static int defrag_global_clear_nocompress;
>>   static int defrag_callback(const char *fpath, const struct stat *sb,
>>   		int typeflag, struct FTW *ftwbuf)
>>   {
>> @@ -904,6 +937,14 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
>>   			err = errno;
>>   			goto error;
>>   		}
>> +
>> +		if (defrag_global_clear_nocompress) {
>> +			ret = clear_nocompress_flag(fd);
>> +			if (ret) {
>> +				err = -ret;
>> +				goto error;
>> +			}
>> +		}
>>   	}
>>   	return 0;
>>   
>> @@ -926,6 +967,12 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>>   	int compress_type = BTRFS_COMPRESS_NONE;
>>   	DIR *dirstream;
>>   
>> +	enum { GETOPT_VAL_CLEAR_NOCOMPRESS = 257};
>> +	static const struct option long_options[] = {
>> +		{ "clear-nocompress", no_argument, NULL,
>> +		  GETOPT_VAL_CLEAR_NOCOMPRESS},
>> +		{ NULL, 0, NULL, 0}
>> +	};
>>   	/*
>>   	 * Kernel has a different default (256K) that is supposed to be safe,
>>   	 * but it does not defragment very well. The 32M will likely lead to
>> @@ -937,8 +984,10 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>>   	defrag_global_errors = 0;
>>   	defrag_global_verbose = 0;
>>   	defrag_global_errors = 0;
>> +	defrag_global_clear_nocompress = 0;
>>   	while(1) {
>> -		int c = getopt(argc, argv, "vrc::fs:l:t:");
>> +		int c = getopt_long(argc, argv, "vrc::fs:l:t:", long_options,
>> +				    NULL);
>>   		if (c < 0)
>>   			break;
>>   
>> @@ -972,6 +1021,9 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>>   		case 'r':
>>   			recursive = 1;
>>   			break;
>> +		case GETOPT_VAL_CLEAR_NOCOMPRESS:
>> +			defrag_global_clear_nocompress = 1;
>> +			break;
>>   		default:
>>   			usage(cmd_filesystem_defrag_usage);
>>   		}
>> @@ -987,6 +1039,8 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>>   	if (compress_type) {
>>   		defrag_global_range.flags |= BTRFS_DEFRAG_RANGE_COMPRESS;
>>   		defrag_global_range.compress_type = compress_type;
>> +	} else if (defrag_global_clear_nocompress) {
>> +		warning("Option --clear-nocompress only works for -c");
>>   	}
>>   	if (flush)
>>   		defrag_global_range.flags |= BTRFS_DEFRAG_RANGE_START_IO;
>> @@ -1071,6 +1125,15 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>>   				      strerror(defrag_err));
>>   				goto next;
>>   			}
>> +
>> +			if (defrag_global_clear_nocompress)
>> +				ret = clear_nocompress_flag(fd);
>> +			if (ret) {
>> +				error(
>> +				"failed to drop nocompress flag on %s: %s",
>> +				argv[i], strerror(-ret));
>> +				goto next;
>> +			}
>>   		}
>>   next:
>>   		if (ret)
>> -- 
>> 2.15.0
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH v2 3/3] btrfs-progs: fi defrag: extend -c to drop nocompress flag on files

[David Sterba]

On Wed, Nov 29, 2017 at 09:54:01AM +0800, Su Yue wrote:
> On 11/29/2017 12:07 AM, David Sterba wrote:
> > On Tue, Nov 28, 2017 at 05:14:50PM +0800, Su Yue wrote:
> >> Now, files which have nocompress flag also will be defraged
> >> with compression. However, nocompress flag is still existed
> >> and have to be cleared manually.
> >>
> >> So add an option '--clear-nocompress' to extend -c to drop
> >> nocompress flag after defragement.
> > 
> >> Suggested-by: David Sterba <dsterba@suse.com>
> >> Suggested-by: Anand Jain <anand.jain@oracle.com>
> > 
> > Do you have the pointer to the discussion? The whole idea sounds
> > familiar and seeing my name here means I must have been involved, but I
> > have only vague memories.
> > 
> 
> Here is the link:
> https://www.spinics.net/lists/linux-btrfs/msg67529.html

Thanks.

> >> +static int clear_nocompress_flag(int fd)
> >> +{
> >> +	unsigned int flags;
> >> +	int ret = 0;
> >> +
> >> +	ret = ioctl(fd, FS_IOC_GETFLAGS, &flags);
> >> +	if (ret < 0) {
> >> +		ret = -errno;
> >> +		error("failed to get flags: %s", strerror(-ret));
> >> +		goto out;
> >> +	}
> >> +
> >> +	if (!(flags & FS_NOCOMP_FL)) {
> >> +		ret = 0;
> >> +		goto out;
> >> +	}
> >> +	flags &= ~FS_NOCOMP_FL;
> >> +	ret = ioctl(fd, FS_IOC_SETFLAGS, &flags);
> > 
> > This is inherently racy, but the inode flags do not change that often so
> > concurrent change is unlikely.
> > 
> 
> I don't quite understand what you mean.
> Could you explain some details?

If the inode flags change externally between the set and get calls, but
there's not much we can do about it now.

Re: [PATCH v2 1/3] btrfs-progs: fi defrag: clean up duplicate code if find errors

[David Sterba]

On Tue, Nov 28, 2017 at 05:14:48PM +0800, Su Yue wrote:
> In function cmd_filesystem_defrag(), lines of code for error handling
> are duplicate and hard to expand in further.
> 
> Create a jump label for errors.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>

Applied, thanks.

[PATCH v3 2/3] btrfs-progs: fi defrag: do not exit if defrag range ioctl is unsupported

[Su Yue]

If ioctl of defrag range is unsupported, defrag will exit immediately.

Since caller can handle the error, let cmd_filesystem_defrag()
close file, break the loop and return error instead of calling exit(1).

Suggested-by: David Sterba <dsterba@suse.com>
Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
Changelog:
v2:	Separate the patch from commit 6e991b9161fa ("btrfs-progs: fi
	defrag: clean up duplicate code if find errors").
v3:	Call close_file_or_dir() before breaking the loop.
---
 cmds-filesystem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 17d399d58adf..232d4e88e0c0 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -1049,8 +1049,10 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 		if (recursive && S_ISDIR(st.st_mode)) {
 			ret = nftw(argv[i], defrag_callback, 10,
 						FTW_MOUNT | FTW_PHYS);
-			if (ret == ENOTTY)
-				exit(1);
+			if (ret == ENOTTY) {
+				close_file_or_dir(fd, dirstream);
+				break;
+			}
 			/* errors are handled in the callback */
 			ret = 0;
 		} else {
-- 
2.15.0

Re: [PATCH v3 2/3] btrfs-progs: fi defrag: do not exit if defrag range ioctl is unsupported

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1308 bytes --]



On 2017年11月29日 10:12, Su Yue wrote:
> If ioctl of defrag range is unsupported, defrag will exit immediately.
> 
> Since caller can handle the error, let cmd_filesystem_defrag()
> close file, break the loop and return error instead of calling exit(1).
> 
> Suggested-by: David Sterba <dsterba@suse.com>
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
> Changelog:
> v2:	Separate the patch from commit 6e991b9161fa ("btrfs-progs: fi
> 	defrag: clean up duplicate code if find errors").
> v3:	Call close_file_or_dir() before breaking the loop.
> ---
>  cmds-filesystem.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 17d399d58adf..232d4e88e0c0 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -1049,8 +1049,10 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  		if (recursive && S_ISDIR(st.st_mode)) {
>  			ret = nftw(argv[i], defrag_callback, 10,
>  						FTW_MOUNT | FTW_PHYS);
> -			if (ret == ENOTTY)
> -				exit(1);
> +			if (ret == ENOTTY) {
> +				close_file_or_dir(fd, dirstream);
> +				break;
> +			}
>  			/* errors are handled in the callback */
>  			ret = 0;
>  		} else {
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH v3 2/3] btrfs-progs: fi defrag: do not exit if defrag range ioctl is unsupported

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1500 bytes --]



On 2017年11月30日 14:37, Qu Wenruo wrote:
> 
> 
> On 2017年11月29日 10:12, Su Yue wrote:
>> If ioctl of defrag range is unsupported, defrag will exit immediately.
>>
>> Since caller can handle the error, let cmd_filesystem_defrag()
>> close file, break the loop and return error instead of calling exit(1).
>>
>> Suggested-by: David Sterba <dsterba@suse.com>
>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Thanks,
> Qu

Please ignore my tag.

The best solution is mentioned in the 1st patch.

Thanks,
Qu

>> ---
>> Changelog:
>> v2:	Separate the patch from commit 6e991b9161fa ("btrfs-progs: fi
>> 	defrag: clean up duplicate code if find errors").
>> v3:	Call close_file_or_dir() before breaking the loop.
>> ---
>>  cmds-filesystem.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
>> index 17d399d58adf..232d4e88e0c0 100644
>> --- a/cmds-filesystem.c
>> +++ b/cmds-filesystem.c
>> @@ -1049,8 +1049,10 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>>  		if (recursive && S_ISDIR(st.st_mode)) {
>>  			ret = nftw(argv[i], defrag_callback, 10,
>>  						FTW_MOUNT | FTW_PHYS);
>> -			if (ret == ENOTTY)
>> -				exit(1);
>> +			if (ret == ENOTTY) {
>> +				close_file_or_dir(fd, dirstream);
>> +				break;
>> +			}
>>  			/* errors are handled in the callback */
>>  			ret = 0;
>>  		} else {
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH v2 1/3] btrfs-progs: fi defrag: clean up duplicate code if find errors

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3250 bytes --]



On 2017年11月28日 17:14, Su Yue wrote:
> In function cmd_filesystem_defrag(), lines of code for error handling
> are duplicate and hard to expand in further.
> 
> Create a jump label for errors.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
> Changelog:
> v2:	Record -errno to return value if open() and fstat() failed.
> 	Move change "do no exit if defrag range ioctl is unsupported"
> 	to another patch.
> 	Suggested by David.
> ---
>  cmds-filesystem.c | 44 ++++++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 7728430f16a1..17d399d58adf 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -1029,23 +1029,22 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  		if (fd < 0) {
>  			error("cannot open %s: %s", argv[i],
>  					strerror(errno));
> -			defrag_global_errors++;
> -			close_file_or_dir(fd, dirstream);
> -			continue;
> +			ret = -errno;
> +			goto next;
>  		}
> -		if (fstat(fd, &st)) {
> +
> +		ret = fstat(fd, &st);
> +		if (ret) {
>  			error("failed to stat %s: %s",
>  					argv[i], strerror(errno));
> -			defrag_global_errors++;
> -			close_file_or_dir(fd, dirstream);
> -			continue;
> +			ret = -errno;
> +			goto next;
>  		}
>  		if (!(S_ISDIR(st.st_mode) || S_ISREG(st.st_mode))) {
>  			error("%s is not a directory or a regular file",
>  					argv[i]);
> -			defrag_global_errors++;
> -			close_file_or_dir(fd, dirstream);
> -			continue;
> +			ret = -EINVAL;
> +			goto next;

Here, unlike the ftw callback, which ignore non-regular file, here we
count them as error.

So if there is some especial file in dir (use "example_dir" as an
example), then
# btrfs fi defrag -r example_dir
will return no error.

While
# btrfs fi defrag -r example_dir/*
Will return error.

IIRC such inconsistent behavior is a bigger problem, and reusing the
defrag_callback() will not only makes the behavior consistent, but also
simplify the loop.

Thanks,
Qu


>  		}
>  		if (recursive && S_ISDIR(st.st_mode)) {
>  			ret = nftw(argv[i], defrag_callback, 10,
> @@ -1060,20 +1059,25 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  			ret = ioctl(fd, BTRFS_IOC_DEFRAG_RANGE,
>  					&defrag_global_range);
>  			defrag_err = errno;
> -		}
> -		close_file_or_dir(fd, dirstream);
> -		if (ret && defrag_err == ENOTTY) {
> -			error(
> +			if (ret && defrag_err == ENOTTY) {
> +				error(
>  "defrag range ioctl not supported in this kernel version, 2.6.33 and newer is required");
> -			defrag_global_errors++;
> -			break;
> +				defrag_global_errors++;
> +				close_file_or_dir(fd, dirstream);
> +				break;
> +			}
> +			if (ret) {
> +				error("defrag failed on %s: %s", argv[i],
> +				      strerror(defrag_err));
> +				goto next;
> +			}
>  		}
> -		if (ret) {
> -			error("defrag failed on %s: %s", argv[i],
> -					strerror(defrag_err));
> +next:
> +		if (ret)
>  			defrag_global_errors++;
> -		}
> +		close_file_or_dir(fd, dirstream);
>  	}
> +
>  	if (defrag_global_errors)
>  		fprintf(stderr, "total %d failures\n", defrag_global_errors);
>  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]