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

2018-09-03 Thread 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 

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 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include 
> @@ -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 []  []",
> @@ -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], );
> + fd = open_file_or_dir3(argv[i], , defrag_ro);
>   if (fd < 0) {
>   error("cannot open %s: %m", argv[i]);
>   ret = -errno;
> 


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

2018-09-03 Thread 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 
---
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 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -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 []  []",
@@ -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], );
+   fd = open_file_or_dir3(argv[i], , defrag_ro);
if (fd < 0) {
error("cannot open %s: %m", argv[i]);
ret = -errno;
-- 
2.19.0.rc1