Re: [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately

Goldwyn Rodrigues
2020-09-02T20:45:23Z

On 16:11 02/09, David Sterba wrote:
> On Tue, Aug 25, 2020 at 10:03:38AM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > Wait for the current exclusive operation to finish before issuing the
> > command ioctl, so we have a better chance of success.
> >
> > Q: The resize argument parsing is hackish. Is there a better way to do
> > this?
>
> You mean parsing in kernel? Progs pass the 1st non-option parameter
> without changes, so if you add a new option, the -- separator needs to
> be used to make sure the relative size update (eg. -1G) is properly
> recognized. This is built in already and should not require anything
> special on the option parsing side.

Particularly the example you mention, -1G. You answered it as well :)

>
> > --- a/cmds/device.c
> > +++ b/cmds/device.c
> > @@ -49,6 +49,7 @@ static const char * const cmd_device_add_usage[] = {
> > "",
> > "-K|--nodiscard do not perform whole device TRIM on devices that report such capability",
> > "-f|--force force overwrite existing filesystem on the disk",
> > + "-q|--enqueue enqueue if an exclusive operation is running",
>
> Short for -q should not be used due to confusion with --quiet. Also I
> think that --enqueue is not a common action that would need a short
> option, the long option is always safe.

Yes, and --quit as well. However, just --enqueue is fine.

>
> > --- a/common/sysfs.c
> > +++ b/common/sysfs.c
> > @@ -50,3 +50,29 @@ int get_exclusive_operation(int mp_fd, char *val)
> > val[n - 1] = '\0';
> > return n;
> > }
> > +
> > +int sysfs_wait(int fd, int seconds)
> > +{
> > + fd_set fds;
> > + struct timeval tv;
> > +
> > + FD_ZERO(&fds);
> > + FD_SET(fd, &fds);
> > +
> > + tv.tv_sec = seconds;
> > + tv.tv_usec = 0;
> > +
> > + return select(fd+1, NULL, NULL, &fds, &tv);
>
> With the short sleep times, do we need to wait using select? Yes this
> would return once the notification event is sent but as the sleep time
> is 1 second, it could simply be sleep(1) unconditionally.

Yes, the kernel provides the sysfs notification. We could sys_wait() for
longer (with a higher wait parameter) and yet the function would return
the moment the kernel notifies the change. I know we are not using it
now, but it is better to use robust interfaces when a event notification
is provided by the kernel.

>
> > +}
> > +
> > +void wait_for_exclusive_operation(int dirfd)
> > +{
> > + char exop[BTRFS_SYSFS_EXOP_SIZE];
> > + int fd;
> > +
> > + fd = sysfs_open(dirfd, "exclusive_operation");
> > + while ((sysfs_get_str_fd(fd, exop, BTRFS_SYSFS_EXOP_SIZE) > 0) &&
> > + strncmp(exop, "none", 4))
> > + sysfs_wait(fd, 1);
> > + close(fd);
> > +}
> > diff --git a/common/utils.h b/common/utils.h
> > index be8aab58..f141edb6 100644
> > --- a/common/utils.h
> > +++ b/common/utils.h
> > @@ -155,5 +155,6 @@ int btrfs_warn_multiple_profiles(int fd);
> >
> > #define BTRFS_SYSFS_EXOP_SIZE 16
> > int get_exclusive_operation(int fd, char *val);
> > +void wait_for_exclusive_operation(int fd);
> >
> > #endif
> > --
> > 2.26.2

--
Goldwyn