[PATCH v3 0/2] btrfs: Enumerate and export exclusive operations

[PATCH v3 0/2] btrfs: Enumerate and export exclusive operations

[Goldwyn Rodrigues]

This patch series enumerates the exlcusive operation currently being
perfomed by the current filesystem and exports it in the sys filesytem
at /sys/fs/btrfs/<fsid>/exclusive_operation.

This would enable our tools to specify precisely which operation is
running on why starting an exclusive operation failed. The series also
adds a sysfs_notify() to alert userspace when the state changes, so
userspace can perform select() on it to get notified of the change.
This would enable us to enqueue a command which will wait for current
exclusive operation to complete before issuing the next exclusive
operation. This has been done synchronously as opposed to a background
process, or else error collection (if any) will become a nightmare.

For backward compatibility, the tools continue working as before if the
sys file is not present.

Changes since v1:
 - Corrected call for btrfs_start_exop() in btrfs_ioctl_dev_replace()
 - Use fsid_str[] instead of fsid[] to save on uuid_parse()

Changes since v2:
 - Dropped patch to add additional balance information
 - modified (simplified) progs patches accordingly

[PATCH 1/2] btrfs: enumerate the type of exclusive operation in progress

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Instead of using a flag bit for exclusive operation, use an atomic_t
variable to store if the exclusive operation is being performed.
Introduce an API to start and finish an exclusive operation.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h       | 24 +++++++++++++++++++-----
 fs/btrfs/dev-replace.c |  4 ++--
 fs/btrfs/inode.c       |  4 ++--
 fs/btrfs/ioctl.c       | 36 +++++++++++++++++++++++-------------
 fs/btrfs/volumes.c     |  8 ++++----
 5 files changed, 50 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a3b110ffbc93..e7ac63446346 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -540,11 +540,6 @@ enum {
 	BTRFS_FS_QUOTA_OVERRIDE,
 	/* Used to record internally whether fs has been frozen */
 	BTRFS_FS_FROZEN,
-	/*
-	 * Indicate that a whole-filesystem exclusive operation is running
-	 * (device replace, resize, device add/delete, balance)
-	 */
-	BTRFS_FS_EXCL_OP,
 	/*
 	 * Indicate that balance has been set up from the ioctl and is in the
 	 * main phase. The fs_info::balance_ctl is initialized.
@@ -565,6 +560,20 @@ enum {
 	BTRFS_FS_DISCARD_RUNNING,
 };
 
+/*
+ * Exclusive operation values
+ * (device replace, resize, device add/remove, balance)
+ */
+enum btrfs_exclusive_operation_t {
+	BTRFS_EXCLOP_NONE = 0,
+	BTRFS_EXCLOP_BALANCE,
+	BTRFS_EXCLOP_DEV_ADD,
+	BTRFS_EXCLOP_DEV_REPLACE,
+	BTRFS_EXCLOP_DEV_REMOVE,
+	BTRFS_EXCLOP_SWAP_ACTIVATE,
+	BTRFS_EXCLOP_RESIZE,
+};
+
 struct btrfs_fs_info {
 	u8 chunk_tree_uuid[BTRFS_UUID_SIZE];
 	unsigned long flags;
@@ -936,6 +945,9 @@ struct btrfs_fs_info {
 	 */
 	int send_in_progress;
 
+	/* Type of exclusive operation running */
+	atomic_t exclusive_operation;
+
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
 	spinlock_t ref_verify_lock;
 	struct rb_root block_tree;
@@ -3034,6 +3046,8 @@ void btrfs_get_block_group_info(struct list_head *groups_list,
 				struct btrfs_ioctl_space_info *space);
 void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info,
 			       struct btrfs_ioctl_balance_args *bargs);
+bool btrfs_exclop_start(struct btrfs_fs_info *fs_info, int type);
+void btrfs_exclop_finish(struct btrfs_fs_info *fs_info);
 
 /* file.c */
 int __init btrfs_auto_defrag_init(void);
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 769ac3098880..3671a2039dad 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -983,7 +983,7 @@ int btrfs_resume_dev_replace_async(struct btrfs_fs_info *fs_info)
 	 * should never allow both to start and pause. We don't want to allow
 	 * dev-replace to start anyway.
 	 */
-	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags)) {
+	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_REPLACE)) {
 		down_write(&dev_replace->rwsem);
 		dev_replace->replace_state =
 					BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
@@ -1020,7 +1020,7 @@ static int btrfs_dev_replace_kthread(void *data)
 	ret = btrfs_dev_replace_finishing(fs_info, ret);
 	WARN_ON(ret && ret != -ECANCELED);
 
-	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+	btrfs_exclop_finish(fs_info);
 	return 0;
 }
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 144b9bd79cfb..87d1b7888d1c 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -9978,7 +9978,7 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
 	 * concurrent device add which isn't actually necessary, but it's not
 	 * really worth the trouble to allow it.
 	 */
-	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags)) {
+	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_SWAP_ACTIVATE)) {
 		btrfs_warn(fs_info,
 	   "cannot activate swapfile while exclusive operation is running");
 		return -EBUSY;
@@ -10124,7 +10124,7 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
 	if (ret)
 		btrfs_swap_deactivate(file);
 
-	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+	btrfs_exclop_finish(fs_info);
 
 	if (ret)
 		return ret;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e45520d3cf51..256e58d42a7b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -378,6 +378,17 @@ static int check_xflags(unsigned int flags)
 	return 0;
 }
 
+bool btrfs_exclop_start(struct btrfs_fs_info *fs_info, int type)
+{
+	return !atomic_cmpxchg(&fs_info->exclusive_operation, BTRFS_EXCLOP_NONE,
+			type);
+}
+
+void btrfs_exclop_finish(struct btrfs_fs_info *fs_info)
+{
+	atomic_set(&fs_info->exclusive_operation, BTRFS_EXCLOP_NONE);
+}
+
 /*
  * Set the xflags from the internal inode flags. The remaining items of fsxattr
  * are zeroed.
@@ -1638,7 +1649,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 	if (ret)
 		return ret;
 
-	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags)) {
+	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_RESIZE)) {
 		mnt_drop_write_file(file);
 		return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
 	}
@@ -1752,7 +1763,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 out_free:
 	kfree(vol_args);
 out:
-	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+	btrfs_exclop_finish(fs_info);
 	mnt_drop_write_file(file);
 	return ret;
 }
@@ -3112,7 +3123,7 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags))
+	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_ADD))
 		return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
 
 	vol_args = memdup_user(arg, sizeof(*vol_args));
@@ -3129,7 +3140,7 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
 
 	kfree(vol_args);
 out:
-	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+	btrfs_exclop_finish(fs_info);
 	return ret;
 }
 
@@ -3158,7 +3169,7 @@ static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
 		goto out;
 	}
 
-	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags)) {
+	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_REMOVE)) {
 		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
 		goto out;
 	}
@@ -3169,7 +3180,7 @@ static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
 		vol_args->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
 		ret = btrfs_rm_device(fs_info, vol_args->name, 0);
 	}
-	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+	btrfs_exclop_finish(fs_info);
 
 	if (!ret) {
 		if (vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID)
@@ -3200,7 +3211,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	if (ret)
 		return ret;
 
-	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags)) {
+	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_REMOVE)) {
 		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
 		goto out_drop_write;
 	}
@@ -3218,7 +3229,7 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 		btrfs_info(fs_info, "disk deleted %s", vol_args->name);
 	kfree(vol_args);
 out:
-	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+	btrfs_exclop_finish(fs_info);
 out_drop_write:
 	mnt_drop_write_file(file);
 
@@ -3722,11 +3733,11 @@ static long btrfs_ioctl_dev_replace(struct btrfs_fs_info *fs_info,
 			ret = -EROFS;
 			goto out;
 		}
-		if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags)) {
+		if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_REPLACE)) {
 			ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
 		} else {
 			ret = btrfs_dev_replace_by_ioctl(fs_info, p);
-			clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+			btrfs_exclop_finish(fs_info);
 		}
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS:
@@ -3937,7 +3948,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 		return ret;
 
 again:
-	if (!test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags)) {
+	if (btrfs_exclop_start(fs_info, BTRFS_EXCLOP_BALANCE)) {
 		mutex_lock(&fs_info->balance_mutex);
 		need_unlock = true;
 		goto locked;
@@ -3983,7 +3994,6 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 	}
 
 locked:
-	BUG_ON(!test_bit(BTRFS_FS_EXCL_OP, &fs_info->flags));
 
 	if (arg) {
 		bargs = memdup_user(arg, sizeof(*bargs));
@@ -4060,7 +4070,7 @@ static long btrfs_ioctl_balance(struct file *file, void __user *arg)
 out_unlock:
 	mutex_unlock(&fs_info->balance_mutex);
 	if (need_unlock)
-		clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+		btrfs_exclop_finish(fs_info);
 out:
 	mnt_drop_write_file(file);
 	return ret;
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1d5026fdfc75..a65199f3dc2b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4183,7 +4183,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 	if ((ret && ret != -ECANCELED && ret != -ENOSPC) ||
 	    balance_need_close(fs_info)) {
 		reset_balance_state(fs_info);
-		clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+		btrfs_exclop_finish(fs_info);
 	}
 
 	wake_up(&fs_info->balance_wait_q);
@@ -4194,7 +4194,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		reset_balance_state(fs_info);
 	else
 		kfree(bctl);
-	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+	btrfs_exclop_finish(fs_info);
 
 	return ret;
 }
@@ -4296,7 +4296,7 @@ int btrfs_recover_balance(struct btrfs_fs_info *fs_info)
 	 * is in a paused state and must have fs_info::balance_ctl properly
 	 * set up.
 	 */
-	if (test_and_set_bit(BTRFS_FS_EXCL_OP, &fs_info->flags))
+	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_BALANCE))
 		btrfs_warn(fs_info,
 	"balance: cannot set exclusive op status, resume manually");
 
@@ -4378,7 +4378,7 @@ int btrfs_cancel_balance(struct btrfs_fs_info *fs_info)
 
 		if (fs_info->balance_ctl) {
 			reset_balance_state(fs_info);
-			clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
+			btrfs_exclop_finish(fs_info);
 			btrfs_info(fs_info, "balance: canceled");
 		}
 	}
-- 
2.26.2

Re: [PATCH 1/2] btrfs: enumerate the type of exclusive operation in progress

[David Sterba]

On Tue, Aug 25, 2020 at 10:02:32AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using a flag bit for exclusive operation, use an atomic_t
> variable to store if the exclusive operation is being performed.
> Introduce an API to start and finish an exclusive operation.

I think we don't need it to be atomic_t, the change must be atomic but
for that cmpxchg should be sufficient.

> +enum btrfs_exclusive_operation_t {
> +	BTRFS_EXCLOP_NONE = 0,
> +	BTRFS_EXCLOP_BALANCE,
> +	BTRFS_EXCLOP_DEV_ADD,
> +	BTRFS_EXCLOP_DEV_REPLACE,
> +	BTRFS_EXCLOP_DEV_REMOVE,
> +	BTRFS_EXCLOP_SWAP_ACTIVATE,
> +	BTRFS_EXCLOP_RESIZE,

Yeah this is much better than the single bit.

[PATCH 2/2] btrfs: export currently executing exclusive operation via sysfs

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

/sys/fs/<fsid>/exclusive_operation contains the currently executing
exclusive operation. Add a sysfs_notify() when operation end, so
userspace can be notified of exclusive operation is finished.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ioctl.c |  2 ++
 fs/btrfs/sysfs.c | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 256e58d42a7b..97ce65c7a631 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -387,6 +387,8 @@ bool btrfs_exclop_start(struct btrfs_fs_info *fs_info, int type)
 void btrfs_exclop_finish(struct btrfs_fs_info *fs_info)
 {
 	atomic_set(&fs_info->exclusive_operation, BTRFS_EXCLOP_NONE);
+	sysfs_notify(&fs_info->fs_devices->fsid_kobj, NULL,
+			"exclusive_operation");
 }
 
 /*
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 8593086d1d10..a301cf74336d 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -809,6 +809,30 @@ static ssize_t btrfs_checksum_show(struct kobject *kobj,
 
 BTRFS_ATTR(, checksum, btrfs_checksum_show);
 
+static ssize_t btrfs_exclusive_operation_show(struct kobject *kobj,
+		struct kobj_attribute *a, char *buf)
+{
+	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
+	switch (atomic_read(&fs_info->exclusive_operation)) {
+		case  BTRFS_EXCLOP_NONE:
+			return scnprintf(buf, PAGE_SIZE, "none\n");
+		case BTRFS_EXCLOP_BALANCE:
+			return scnprintf(buf, PAGE_SIZE, "balance\n");
+		case BTRFS_EXCLOP_DEV_ADD:
+			return scnprintf(buf, PAGE_SIZE, "device add\n");
+		case BTRFS_EXCLOP_DEV_REPLACE:
+			return scnprintf(buf, PAGE_SIZE, "device replace\n");
+		case BTRFS_EXCLOP_DEV_REMOVE:
+			return scnprintf(buf, PAGE_SIZE, "device remove\n");
+		case BTRFS_EXCLOP_SWAP_ACTIVATE:
+			return scnprintf(buf, PAGE_SIZE, "swap activate\n");
+		case BTRFS_EXCLOP_RESIZE:
+			return scnprintf(buf, PAGE_SIZE, "resize\n");
+	}
+	return 0;
+}
+BTRFS_ATTR(, exclusive_operation, btrfs_exclusive_operation_show);
+
 static const struct attribute *btrfs_attrs[] = {
 	BTRFS_ATTR_PTR(, label),
 	BTRFS_ATTR_PTR(, nodesize),
@@ -817,6 +841,7 @@ static const struct attribute *btrfs_attrs[] = {
 	BTRFS_ATTR_PTR(, quota_override),
 	BTRFS_ATTR_PTR(, metadata_uuid),
 	BTRFS_ATTR_PTR(, checksum),
+	BTRFS_ATTR_PTR(, exclusive_operation),
 	NULL,
 };
 
-- 
2.26.2

[PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Add a function get_fsid_fd() to use an open file fd to get the
fsid of the mounted filesystem.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 common/utils.c | 30 ++++++++++++++++--------------
 common/utils.h |  1 +
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/common/utils.c b/common/utils.c
index 9742c2e1..dbe1e806 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -1097,32 +1097,34 @@ out:
 	return ret;
 }
 
+int get_fsid_fd(int fd, u8 *fsid)
+{
+	int ret;
+	struct btrfs_ioctl_fs_info_args args;
+
+	ret = ioctl(fd, BTRFS_IOC_FS_INFO, &args);
+	if (ret < 0)
+		return -errno;
+
+	memcpy(fsid, args.fsid, BTRFS_FSID_SIZE);
+	return 0;
+}
+
 int get_fsid(const char *path, u8 *fsid, int silent)
 {
 	int ret;
 	int fd;
-	struct btrfs_ioctl_fs_info_args args;
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
-		ret = -errno;
 		if (!silent)
 			error("failed to open %s: %m", path);
-		goto out;
-	}
-
-	ret = ioctl(fd, BTRFS_IOC_FS_INFO, &args);
-	if (ret < 0) {
-		ret = -errno;
-		goto out;
+		return -errno;
 	}
 
-	memcpy(fsid, args.fsid, BTRFS_FSID_SIZE);
-	ret = 0;
+	ret = get_fsid_fd(fd, fsid);
+	close(fd);
 
-out:
-	if (fd != -1)
-		close(fd);
 	return ret;
 }
 
diff --git a/common/utils.h b/common/utils.h
index 43e7f471..e34bb5a4 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -75,6 +75,7 @@ void close_file_or_dir(int fd, DIR *dirstream);
 int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		struct btrfs_ioctl_dev_info_args **di_ret);
 int get_fsid(const char *path, u8 *fsid, int silent);
+int get_fsid_fd(int fd, u8 *fsid);
 
 int get_label(const char *btrfs_dev, char *label);
 int set_label(const char *btrfs_dev, const char *label);
-- 
2.26.2

[PATCH 2/4] btrfs-progs: add sysfs file reading functions

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Functions to read and and open sysfs files.
Given a fd, find the fsid associated with the mount and convert into
the sysfs directory to look. Read the exclusive_operation file to find
the current exclusive operation running.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 Makefile       |  4 ++--
 common/sysfs.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 common/utils.h |  3 +++
 3 files changed, 57 insertions(+), 2 deletions(-)
 create mode 100644 common/sysfs.c

diff --git a/Makefile b/Makefile
index ee57d9f8..8a267c45 100644
--- a/Makefile
+++ b/Makefile
@@ -168,8 +168,8 @@ libbtrfs_objects = send-stream.o send-utils.o kernel-lib/rbtree.o btrfs-list.o \
 		   kernel-shared/free-space-tree.o repair.o kernel-shared/inode-item.o \
 		   kernel-shared/file-item.o \
 		   kernel-lib/raid56.o kernel-lib/tables.o \
-		   common/device-scan.o common/path-utils.o \
-		   common/utils.o libbtrfsutil/subvolume.o libbtrfsutil/stubs.o \
+		   common/device-scan.o common/path-utils.o common/utils.o \
+		   common/sysfs.o libbtrfsutil/subvolume.o libbtrfsutil/stubs.o \
 		   crypto/hash.o crypto/xxhash.o $(CRYPTO_OBJECTS)
 libbtrfs_headers = send-stream.h send-utils.h send.h kernel-lib/rbtree.h btrfs-list.h \
 	       crypto/crc32c.h kernel-lib/list.h kerncompat.h \
diff --git a/common/sysfs.c b/common/sysfs.c
new file mode 100644
index 00000000..b2c95cfb
--- /dev/null
+++ b/common/sysfs.c
@@ -0,0 +1,52 @@
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include <unistd.h>
+#include <sys/select.h>
+#include <uuid/uuid.h>
+
+#include "common/utils.h"
+
+static char fsid_str[BTRFS_UUID_UNPARSED_SIZE] = {'\0'};
+
+int sysfs_open(int fd, const char *filename)
+{
+	u8 fsid[BTRFS_UUID_SIZE];
+	int ret;
+	char sysfs_file[128];
+
+	if (fsid_str[0] == '\0') {
+		ret = get_fsid_fd(fd, fsid);
+		if (ret < 0)
+			return ret;
+		uuid_unparse(fsid, fsid_str);
+	}
+
+	snprintf(sysfs_file, 128, "/sys/fs/btrfs/%s/%s", fsid_str, filename);
+	return open(sysfs_file, O_RDONLY);
+}
+
+int sysfs_get_str_fd(int fd, char *val, int size)
+{
+	lseek(fd, 0, SEEK_SET);
+	memset(val, '\0', size);
+	return read(fd, val, size);
+}
+
+int get_exclusive_operation(int mp_fd, char *val)
+{
+	int fd;
+	int n;
+
+	fd = sysfs_open(mp_fd, "exclusive_operation");
+	if (fd < 0)
+		return fd;
+
+	n = sysfs_get_str_fd(fd, val, BTRFS_SYSFS_EXOP_SIZE);
+	close(fd);
+
+	val[n - 1] = '\0';
+	return n;
+}
diff --git a/common/utils.h b/common/utils.h
index e34bb5a4..be8aab58 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -153,4 +153,7 @@ void init_rand_seed(u64 seed);
 char *btrfs_test_for_multiple_profiles(int fd);
 int btrfs_warn_multiple_profiles(int fd);
 
+#define BTRFS_SYSFS_EXOP_SIZE		16
+int get_exclusive_operation(int fd, char *val);
+
 #endif
-- 
2.26.2

[PATCH 3/4] btrfs-progs: Check for exclusive operation before issuing ioctl

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Check if an exclusive operation is running and if it is, err with the
name of the exclusive operation running.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 cmds/device.c     | 14 ++++++++++++++
 cmds/filesystem.c |  7 +++++++
 2 files changed, 21 insertions(+)

diff --git a/cmds/device.c b/cmds/device.c
index 99ceed93..6acd4ae6 100644
--- a/cmds/device.c
+++ b/cmds/device.c
@@ -61,6 +61,7 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 	int discard = 1;
 	int force = 0;
 	int last_dev;
+	char exop[BTRFS_SYSFS_EXOP_SIZE];
 
 	optind = 0;
 	while (1) {
@@ -96,6 +97,12 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 	if (fdmnt < 0)
 		return 1;
 
+	if (get_exclusive_operation(fdmnt, exop) > 0 && strcmp(exop, "none")) {
+		error("unable to add device: %s in progress", exop);
+		close_file_or_dir(fdmnt, dirstream);
+		return 1;
+	}
+
 	for (i = optind; i < last_dev; i++){
 		struct btrfs_ioctl_vol_args ioctl_args;
 		int	devfd, res;
@@ -155,6 +162,7 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 	char	*mntpnt;
 	int i, fdmnt, ret = 0;
 	DIR	*dirstream = NULL;
+	char exop[BTRFS_SYSFS_EXOP_SIZE];
 
 	clean_args_no_options(cmd, argc, argv);
 
@@ -167,6 +175,12 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 	if (fdmnt < 0)
 		return 1;
 
+	if (get_exclusive_operation(fdmnt, exop) > 0 && strcmp(exop, "none")) {
+		error("unable to remove device: %s in progress", exop);
+		close_file_or_dir(fdmnt, dirstream);
+		return 1;
+	}
+
 	for(i = optind; i < argc - 1; i++) {
 		struct	btrfs_ioctl_vol_args arg;
 		struct btrfs_ioctl_vol_args_v2 argv2 = {0};
diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index 6c1b6908..c3efb405 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -1079,6 +1079,7 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 	char	*amount, *path;
 	DIR	*dirstream = NULL;
 	struct stat st;
+	char exop[BTRFS_SYSFS_EXOP_SIZE];
 
 	clean_args_no_options_relaxed(cmd, argc, argv);
 
@@ -1110,6 +1111,12 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 	if (fd < 0)
 		return 1;
 
+	if (get_exclusive_operation(fd, exop) > 0 && strcmp(exop, "none")) {
+		error("unable to resize: %s in progress", exop);
+		close_file_or_dir(fd, dirstream);
+		return 1;
+	}
+
 	printf("Resize '%s' of '%s'\n", path, amount);
 	memset(&args, 0, sizeof(args));
 	strncpy_null(args.name, amount);
-- 
2.26.2

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

[Goldwyn Rodrigues]

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?

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 cmds/device.c     | 56 +++++++++++++++++++++++++++++++++++++++--------
 cmds/filesystem.c | 25 +++++++++++++++++----
 common/sysfs.c    | 26 ++++++++++++++++++++++
 common/utils.h    |  1 +
 4 files changed, 95 insertions(+), 13 deletions(-)

diff --git a/cmds/device.c b/cmds/device.c
index 6acd4ae6..12d92dac 100644
--- 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",
 	NULL
 };
 
@@ -62,6 +63,7 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 	int force = 0;
 	int last_dev;
 	char exop[BTRFS_SYSFS_EXOP_SIZE];
+	bool enqueue = false;
 
 	optind = 0;
 	while (1) {
@@ -69,10 +71,11 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 		static const struct option long_options[] = {
 			{ "nodiscard", optional_argument, NULL, 'K'},
 			{ "force", no_argument, NULL, 'f'},
+			{ "enqueue", no_argument, NULL, 'q'},
 			{ NULL, 0, NULL, 0}
 		};
 
-		c = getopt_long(argc, argv, "Kf", long_options, NULL);
+		c = getopt_long(argc, argv, "Kfq", long_options, NULL);
 		if (c < 0)
 			break;
 		switch (c) {
@@ -82,6 +85,9 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 		case 'f':
 			force = 1;
 			break;
+		case 'q':
+			enqueue = true;
+			break;
 		default:
 			usage_unknown_option(cmd, argv);
 		}
@@ -98,9 +104,15 @@ static int cmd_device_add(const struct cmd_struct *cmd,
 		return 1;
 
 	if (get_exclusive_operation(fdmnt, exop) > 0 && strcmp(exop, "none")) {
-		error("unable to add device: %s in progress", exop);
-		close_file_or_dir(fdmnt, dirstream);
-		return 1;
+		if (enqueue) {
+			printf("%s in progress. Waiting for %s to finish.\n",
+					exop, exop);
+			wait_for_exclusive_operation(fdmnt);
+		} else {
+			error("unable to add: %s in progress", exop);
+			close_file_or_dir(fdmnt, dirstream);
+			return 1;
+		}
 	}
 
 	for (i = optind; i < last_dev; i++){
@@ -163,8 +175,27 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 	int i, fdmnt, ret = 0;
 	DIR	*dirstream = NULL;
 	char exop[BTRFS_SYSFS_EXOP_SIZE];
+	bool enqueue = false;
 
-	clean_args_no_options(cmd, argc, argv);
+
+	while (1) {
+		int c;
+		static const struct option long_options[] = {
+			{ "enqueue", no_argument, NULL, 'q'},
+			{ NULL, 0, NULL, 0}
+		};
+
+		c = getopt_long(argc, argv, "q", long_options, NULL);
+		if (c < 0)
+			break;
+		switch (c) {
+		case 'q':
+			enqueue = true;
+			break;
+		default:
+			usage_unknown_option(cmd, argv);
+		}
+	}
 
 	if (check_argc_min(argc - optind, 2))
 		return 1;
@@ -176,9 +207,15 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 		return 1;
 
 	if (get_exclusive_operation(fdmnt, exop) > 0 && strcmp(exop, "none")) {
-		error("unable to remove device: %s in progress", exop);
-		close_file_or_dir(fdmnt, dirstream);
-		return 1;
+		if (enqueue) {
+			printf("%s in progress. Waiting for %s to finish.\n",
+					exop, exop);
+			wait_for_exclusive_operation(fdmnt);
+		} else {
+			error("unable to remove device: %s in progress", exop);
+			close_file_or_dir(fdmnt, dirstream);
+			return 1;
+		}
 	}
 
 	for(i = optind; i < argc - 1; i++) {
@@ -251,7 +288,8 @@ static int _cmd_device_remove(const struct cmd_struct *cmd,
 	"the device.",								\
 	"If 'missing' is specified for <device>, the first device that is",	\
 	"described by the filesystem metadata, but not present at the mount",	\
-	"time will be removed. (only in degraded mode)"
+	"time will be removed. (only in degraded mode)", \
+	"-q|--enqueue	   enqueue if an exclusive operation is running"
 
 static const char * const cmd_device_remove_usage[] = {
 	"btrfs device remove <device>|<devid> [<device>|<devid>...] <path>",
diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index c3efb405..a584b1d3 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -1080,8 +1080,19 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 	DIR	*dirstream = NULL;
 	struct stat st;
 	char exop[BTRFS_SYSFS_EXOP_SIZE];
+	bool enqueue = false;
 
-	clean_args_no_options_relaxed(cmd, argc, argv);
+	while(1) {
+		int c = getopt(argc - 2, argv, "q");
+		if (c < 0)
+			break;
+
+		switch(c) {
+		case 'q':
+			enqueue = true;
+			break;
+		}
+	}
 
 	if (check_argc_exact(argc - optind, 2))
 		return 1;
@@ -1112,9 +1123,15 @@ static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 		return 1;
 
 	if (get_exclusive_operation(fd, exop) > 0 && strcmp(exop, "none")) {
-		error("unable to resize: %s in progress", exop);
-		close_file_or_dir(fd, dirstream);
-		return 1;
+		if (enqueue) {
+			printf("%s in progress. Waiting for %s to finish.\n",
+					exop, exop);
+			wait_for_exclusive_operation(fd);
+		} else {
+			error("unable to resize: %s in progress", exop);
+			close_file_or_dir(fd, dirstream);
+			return 1;
+		}
 	}
 
 	printf("Resize '%s' of '%s'\n", path, amount);
diff --git a/common/sysfs.c b/common/sysfs.c
index b2c95cfb..3b6df52e 100644
--- 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);
+}
+
+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

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

[David Sterba]

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.

> --- 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.

> --- 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.

> +}
> +
> +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

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

[Goldwyn Rodrigues]

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

Re: [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations

[David Sterba]

On Tue, Aug 25, 2020 at 10:02:31AM -0500, Goldwyn Rodrigues wrote:
> This patch series enumerates the exlcusive operation currently being
> perfomed by the current filesystem and exports it in the sys filesytem
> at /sys/fs/btrfs/<fsid>/exclusive_operation.
> 
> This would enable our tools to specify precisely which operation is
> running on why starting an exclusive operation failed. The series also
> adds a sysfs_notify() to alert userspace when the state changes, so
> userspace can perform select() on it to get notified of the change.
> This would enable us to enqueue a command which will wait for current
> exclusive operation to complete before issuing the next exclusive
> operation. This has been done synchronously as opposed to a background
> process, or else error collection (if any) will become a nightmare.
> 
> For backward compatibility, the tools continue working as before if the
> sys file is not present.
> 
> Changes since v1:
>  - Corrected call for btrfs_start_exop() in btrfs_ioctl_dev_replace()
>  - Use fsid_str[] instead of fsid[] to save on uuid_parse()
> 
> Changes since v2:
>  - Dropped patch to add additional balance information
>  - modified (simplified) progs patches accordingly

I've switched exclusive_operation to long and used cmpxchg, plus some
fixup to comments still mentioning the old EXCL_OP bit, some other minor
style fixups.  The patches will be in for-next for a day or two and then
moved to misc-next.