| Message ID | 20160621151659.32157-1-ahferroin7@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 2016/06/22 0:16, Austin S. Hemmelgarn wrote: > Currently, balance operations are run synchronously in the foreground. > This is nice for interactive management, but is kind of crappy when you > start looking at automation and similar things. > > This patch adds an option to `btrfs balance start` to tell it to > daemonize prior to running the balance operation, thus allowing us to > preform balances asynchronously. The two biggest use cases I have for > this are starting a balance on a remote server without establishing a > full shell session, and being able to background the balance in a > recovery shell (which usually has no job control) so I can still get > progress information. > > Because it simply daemonizes prior to calling the balance ioctl, this > doesn't actually need any kernel support. > > Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com> > --- > This works as is, but there are two specific things I would love to > eventually fix but don't have the time to fix right now: > * There is no way to get any feedback from the balance operation. > * Because of how everything works, trying to start a new balance with > --background while one iw already running won't return an error but > won't queue or start a new balance either. > > The first one is more a utility item than anything else, and probably > would not be hard to add. Ideally, it should be output to a user > specified file, and this should work even for a normal foreground balance. > > The second is very much a UX issue, but can't be easily sovled without > doing some creative process monitoring from the parrent processes. > > Documentation/btrfs-balance.asciidoc | 2 ++ > cmds-balance.c | 43 +++++++++++++++++++++++++++++++++++- > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/Documentation/btrfs-balance.asciidoc b/Documentation/btrfs-balance.asciidoc > index 7df40b9..f487dbb 100644 > --- a/Documentation/btrfs-balance.asciidoc > +++ b/Documentation/btrfs-balance.asciidoc > @@ -85,6 +85,8 @@ act on system chunks (requires '-f'), see `FILTERS` section for details about 'f > be verbose and print balance filter arguments > -f:::: > force reducing of metadata integrity, eg. when going from 'raid1' to 'single' > +--background:::: > +run the balance operation asynchronously in the background > > *status* [-v] <path>:: > Show status of running or paused balance. > diff --git a/cmds-balance.c b/cmds-balance.c > index 708bbf4..66169b7 100644 > --- a/cmds-balance.c > +++ b/cmds-balance.c > @@ -20,6 +20,9 @@ > #include <unistd.h> > #include <getopt.h> > #include <sys/ioctl.h> > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <fcntl.h> > #include <errno.h> > > #include "kerncompat.h" > @@ -510,6 +513,7 @@ static const char * const cmd_balance_start_usage[] = { > "-v be verbose", > "-f force reducing of metadata integrity", > "--full-balance do not print warning and do not delay start", > + "--background run the balance as a background process", > NULL > }; > > @@ -520,6 +524,7 @@ static int cmd_balance_start(int argc, char **argv) > &args.meta, NULL }; > int force = 0; > int verbose = 0; > + int background = 0; > unsigned start_flags = 0; > int i; > > @@ -527,7 +532,8 @@ static int cmd_balance_start(int argc, char **argv) > > optind = 1; > while (1) { > - enum { GETOPT_VAL_FULL_BALANCE = 256 }; > + enum { GETOPT_VAL_FULL_BALANCE = 256, > + GETOPT_VAL_BACKGROUND = 257 }; > static const struct option longopts[] = { > { "data", optional_argument, NULL, 'd'}, > { "metadata", optional_argument, NULL, 'm' }, > @@ -536,6 +542,8 @@ static int cmd_balance_start(int argc, char **argv) > { "verbose", no_argument, NULL, 'v' }, > { "full-balance", no_argument, NULL, > GETOPT_VAL_FULL_BALANCE }, > + { "background", no_argument, NULL, > + GETOPT_VAL_BACKGROUND }, > { NULL, 0, NULL, 0 } > }; > > @@ -574,6 +582,9 @@ static int cmd_balance_start(int argc, char **argv) > case GETOPT_VAL_FULL_BALANCE: > start_flags |= BALANCE_START_NOWARN; > break; > + case GETOPT_VAL_BACKGROUND: > + background = 1; > + break; > default: > usage(cmd_balance_start_usage); > } > @@ -626,6 +637,36 @@ static int cmd_balance_start(int argc, char **argv) > args.flags |= BTRFS_BALANCE_FORCE; > if (verbose) > dump_ioctl_balance_args(&args); > + if (background) { > + switch (fork()) { > + case (-1): > + error("Unable to fork to run balance in background"); > + return 1; > + break; > + case (0): > + setsid(); > + switch(fork()) { > + case (-1): > + error("Unable to fork to run balance in background"); > + exit(1); > + break; > + case (0): > + chdir("/"); You should check the return value of chdir(). Otherwise we get the following warning message at the build time. ================================= cmds-balance.c: In function 'cmd_balance_start': cmds-balance.c:654:6: warning: ignoring return value of 'chdir', declared with attribute warn_unused_result [-Wunused-result] chdir("/"); ^ ================================= I found this warning message at integration-20160704(2355a7e5dcdf122d1924). Thanks, Satoru > + close(0); > + close(1); > + close(2); > + open("/dev/null", O_RDONLY); > + open("/dev/null", O_WRONLY); > + open("/dev/null", O_WRONLY); > + break; > + default: > + exit(0); > + } > + break; > + default: > + exit(0); > + } > + } > > return do_balance(argv[optind], &args, start_flags); > } > -- 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
On Tue, Jun 21, 2016 at 11:16:59AM -0400, Austin S. Hemmelgarn wrote: > Currently, balance operations are run synchronously in the foreground. > This is nice for interactive management, but is kind of crappy when you > start looking at automation and similar things. It can be done with simplest systemd unit file: btrfs-balance@.service: --- [Unit] Description=btrfs balance for %I [Service] ExecStart=/usr/bin/btrfs balance start %I ExecStop=/usr/bin/btrfs balance cancel %I --- It automates quite nicely and needs no additional code.
On 2016-07-11 03:26, Tomasz Torcz wrote: > On Tue, Jun 21, 2016 at 11:16:59AM -0400, Austin S. Hemmelgarn wrote: >> Currently, balance operations are run synchronously in the foreground. >> This is nice for interactive management, but is kind of crappy when you >> start looking at automation and similar things. > > It can be done with simplest systemd unit file: > btrfs-balance@.service: > --- > [Unit] > Description=btrfs balance for %I > > [Service] > ExecStart=/usr/bin/btrfs balance start %I > ExecStop=/usr/bin/btrfs balance cancel %I > --- > > It automates quite nicely and needs no additional code. > It's also entirely dependent on a couple of things: 1. You're running systemd (not everyone is, I'm certainly not). 2. You're only dealing with the local system. The type of situation I'm thinking of is dealing with non-local systems. For example, running something like this: ssh user@remotehost btrfs balance start --background / Keeping the SSH connection open for the duration of the balance has issues for some people (may close without keep-alive set, uses network bandwidth with keep-alive set, many people who are hosted have bandwidth quotas still), and it's extremely useful to have the option to fire and forget. -- 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
On Mon, Jul 11, 2016 at 07:17:28AM -0400, Austin S. Hemmelgarn wrote: > On 2016-07-11 03:26, Tomasz Torcz wrote: > > On Tue, Jun 21, 2016 at 11:16:59AM -0400, Austin S. Hemmelgarn wrote: > > > Currently, balance operations are run synchronously in the foreground. > > > This is nice for interactive management, but is kind of crappy when you > > > start looking at automation and similar things. > > > > It can be done with simplest systemd unit file: > > btrfs-balance@.service: > > --- > > [Unit] > > Description=btrfs balance for %I > > > > [Service] > > ExecStart=/usr/bin/btrfs balance start %I > > ExecStop=/usr/bin/btrfs balance cancel %I > > --- > > > > It automates quite nicely and needs no additional code. > > > It's also entirely dependent on a couple of things: > 1. You're running systemd (not everyone is, I'm certainly not). So instead of using widespread, tested code, you re-implement parts of it. BTW, your patch for daemonizing does only 5 steps out of 15 described in man 7 daemon. > 2. You're only dealing with the local system. > > The type of situation I'm thinking of is dealing with non-local systems. > For example, running something like this: > ssh user@remotehost btrfs balance start --background / > Keeping the SSH connection open for the duration of the balance has issues > for some people (may close without keep-alive set, uses network bandwidth > with keep-alive set, many people who are hosted have bandwidth quotas > still), and it's extremely useful to have the option to fire and forget. I don't get the local part. Right now, when using above unit you can ssh user@remotehost systemctl start btrfs-balance@- (or even systemctl -H user@remotehost start btrfs-balance@-) and balance for / runs in background on target host. With clean environment, logs being captured, locking against multiple startups and so on. Right now, without any additional code.
On 2016-07-11 12:58, Tomasz Torcz wrote: > On Mon, Jul 11, 2016 at 07:17:28AM -0400, Austin S. Hemmelgarn wrote: >> On 2016-07-11 03:26, Tomasz Torcz wrote: >>> On Tue, Jun 21, 2016 at 11:16:59AM -0400, Austin S. Hemmelgarn wrote: >>>> Currently, balance operations are run synchronously in the foreground. >>>> This is nice for interactive management, but is kind of crappy when you >>>> start looking at automation and similar things. >>> >>> It can be done with simplest systemd unit file: >>> btrfs-balance@.service: >>> --- >>> [Unit] >>> Description=btrfs balance for %I >>> >>> [Service] >>> ExecStart=/usr/bin/btrfs balance start %I >>> ExecStop=/usr/bin/btrfs balance cancel %I >>> --- >>> >>> It automates quite nicely and needs no additional code. >>> >> It's also entirely dependent on a couple of things: >> 1. You're running systemd (not everyone is, I'm certainly not). > > So instead of using widespread, tested code, you re-implement > parts of it. BTW, your patch for daemonizing does only 5 steps > out of 15 described in man 7 daemon. Sysvinit is more widely tested and used than systemd has ever been. Systemd's been around for only a few years, sysvinit has been around for decades. I'm not going to add a dependency on something that's just popular for the moment, especially when that thing is such a point of contention for so many people. As far as daemonization, I have no man-page called daemon in section seven, yet I have an up-to-date upstream copy of the Linux man pages. My guess is that this is a systemd man page, which in turn means it does a bunch of stuff that's only needed for system services on systemd based systems. The method I used, in a slightly different form (most people use if instead of switch), is pretty much identical to what's been done since before SVR4 for daemonization. In theory, I could use libdaemon, but that adds a new dependency. In theory I could use the daemon() function from unistd.h, but the implementation in glibc doesn't work correctly (it only forks once). This doesn't do special handling for systemd monitoring, because it doesn't need to. It's not a system service, it's just a background process. > >> 2. You're only dealing with the local system. >> >> The type of situation I'm thinking of is dealing with non-local systems. >> For example, running something like this: >> ssh user@remotehost btrfs balance start --background / >> Keeping the SSH connection open for the duration of the balance has issues >> for some people (may close without keep-alive set, uses network bandwidth >> with keep-alive set, many people who are hosted have bandwidth quotas >> still), and it's extremely useful to have the option to fire and forget. > > I don't get the local part. Right now, when using above unit you can > > ssh user@remotehost systemctl start btrfs-balance@- > > (or even > systemctl -H user@remotehost start btrfs-balance@-) > > and balance for / runs in background on target host. With clean > environment, logs being captured, locking against multiple > startups and so on. Right now, without any additional code. **************************************** *EXCEPT THAT NOT EVERYONE USES SYSTEMD!* **************************************** Why do you fail to get that simple fact? I'm not changing my init system just to add functionality that should already exist in btrfs-progs. The fact that the balance ioctl is synchronous was a poor design choice, and we need to provide the option to work around that independent of what our users are running. There's been enough interest from other people that it should be obvious that people want this _in_ btrfs-progs. The point is to not _need_ anything else to do this. -- 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
Austin S. Hemmelgarn posted on Tue, 12 Jul 2016 08:25:24 -0400 as excerpted: > As far as daemonization, I have no man-page called daemon in section > seven, yet I have an up-to-date upstream copy of the Linux man pages. My > guess is that this is a systemd man page, which in turn means it does a > bunch of stuff that's only needed for system services on systemd based > systems. Your guess would appear to be correct, as my systemd package includes a daemon manpage, here (gentoo). As for the general debate, while I'm a systemd user, I definitely support it remaining optional, which means things need to work without it, too.
On 2016-07-12 11:22, Duncan wrote: > Austin S. Hemmelgarn posted on Tue, 12 Jul 2016 08:25:24 -0400 as > excerpted: > >> As far as daemonization, I have no man-page called daemon in section >> seven, yet I have an up-to-date upstream copy of the Linux man pages. My >> guess is that this is a systemd man page, which in turn means it does a >> bunch of stuff that's only needed for system services on systemd based >> systems. > > Your guess would appear to be correct, as my systemd package includes a > daemon manpage, here (gentoo). OK, in that case, what that page will list is the requirements for a daemon which runs under systemd. That's _way_ more than what's needed for a regular daemon, and IIRC, some of the things systemd requires will cause issues on non-systemd systems (for those who use Gentoo or build their own software, this is why so many things have systemd support optional, many things haven't been written to choose at runtime). For reference, the de-fact standard method of daemonization on UNIX is generally: 1. Perform any config parsing and similar and exit if it fails. 2. fork() 3. setsid() in the child process 4. fork() in the child process 5. chdir("/") in the new child process (this shouldn't need to have it's return code checked, if it fails, you almost certainly have bigger issues to deal with). 6. Close stdin, stdout, and stderr in the new child process 7. Open stdin, stdout, and stderr to whatever you want (usually /dev/null unless logging is being done). 8. Optionally provide verification from the first child that the second child is still running, then exit the first child process. 9. Optionally provide verification from the parent process that the first child exited with out indicating errors, then exit. Item 1 is to allow for verbose error info to be returned if something that can be done in the foreground fails. Items 2 and 3 set up a session leader process which would get re-parented to init if it's parent exited. Item 4 makes sure the daemon isn't a session leader, which means it will never have a controlling terminal again (this is the bit that the glibc implementation of daemon(3) doesn't do). Item 5 frees up the previous working directory so that it isn't referenced by the daemon directly, and thus can be unmounted or removed (this isn't strictly necessary, but it's considered good practice). Item 6 ensures that the daemon has no controlling terminal (so it doesn't get hit by SIGHUP or similar things accidentally). Item 7 is not 100% necessary, but is also considered good practice (especially if the code is only optionally run as a daemon). Items 8 and 9 are generally considered good practice, but are less commonly used in simple daemons than other options. As stated in a previous e-mail, there are two rather obvious options which I chose not to use here: 1. libdaemon 2. daemon(3) Additionally, libbsd also has a daemon() function. I chose to not use daemon(3) because it has issues as implemented by glibc (it skips step 4 above, which has potentially problematic implications). I chose not to use libdaemon or libbsd because those would add another dependency, which is not a good thing for a low-level maintenance tool. As far as this particular implementation I did, I've got items 2-7. Item 1 is equivalent to command-line parsing, which is mostly done at this point. I plan to do items 8 and 9, but don't really have the time to code them right now. > > As for the general debate, while I'm a systemd user, I definitely support > it remaining optional, which means things need to work without it, too. I'm glad to hear some one else feels this way, although I'm not surprised it's another Gentoo user :). -- 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
12.07.2016 15:25, Austin S. Hemmelgarn пишет: > > I'm not changing my init system just to add functionality that should > already exist in btrfs-progs. The fact that the balance ioctl is > synchronous was a poor design choice, and we need to provide the option > to work around that independent of what our users are running. There's > been enough interest from other people that it should be obvious that > people want this _in_ btrfs-progs. The point is to not _need_ anything > else to do this. May be I miss something obvious, but what is wrong with nohup btrfs balance start & nohup is definitely available on every system and so far I have been using it in exactly such cases when I needed to leave something running for a long time. -- 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
On 2016-07-13 00:39, Andrei Borzenkov wrote: > 12.07.2016 15:25, Austin S. Hemmelgarn пишет: >> >> I'm not changing my init system just to add functionality that should >> already exist in btrfs-progs. The fact that the balance ioctl is >> synchronous was a poor design choice, and we need to provide the option >> to work around that independent of what our users are running. There's >> been enough interest from other people that it should be obvious that >> people want this _in_ btrfs-progs. The point is to not _need_ anything >> else to do this. > > May be I miss something obvious, but what is wrong with > > nohup btrfs balance start & > > nohup is definitely available on every system and so far I have been > using it in exactly such cases when I needed to leave something running > for a long time. > That does work, but it still falls into the category of needing external software to do it. I'm trying to get something that works without any external software, and doesn't even depend on having a shell with job control. -- 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
On Tue, Jun 21, 2016 at 11:16:59AM -0400, Austin S. Hemmelgarn wrote: > Currently, balance operations are run synchronously in the foreground. > This is nice for interactive management, but is kind of crappy when you > start looking at automation and similar things. Yeah, people have been complaining about the lack of backgrounding support over the time. > This patch adds an option to `btrfs balance start` to tell it to > daemonize prior to running the balance operation, thus allowing us to > preform balances asynchronously. The two biggest use cases I have for > this are starting a balance on a remote server without establishing a > full shell session, and being able to background the balance in a > recovery shell (which usually has no job control) so I can still get > progress information. > > Because it simply daemonizes prior to calling the balance ioctl, this > doesn't actually need any kernel support. We could also add the kernel support, but this would need to extend the ioctl flags. Unfortunatelly older kernels would ignore it and always let balance run in foreground (due to lack of checks for the known flags). So at the moment forking a process seems to be an option. > Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com> > --- > This works as is, but there are two specific things I would love to > eventually fix but don't have the time to fix right now: > * There is no way to get any feedback from the balance operation. Does this mean that 'btrfs balance status' is not sufficient? Or I don't understand what you mean. > * Because of how everything works, trying to start a new balance with > --background while one iw already running won't return an error but > won't queue or start a new balance either. > > The first one is more a utility item than anything else, and probably > would not be hard to add. Ideally, it should be output to a user > specified file, and this should work even for a normal foreground balance. > > The second is very much a UX issue, but can't be easily sovled without > doing some creative process monitoring from the parrent processes. Currently, starting a second balance will return immediatelly with "in progress" message, this is returned by the balance iotctl itself. In this case it would be the child process and communicating it back would need a pipe or somesuch. But, the parent can check the balance status by itself, before calling fork, right? Unless I'm missing something, this should address your concerns. -- 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
On 2016-07-13 12:38, David Sterba wrote: > On Tue, Jun 21, 2016 at 11:16:59AM -0400, Austin S. Hemmelgarn wrote: >> Currently, balance operations are run synchronously in the foreground. >> This is nice for interactive management, but is kind of crappy when you >> start looking at automation and similar things. > > Yeah, people have been complaining about the lack of backgrounding > support over the time. TBH, I actually understand why it was implemented the way it was in the first place, it's a lot easier to work with something running in the foreground, and having the ability to run in the background adds complexity, so blocking like it does currently makes sense for an initial implementation. Beyond the initial implementation, I don't think it's bothered anyone enough that they wanted to fix it themselves. > >> This patch adds an option to `btrfs balance start` to tell it to >> daemonize prior to running the balance operation, thus allowing us to >> preform balances asynchronously. The two biggest use cases I have for >> this are starting a balance on a remote server without establishing a >> full shell session, and being able to background the balance in a >> recovery shell (which usually has no job control) so I can still get >> progress information. >> >> Because it simply daemonizes prior to calling the balance ioctl, this >> doesn't actually need any kernel support. > > We could also add the kernel support, but this would need to extend the > ioctl flags. Unfortunatelly older kernels would ignore it and always let > balance run in foreground (due to lack of checks for the known flags). > So at the moment forking a process seems to be an option. Ideally, I'd like to see kernel support for this too so that tools that want to manage it don't need to spawn a thread or fork a new process just to run it in the background. That said, it's a lot quicker to implement in userspace, and we'd need such an implementation anyway to support old kernels. > >> Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com> >> --- >> This works as is, but there are two specific things I would love to >> eventually fix but don't have the time to fix right now: >> * There is no way to get any feedback from the balance operation. > > Does this mean that 'btrfs balance status' is not sufficient? Or I don't > understand what you mean. `btrfs balance status` calls still work. What I was referring to is the lack of the usual 'Done, had to relocate X out of Y chunks.' output on completion. I probably should have worded this differently in retrospect. As mentioned below though, in an ideal situation, this would have the ability to log the results, but doing that the right way is more code than I'm willing to deal with for a first pass at this. > >> * Because of how everything works, trying to start a new balance with >> --background while one iw already running won't return an error but >> won't queue or start a new balance either. >> >> The first one is more a utility item than anything else, and probably >> would not be hard to add. Ideally, it should be output to a user >> specified file, and this should work even for a normal foreground balance. >> >> The second is very much a UX issue, but can't be easily sovled without >> doing some creative process monitoring from the parrent processes. > > Currently, starting a second balance will return immediatelly with "in > progress" message, this is returned by the balance iotctl itself. In > this case it would be the child process and communicating it back would > need a pipe or somesuch. But, the parent can check the balance status by > itself, before calling fork, right? Unless I'm missing something, this > should address your concerns. What I had been thinking was just having the parent wait and see that the child is still running after a short time (100ms maybe), or has exited with a 0 exit status. I'm hesitant to add an extra ioctl just to check if a balance is running because of TOCTOU races, I'd rather have it deterministically start the balance or not based solely on the balance ioctl, especially since fork() is expensive enough to make such a race rather easy to hit). Hopefully, I'll have some time in the near future to do an updated version that properly handles this. -- 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
On Mon, Jul 11, 2016 at 10:44:30AM +0900, Satoru Takeuchi wrote: > > + chdir("/"); > > You should check the return value of chdir(). Otherwise > we get the following warning message at the build time. Can we actually fail to change directory to '/' ? Otherwise I think the warning should be silenced so the build is clean. -- 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
On 2016-07-26 13:07, David Sterba wrote: > On Mon, Jul 11, 2016 at 10:44:30AM +0900, Satoru Takeuchi wrote: >>> + chdir("/"); >> >> You should check the return value of chdir(). Otherwise >> we get the following warning message at the build time. > > Can we actually fail to change directory to '/' ? Otherwise I think the > warning should be silenced so the build is clean. It is theoretically possible, but in practice it means you either have failing hardware or your kernel is broken somehow, both of which should be causing all kinds of other issues, and thus we don't really gain anything from trying to catch the error. -- 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
diff --git a/Documentation/btrfs-balance.asciidoc b/Documentation/btrfs-balance.asciidoc index 7df40b9..f487dbb 100644 --- a/Documentation/btrfs-balance.asciidoc +++ b/Documentation/btrfs-balance.asciidoc @@ -85,6 +85,8 @@ act on system chunks (requires '-f'), see `FILTERS` section for details about 'f be verbose and print balance filter arguments -f:::: force reducing of metadata integrity, eg. when going from 'raid1' to 'single' +--background:::: +run the balance operation asynchronously in the background *status* [-v] <path>:: Show status of running or paused balance. diff --git a/cmds-balance.c b/cmds-balance.c index 708bbf4..66169b7 100644 --- a/cmds-balance.c +++ b/cmds-balance.c @@ -20,6 +20,9 @@ #include <unistd.h> #include <getopt.h> #include <sys/ioctl.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> #include <errno.h> #include "kerncompat.h" @@ -510,6 +513,7 @@ static const char * const cmd_balance_start_usage[] = { "-v be verbose", "-f force reducing of metadata integrity", "--full-balance do not print warning and do not delay start", + "--background run the balance as a background process", NULL }; @@ -520,6 +524,7 @@ static int cmd_balance_start(int argc, char **argv) &args.meta, NULL }; int force = 0; int verbose = 0; + int background = 0; unsigned start_flags = 0; int i; @@ -527,7 +532,8 @@ static int cmd_balance_start(int argc, char **argv) optind = 1; while (1) { - enum { GETOPT_VAL_FULL_BALANCE = 256 }; + enum { GETOPT_VAL_FULL_BALANCE = 256, + GETOPT_VAL_BACKGROUND = 257 }; static const struct option longopts[] = { { "data", optional_argument, NULL, 'd'}, { "metadata", optional_argument, NULL, 'm' }, @@ -536,6 +542,8 @@ static int cmd_balance_start(int argc, char **argv) { "verbose", no_argument, NULL, 'v' }, { "full-balance", no_argument, NULL, GETOPT_VAL_FULL_BALANCE }, + { "background", no_argument, NULL, + GETOPT_VAL_BACKGROUND }, { NULL, 0, NULL, 0 } }; @@ -574,6 +582,9 @@ static int cmd_balance_start(int argc, char **argv) case GETOPT_VAL_FULL_BALANCE: start_flags |= BALANCE_START_NOWARN; break; + case GETOPT_VAL_BACKGROUND: + background = 1; + break; default: usage(cmd_balance_start_usage); } @@ -626,6 +637,36 @@ static int cmd_balance_start(int argc, char **argv) args.flags |= BTRFS_BALANCE_FORCE; if (verbose) dump_ioctl_balance_args(&args); + if (background) { + switch (fork()) { + case (-1): + error("Unable to fork to run balance in background"); + return 1; + break; + case (0): + setsid(); + switch(fork()) { + case (-1): + error("Unable to fork to run balance in background"); + exit(1); + break; + case (0): + chdir("/"); + close(0); + close(1); + close(2); + open("/dev/null", O_RDONLY); + open("/dev/null", O_WRONLY); + open("/dev/null", O_WRONLY); + break; + default: + exit(0); + } + break; + default: + exit(0); + } + } return do_balance(argv[optind], &args, start_flags); }
Currently, balance operations are run synchronously in the foreground. This is nice for interactive management, but is kind of crappy when you start looking at automation and similar things. This patch adds an option to `btrfs balance start` to tell it to daemonize prior to running the balance operation, thus allowing us to preform balances asynchronously. The two biggest use cases I have for this are starting a balance on a remote server without establishing a full shell session, and being able to background the balance in a recovery shell (which usually has no job control) so I can still get progress information. Because it simply daemonizes prior to calling the balance ioctl, this doesn't actually need any kernel support. Signed-off-by: Austin S. Hemmelgarn <ahferroin7@gmail.com> --- This works as is, but there are two specific things I would love to eventually fix but don't have the time to fix right now: * There is no way to get any feedback from the balance operation. * Because of how everything works, trying to start a new balance with --background while one iw already running won't return an error but won't queue or start a new balance either. The first one is more a utility item than anything else, and probably would not be hard to add. Ideally, it should be output to a user specified file, and this should work even for a normal foreground balance. The second is very much a UX issue, but can't be easily sovled without doing some creative process monitoring from the parrent processes. Documentation/btrfs-balance.asciidoc | 2 ++ cmds-balance.c | 43 +++++++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-)