Re: [PATCH 2/2] btrfs-progs: defrag: open files RO on new enough kernels or if root
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]
Re: [PATCH 2/2] btrfs-progs: defrag: open files RO on new enough kernels or if root
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
On 3.09.2018 13:14, Adam Borowski wrote:
> Fixes EXTXBSY races.
>
> Signed-off-by: Adam Borowski
> ---
> 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
> #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);
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], );
> + fd = open_file_or_dir3(argv[i], , 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
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
> ---
> 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
> #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;
This brings no value whatsoever, just use O_RDONLY directly
>
> 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) && 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], );
> + fd = open_file_or_dir3(argv[i], , defrag_ro);
> if (fd < 0) {
> error("cannot open %s: %m", argv[i]);
> ret = -errno;
>
