[PATCH 1/2] btrfs-progs: fix kernel version parsing on some versions past 3.0

[PATCH 1/2] btrfs-progs: fix kernel version parsing on some versions past 3.0

[Adam Borowski]

The code fails if the third section is missing (like "4.18") or is followed
by anything but "." or "-".  This happens for example if we're not exactly
at a tag and CONFIG_LOCALVERSION_AUTO=n (which results in "4.18.5+").

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 fsfeatures.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fsfeatures.c b/fsfeatures.c
index 7d85d60f..66111bf4 100644
--- a/fsfeatures.c
+++ b/fsfeatures.c
@@ -216,11 +216,8 @@ u32 get_running_kernel_version(void)
 		return (u32)-1;
 	version |= atoi(tmp) << 8;
 	tmp = strtok_r(NULL, ".", &saveptr);
-	if (tmp) {
-		if (!string_is_numerical(tmp))
-			return (u32)-1;
+	if (tmp && string_is_numerical(tmp))
 		version |= atoi(tmp);
-	}
 
 	return version;
 }
-- 
2.19.0.rc1

[PATCH 2/2] btrfs-progs: defrag: open files RO on new enough kernels or if root

[Adam Borowski]

Fixes EXTXBSY races.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
 cmds-filesystem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 06c8311b..4c9df69f 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -26,6 +26,7 @@
 #include <ftw.h>
 #include <mntent.h>
 #include <linux/limits.h>
+#include <linux/version.h>
 #include <getopt.h>
 
 #include <btrfsutil.h>
@@ -39,12 +40,14 @@
 #include "list_sort.h"
 #include "disk-io.h"
 #include "help.h"
+#include "fsfeatures.h"
 
 /*
  * for btrfs fi show, we maintain a hash of fsids we've already printed.
  * This way we don't print dups if a given FS is mounted more than once.
  */
 static struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {NULL,};
+static mode_t defrag_ro = O_RDONLY;
 
 static const char * const filesystem_cmd_group_usage[] = {
 	"btrfs filesystem [<group>] <command> [<args>]",
@@ -877,7 +880,7 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
 	if ((typeflag == FTW_F) && S_ISREG(sb->st_mode)) {
 		if (defrag_global_verbose)
 			printf("%s\n", fpath);
-		fd = open(fpath, O_RDWR);
+		fd = open(fpath, defrag_ro);
 		if (fd < 0) {
 			goto error;
 		}
@@ -914,6 +917,9 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 	int compress_type = BTRFS_COMPRESS_NONE;
 	DIR *dirstream;
 
+	if (get_running_kernel_version() < KERNEL_VERSION(4,19,0) && getuid())
+		defrag_ro = O_RDWR;
+
 	/*
 	 * 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
@@ -1014,7 +1020,7 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 		int defrag_err = 0;
 
 		dirstream = NULL;
-		fd = open_file_or_dir(argv[i], &dirstream);
+		fd = open_file_or_dir3(argv[i], &dirstream, defrag_ro);
 		if (fd < 0) {
 			error("cannot open %s: %m", argv[i]);
 			ret = -errno;
-- 
2.19.0.rc1

Re: [PATCH 2/2] btrfs-progs: defrag: open files RO on new enough kernels or if root

[Nikolay Borisov]

On  3.09.2018 13:14, Adam Borowski wrote:
> Fixes EXTXBSY races.

You have to be more eloquent than that and explain at least one race
condition.


> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
>  cmds-filesystem.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 06c8311b..4c9df69f 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -26,6 +26,7 @@
>  #include <ftw.h>
>  #include <mntent.h>
>  #include <linux/limits.h>
> +#include <linux/version.h>
>  #include <getopt.h>
>  
>  #include <btrfsutil.h>
> @@ -39,12 +40,14 @@
>  #include "list_sort.h"
>  #include "disk-io.h"
>  #include "help.h"
> +#include "fsfeatures.h"
>  
>  /*
>   * for btrfs fi show, we maintain a hash of fsids we've already printed.
>   * This way we don't print dups if a given FS is mounted more than once.
>   */
>  static struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {NULL,};
> +static mode_t defrag_ro = O_RDONLY;

This brings no value whatsoever, just use O_RDONLY directly

>  
>  static const char * const filesystem_cmd_group_usage[] = {
>  	"btrfs filesystem [<group>] <command> [<args>]",
> @@ -877,7 +880,7 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
>  	if ((typeflag == FTW_F) && S_ISREG(sb->st_mode)) {
>  		if (defrag_global_verbose)
>  			printf("%s\n", fpath);
> -		fd = open(fpath, O_RDWR);
> +		fd = open(fpath, defrag_ro);
>  		if (fd < 0) {
>  			goto error;
>  		}
> @@ -914,6 +917,9 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  	int compress_type = BTRFS_COMPRESS_NONE;
>  	DIR *dirstream;
>  
> +	if (get_running_kernel_version() < KERNEL_VERSION(4,19,0) && getuid())
> +		defrag_ro = O_RDWR;
> +
>  	/*
>  	 * 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
> @@ -1014,7 +1020,7 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  		int defrag_err = 0;
>  
>  		dirstream = NULL;
> -		fd = open_file_or_dir(argv[i], &dirstream);
> +		fd = open_file_or_dir3(argv[i], &dirstream, defrag_ro);
>  		if (fd < 0) {
>  			error("cannot open %s: %m", argv[i]);
>  			ret = -errno;
> 

Re: [PATCH 2/2] btrfs-progs: defrag: open files RO on new enough kernels or if root

[Nikolay Borisov]

On  3.09.2018 13:14, Adam Borowski wrote:
> Fixes EXTXBSY races.
> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>
> ---
>  cmds-filesystem.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 06c8311b..4c9df69f 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -26,6 +26,7 @@
>  #include <ftw.h>
>  #include <mntent.h>
>  #include <linux/limits.h>
> +#include <linux/version.h>
>  #include <getopt.h>
>  
>  #include <btrfsutil.h>
> @@ -39,12 +40,14 @@
>  #include "list_sort.h"
>  #include "disk-io.h"
>  #include "help.h"
> +#include "fsfeatures.h"
>  
>  /*
>   * for btrfs fi show, we maintain a hash of fsids we've already printed.
>   * This way we don't print dups if a given FS is mounted more than once.
>   */
>  static struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {NULL,};
> +static mode_t defrag_ro = O_RDONLY;
>  
>  static const char * const filesystem_cmd_group_usage[] = {
>  	"btrfs filesystem [<group>] <command> [<args>]",
> @@ -877,7 +880,7 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
>  	if ((typeflag == FTW_F) && S_ISREG(sb->st_mode)) {
>  		if (defrag_global_verbose)
>  			printf("%s\n", fpath);
> -		fd = open(fpath, O_RDWR);
> +		fd = open(fpath, defrag_ro);

Looking at the kernel code I think this is in fact incorrect, because in
ioctl.c we have:

                if (!(file->f_mode & FMODE_WRITE)) {

                        ret = -EINVAL;

                        goto out;

                }

So it seems a hard requirement to have opened a file for RW when you
want to defragment it.

>  		if (fd < 0) {
>  			goto error;
>  		}
> @@ -914,6 +917,9 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  	int compress_type = BTRFS_COMPRESS_NONE;
>  	DIR *dirstream;
>  
> +	if (get_running_kernel_version() < KERNEL_VERSION(4,19,0) && getuid())
> +		defrag_ro = O_RDWR;
> +
>  	/*
>  	 * 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
> @@ -1014,7 +1020,7 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  		int defrag_err = 0;
>  
>  		dirstream = NULL;
> -		fd = open_file_or_dir(argv[i], &dirstream);
> +		fd = open_file_or_dir3(argv[i], &dirstream, defrag_ro);
>  		if (fd < 0) {
>  			error("cannot open %s: %m", argv[i]);
>  			ret = -errno;
> 

Re: [PATCH 2/2] btrfs-progs: defrag: open files RO on new enough kernels or if root

[Adam Borowski]

On Mon, Sep 03, 2018 at 02:01:21PM +0300, Nikolay Borisov wrote:
> On  3.09.2018 13:14, Adam Borowski wrote:
> > Fixes EXTXBSY races.
> 
> You have to be more eloquent than that and explain at least one race
> condition.

If you try to defrag an executable that's currently running:

ERROR: cannot open XXX: Text file busy
total 1 failures

If you try to run an executable that's being defragged:

-bash: XXX: Text file busy

The former tends to be a long-lasting condition but has only benign fallout
(executables almost never get fragmented, not recompressing a single file is
not the end of the world), the latter is only a brief window of time but has
potential for data loss.

> > +static mode_t defrag_ro = O_RDONLY;
> 
> This brings no value whatsoever, just use O_RDONLY directly

On old kernels it gets overwritten with:

> > +	if (get_running_kernel_version() < KERNEL_VERSION(4,19,0) && getuid())
> > +		defrag_ro = O_RDWR;


Meow!
-- 
⢀⣴⠾⠻⢶⣦⠀ What Would Jesus Do, MUD/MMORPG edition:
⣾⠁⢰⠒⠀⣿⡁ • multiplay with an admin char to benefit your mortal [Mt3:16-17]
⢿⡄⠘⠷⠚⠋⠀ • abuse item cloning bugs [Mt14:17-20, Mt15:34-37]
⠈⠳⣄⠀⠀⠀⠀ • use glitches to walk on water [Mt14:25-26]

Re: [PATCH 2/2] btrfs-progs: defrag: open files RO on new enough kernels or if root

[Adam Borowski]

On Mon, Sep 03, 2018 at 02:04:23PM +0300, Nikolay Borisov wrote:
> On  3.09.2018 13:14, Adam Borowski wrote:
> > -		fd = open(fpath, O_RDWR);
> > +		fd = open(fpath, defrag_ro);
> 
> Looking at the kernel code I think this is in fact incorrect, because in
> ioctl.c we have:
> 
>                 if (!(file->f_mode & FMODE_WRITE)) {
> 
>                         ret = -EINVAL;
> 
>                         goto out;
> 
>                 }
> 
> So it seems a hard requirement to have opened a file for RW when you
> want to defragment it.

Oif!  I confused this with dedup, which does allow root to dedup RO even on
old kernels.  Good catch.


-- 
⢀⣴⠾⠻⢶⣦⠀ What Would Jesus Do, MUD/MMORPG edition:
⣾⠁⢰⠒⠀⣿⡁ • multiplay with an admin char to benefit your mortal [Mt3:16-17]
⢿⡄⠘⠷⠚⠋⠀ • abuse item cloning bugs [Mt14:17-20, Mt15:34-37]
⠈⠳⣄⠀⠀⠀⠀ • use glitches to walk on water [Mt14:25-26]

[PATCH v2] btrfs-progs: defrag: open files RO on new enough kernels

[Adam Borowski]

Defragging an executable conflicts both way with it being run, resulting in
ETXTBSY.  This either makes defrag fail or prevents the program from being
executed.

Kernels 4.19-rc1 and later allow defragging files you could have possibly
opened rw, even if the passed descriptor is ro.

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
v2: more eloquent description; root can't defrag RO on old kernels (unlike
dedupe)


 cmds-filesystem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 06c8311b..17e992a3 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -26,6 +26,7 @@
 #include <ftw.h>
 #include <mntent.h>
 #include <linux/limits.h>
+#include <linux/version.h>
 #include <getopt.h>
 
 #include <btrfsutil.h>
@@ -39,12 +40,14 @@
 #include "list_sort.h"
 #include "disk-io.h"
 #include "help.h"
+#include "fsfeatures.h"
 
 /*
  * for btrfs fi show, we maintain a hash of fsids we've already printed.
  * This way we don't print dups if a given FS is mounted more than once.
  */
 static struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {NULL,};
+static mode_t defrag_ro = O_RDONLY;
 
 static const char * const filesystem_cmd_group_usage[] = {
 	"btrfs filesystem [<group>] <command> [<args>]",
@@ -877,7 +880,7 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
 	if ((typeflag == FTW_F) && S_ISREG(sb->st_mode)) {
 		if (defrag_global_verbose)
 			printf("%s\n", fpath);
-		fd = open(fpath, O_RDWR);
+		fd = open(fpath, defrag_ro);
 		if (fd < 0) {
 			goto error;
 		}
@@ -914,6 +917,9 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 	int compress_type = BTRFS_COMPRESS_NONE;
 	DIR *dirstream;
 
+	if (get_running_kernel_version() < KERNEL_VERSION(4,19,0))
+		defrag_ro = O_RDWR;
+
 	/*
 	 * 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
@@ -1014,7 +1020,7 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 		int defrag_err = 0;
 
 		dirstream = NULL;
-		fd = open_file_or_dir(argv[i], &dirstream);
+		fd = open_file_or_dir3(argv[i], &dirstream, defrag_ro);
 		if (fd < 0) {
 			error("cannot open %s: %m", argv[i]);
 			ret = -errno;
-- 
2.19.0.rc1

Re: [PATCH v2] btrfs-progs: defrag: open files RO on new enough kernels

[Nikolay Borisov]

On  3.09.2018 14:31, Adam Borowski wrote:
> Defragging an executable conflicts both way with it being run, resulting in
> ETXTBSY.  This either makes defrag fail or prevents the program from being
> executed.
> 
> Kernels 4.19-rc1 and later allow defragging files you could have possibly
> opened rw, even if the passed descriptor is ro.
> 
> Signed-off-by: Adam Borowski <kilobyte@angband.pl>

So this commit really seems to be the userspace counterpart of
616d374efa23 ("btrfs: allow defrag on a file opened read-only that has
rw permissions") as such IMO it will be good if you reference it.


> ---
> v2: more eloquent description; root can't defrag RO on old kernels (unlike
> dedupe)
> 
> 
>  cmds-filesystem.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 06c8311b..17e992a3 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -26,6 +26,7 @@
>  #include <ftw.h>
>  #include <mntent.h>
>  #include <linux/limits.h>
> +#include <linux/version.h>
>  #include <getopt.h>
>  
>  #include <btrfsutil.h>
> @@ -39,12 +40,14 @@
>  #include "list_sort.h"
>  #include "disk-io.h"
>  #include "help.h"
> +#include "fsfeatures.h"
>  
>  /*
>   * for btrfs fi show, we maintain a hash of fsids we've already printed.
>   * This way we don't print dups if a given FS is mounted more than once.
>   */
>  static struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {NULL,};
> +static mode_t defrag_ro = O_RDONLY;
>  
>  static const char * const filesystem_cmd_group_usage[] = {
>  	"btrfs filesystem [<group>] <command> [<args>]",
> @@ -877,7 +880,7 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
>  	if ((typeflag == FTW_F) && S_ISREG(sb->st_mode)) {
>  		if (defrag_global_verbose)
>  			printf("%s\n", fpath);
> -		fd = open(fpath, O_RDWR);
> +		fd = open(fpath, defrag_ro);
>  		if (fd < 0) {
>  			goto error;
>  		}
> @@ -914,6 +917,9 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  	int compress_type = BTRFS_COMPRESS_NONE;
>  	DIR *dirstream;
>  
> +	if (get_running_kernel_version() < KERNEL_VERSION(4,19,0))
> +		defrag_ro = O_RDWR;

I completely missed those lines in the previous posting, so alongside
the context information of the kernel commit this makes sense. However,
defrag_ro is a really bad name because there are case where, well, it's
not going to be ro, is it ? How about something like "defrag_open_mode"
or "defrag_open_flags" or something neutral which won't be implying the
mode of operation.

> +
>  	/*
>  	 * 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
> @@ -1014,7 +1020,7 @@ static int cmd_filesystem_defrag(int argc, char **argv)
>  		int defrag_err = 0;
>  
>  		dirstream = NULL;
> -		fd = open_file_or_dir(argv[i], &dirstream);
> +		fd = open_file_or_dir3(argv[i], &dirstream, defrag_ro);
>  		if (fd < 0) {
>  			error("cannot open %s: %m", argv[i]);
>  			ret = -errno;
> 

[PATCH v3] btrfs-progs: defrag: open files RO on new enough kernels

[Adam Borowski]

Defragging an executable conflicts both way with it being run, resulting in
ETXTBSY.  This either makes defrag fail or prevents the program from being
executed.

Kernels 4.19-rc1 and later allow defragging files you could have possibly
opened rw, even if the passed descriptor is ro (commit 616d374efa23).

Signed-off-by: Adam Borowski <kilobyte@angband.pl>
---
v2: more eloquent description; root can't defrag RO on old kernels (unlike
dedupe)
v3: more eloquentier description; s/defrag_ro/defrag_open_mode/

 cmds-filesystem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 06c8311b..99e2aec0 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -26,6 +26,7 @@
 #include <ftw.h>
 #include <mntent.h>
 #include <linux/limits.h>
+#include <linux/version.h>
 #include <getopt.h>
 
 #include <btrfsutil.h>
@@ -39,12 +40,14 @@
 #include "list_sort.h"
 #include "disk-io.h"
 #include "help.h"
+#include "fsfeatures.h"
 
 /*
  * for btrfs fi show, we maintain a hash of fsids we've already printed.
  * This way we don't print dups if a given FS is mounted more than once.
  */
 static struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {NULL,};
+static mode_t defrag_open_mode = O_RDONLY;
 
 static const char * const filesystem_cmd_group_usage[] = {
 	"btrfs filesystem [<group>] <command> [<args>]",
@@ -877,7 +880,7 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
 	if ((typeflag == FTW_F) && S_ISREG(sb->st_mode)) {
 		if (defrag_global_verbose)
 			printf("%s\n", fpath);
-		fd = open(fpath, O_RDWR);
+		fd = open(fpath, defrag_open_mode);
 		if (fd < 0) {
 			goto error;
 		}
@@ -914,6 +917,9 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 	int compress_type = BTRFS_COMPRESS_NONE;
 	DIR *dirstream;
 
+	if (get_running_kernel_version() < KERNEL_VERSION(4,19,0))
+		defrag_open_mode = O_RDWR;
+
 	/*
 	 * 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
@@ -1014,7 +1020,7 @@ static int cmd_filesystem_defrag(int argc, char **argv)
 		int defrag_err = 0;
 
 		dirstream = NULL;
-		fd = open_file_or_dir(argv[i], &dirstream);
+		fd = open_file_or_dir3(argv[i], &dirstream, defrag_open_mode);
 		if (fd < 0) {
 			error("cannot open %s: %m", argv[i]);
 			ret = -errno;
-- 
2.19.0.rc1