All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations
@ 2020-08-25 15:02 Goldwyn Rodrigues
  2020-08-25 15:02 ` [PATCH 1/2] btrfs: enumerate the type of exclusive operation in progress Goldwyn Rodrigues
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-08-25 15:02 UTC (permalink / raw)
  To: linux-btrfs

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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] btrfs: enumerate the type of exclusive operation in progress
  2020-08-25 15:02 [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations Goldwyn Rodrigues
@ 2020-08-25 15:02 ` Goldwyn Rodrigues
  2020-09-02 13:42   ` David Sterba
  2020-08-25 15:02 ` [PATCH 2/2] btrfs: export currently executing exclusive operation via sysfs Goldwyn Rodrigues
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-08-25 15:02 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goldwyn Rodrigues, Nikolay Borisov

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] btrfs: export currently executing exclusive operation via sysfs
  2020-08-25 15:02 [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations Goldwyn Rodrigues
  2020-08-25 15:02 ` [PATCH 1/2] btrfs: enumerate the type of exclusive operation in progress Goldwyn Rodrigues
@ 2020-08-25 15:02 ` Goldwyn Rodrigues
  2020-08-25 15:03 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
  2020-09-02 20:11 ` [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations David Sterba
  3 siblings, 0 replies; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-08-25 15:02 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Goldwyn Rodrigues, Nikolay Borisov

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd
  2020-08-25 15:02 [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations Goldwyn Rodrigues
  2020-08-25 15:02 ` [PATCH 1/2] btrfs: enumerate the type of exclusive operation in progress Goldwyn Rodrigues
  2020-08-25 15:02 ` [PATCH 2/2] btrfs: export currently executing exclusive operation via sysfs Goldwyn Rodrigues
@ 2020-08-25 15:03 ` Goldwyn Rodrigues
  2020-08-25 15:03   ` [PATCH 2/4] btrfs-progs: add sysfs file reading functions Goldwyn Rodrigues
                     ` (2 more replies)
  2020-09-02 20:11 ` [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations David Sterba
  3 siblings, 3 replies; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-08-25 15:03 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] btrfs-progs: add sysfs file reading functions
  2020-08-25 15:03 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
@ 2020-08-25 15:03   ` Goldwyn Rodrigues
  2020-08-25 15:03   ` [PATCH 3/4] btrfs-progs: Check for exclusive operation before issuing ioctl Goldwyn Rodrigues
  2020-08-25 15:03   ` [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately Goldwyn Rodrigues
  2 siblings, 0 replies; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-08-25 15:03 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] btrfs-progs: Check for exclusive operation before issuing ioctl
  2020-08-25 15:03 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
  2020-08-25 15:03   ` [PATCH 2/4] btrfs-progs: add sysfs file reading functions Goldwyn Rodrigues
@ 2020-08-25 15:03   ` Goldwyn Rodrigues
  2020-08-25 15:03   ` [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately Goldwyn Rodrigues
  2 siblings, 0 replies; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-08-25 15:03 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately
  2020-08-25 15:03 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
  2020-08-25 15:03   ` [PATCH 2/4] btrfs-progs: add sysfs file reading functions Goldwyn Rodrigues
  2020-08-25 15:03   ` [PATCH 3/4] btrfs-progs: Check for exclusive operation before issuing ioctl Goldwyn Rodrigues
@ 2020-08-25 15:03   ` Goldwyn Rodrigues
  2020-09-02 14:11     ` David Sterba
  2 siblings, 1 reply; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-08-25 15:03 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] btrfs: enumerate the type of exclusive operation in progress
  2020-08-25 15:02 ` [PATCH 1/2] btrfs: enumerate the type of exclusive operation in progress Goldwyn Rodrigues
@ 2020-09-02 13:42   ` David Sterba
  0 siblings, 0 replies; 13+ messages in thread
From: David Sterba @ 2020-09-02 13:42 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues, Nikolay Borisov

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately
  2020-08-25 15:03   ` [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately Goldwyn Rodrigues
@ 2020-09-02 14:11     ` David Sterba
  2020-09-02 20:45       ` Goldwyn Rodrigues
  0 siblings, 1 reply; 13+ messages in thread
From: David Sterba @ 2020-09-02 14:11 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs, Goldwyn Rodrigues

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations
  2020-08-25 15:02 [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations Goldwyn Rodrigues
                   ` (2 preceding siblings ...)
  2020-08-25 15:03 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
@ 2020-09-02 20:11 ` David Sterba
  3 siblings, 0 replies; 13+ messages in thread
From: David Sterba @ 2020-09-02 20:11 UTC (permalink / raw)
  To: Goldwyn Rodrigues; +Cc: linux-btrfs

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately
  2020-09-02 14:11     ` David Sterba
@ 2020-09-02 20:45       ` Goldwyn Rodrigues
  0 siblings, 0 replies; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-09-02 20:45 UTC (permalink / raw)
  To: dsterba, linux-btrfs, 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately
  2020-08-03 20:30 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
@ 2020-08-03 20:30   ` Goldwyn Rodrigues
  0 siblings, 0 replies; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-08-03 20:30 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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.

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 9a18c753..123da97a 100644
--- a/common/sysfs.c
+++ b/common/sysfs.c
@@ -58,3 +58,29 @@ int get_exclusive_operation(int mp_fd, char *val)
 	*s = '\0';
 	return strlen(val);
 }
+
+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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately
  2020-07-27 22:08 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
@ 2020-07-27 22:08   ` Goldwyn Rodrigues
  0 siblings, 0 replies; 13+ messages in thread
From: Goldwyn Rodrigues @ 2020-07-27 22:08 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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 91ed270c..0067d098 100644
--- a/common/sysfs.c
+++ b/common/sysfs.c
@@ -58,3 +58,29 @@ int get_exclusive_operation(int mp_fd, char *val)
 	*s = '\0';
 	return strlen(val);
 }
+
+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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-09-02 20:45 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-25 15:02 [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations Goldwyn Rodrigues
2020-08-25 15:02 ` [PATCH 1/2] btrfs: enumerate the type of exclusive operation in progress Goldwyn Rodrigues
2020-09-02 13:42   ` David Sterba
2020-08-25 15:02 ` [PATCH 2/2] btrfs: export currently executing exclusive operation via sysfs Goldwyn Rodrigues
2020-08-25 15:03 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
2020-08-25 15:03   ` [PATCH 2/4] btrfs-progs: add sysfs file reading functions Goldwyn Rodrigues
2020-08-25 15:03   ` [PATCH 3/4] btrfs-progs: Check for exclusive operation before issuing ioctl Goldwyn Rodrigues
2020-08-25 15:03   ` [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately Goldwyn Rodrigues
2020-09-02 14:11     ` David Sterba
2020-09-02 20:45       ` Goldwyn Rodrigues
2020-09-02 20:11 ` [PATCH v3 0/2] btrfs: Enumerate and export exclusive operations David Sterba
  -- strict thread matches above, loose matches on Subject: below --
2020-08-03 20:29 [PATCH v2 0/3] " Goldwyn Rodrigues
2020-08-03 20:30 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
2020-08-03 20:30   ` [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately Goldwyn Rodrigues
2020-07-27 22:04 [PATCH 0/3] btrfs: Enumerate and export exclusive operations Goldwyn Rodrigues
2020-07-27 22:08 ` [PATCH 1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd Goldwyn Rodrigues
2020-07-27 22:08   ` [PATCH 4/4] btrfs-progs: Enqueue command if it can't be performed immediately Goldwyn Rodrigues

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.