[PATCH v4 0/6] Fix lockdep issues around device removal

[PATCH v4 0/6] Fix lockdep issues around device removal

[Josef Bacik]

v3->v4:
- I had a fixup for assinging devid that I mis-merged into the wrong patch,
  fixed this up by putting it in the correct patch.

v2->v3:
- Dropped the patches that kdave already merged.
- Added some prep patches that Anand had that I need for my fixes.
- Reworked device lookup to take an args struct since we were getting a
  complicated mixture of function arguments.
- Reworked device removal to generate this args struct so we can pass it into
  device_remove in order to find the correct device to remove.

v1->v2:
- Rework the first patch as it was wrong because we need it for seed devices.
- Fix another lockdep splat I uncovered while testing against seed devices to
  make sure I hadn't broken anything.

--- Original email ---

Hello,

The commit 87579e9b7d8d ("loop: use worker per cgroup instead of kworker")
enabled the use of workqueues for loopback devices, which brought with it
lockdep annotations for the workqueues for loopback devices.  This uncovered a
cascade of lockdep warnings because of how we mess with the block_device while
under the sb writers lock while doing the device removal.

The first patch seems innocuous but we have a lockdep_assert_held(&uuid_mutex)
in one of the helpers, which is why I have it first.  The code should never be
called which is why it is removed, but I'm removing it specifically to remove
confusion about the role of the uuid_mutex here.

The next 4 patches are to resolve the lockdep messages as they occur.  There are
several issues and I address them one at a time until we're no longer getting
lockdep warnings.

The final patch doesn't necessarily have to go in right away, it's just a
cleanup as I noticed we have a lot of duplicated code between the v1 and v2
device removal handling.  Thanks,

Josef

Anand Jain (2):
  btrfs: use num_device to check for the last surviving seed device
  btrfs: add comments for device counts in struct btrfs_fs_devices

Josef Bacik (4):
  btrfs: do not call close_fs_devices in btrfs_rm_device
  btrfs: handle device lookup with btrfs_dev_lookup_args
  btrfs: add a btrfs_get_dev_args_from_path helper
  btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls

 fs/btrfs/dev-replace.c |  18 ++--
 fs/btrfs/ioctl.c       |  80 ++++++++-------
 fs/btrfs/scrub.c       |   6 +-
 fs/btrfs/volumes.c     | 215 +++++++++++++++++++++++++++--------------
 fs/btrfs/volumes.h     |  40 +++++++-
 5 files changed, 238 insertions(+), 121 deletions(-)

-- 
2.26.3

[PATCH v4 1/6] btrfs: use num_device to check for the last surviving seed device

[Josef Bacik]

From: Anand Jain <anand.jain@oracle.com>

For both sprout and seed fsids,
 btrfs_fs_devices::num_devices provides device count including missing
 btrfs_fs_devices::open_devices provides device count excluding missing

We create a dummy struct btrfs_device for the missing device, so
num_devices != open_devices when there is a missing device.

In btrfs_rm_devices() we wrongly check for %cur_devices->open_devices
before freeing the seed fs_devices. Instead we should check for
%cur_devices->num_devices.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6031e2f4c6bc..0941f61d8071 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2211,7 +2211,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	synchronize_rcu();
 	btrfs_free_device(device);
 
-	if (cur_devices->open_devices == 0) {
+	if (cur_devices->num_devices == 0) {
 		list_del_init(&cur_devices->seed_list);
 		close_fs_devices(cur_devices);
 		free_fs_devices(cur_devices);
-- 
2.26.3

[PATCH v4 2/6] btrfs: add comments for device counts in struct btrfs_fs_devices

[Josef Bacik]

From: Anand Jain <anand.jain@oracle.com>

A bug was was checking a wrong device count before we delete the struct
btrfs_fs_devices in btrfs_rm_device(). To avoid future confusion and
easy reference add a comment about the various device counts that we have
in the struct btrfs_fs_devices.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 83075d6855db..c7ac43d8a7e8 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -236,11 +236,30 @@ struct btrfs_fs_devices {
 	bool fsid_change;
 	struct list_head fs_list;
 
+	/*
+	 * Number of devices under this fsid including missing and
+	 * replace-target device and excludes seed devices.
+	 */
 	u64 num_devices;
+
+	/*
+	 * The number of devices that successfully opened, including
+	 * replace-target, excludes seed devices.
+	 */
 	u64 open_devices;
+
+	/* The number of devices that are under the chunk allocation list. */
 	u64 rw_devices;
+
+	/* Count of missing devices under this fsid excluding seed device. */
 	u64 missing_devices;
 	u64 total_rw_bytes;
+
+	/*
+	 * Count of devices from btrfs_super_block::num_devices for this fsid,
+	 * which includes the seed device, excludes the transient replace-target
+	 * device.
+	 */
 	u64 total_devices;
 
 	/* Highest generation number of seen devices */
-- 
2.26.3

[PATCH v4 3/6] btrfs: do not call close_fs_devices in btrfs_rm_device

[Josef Bacik]

There's a subtle case where if we're removing the seed device from a
file system we need to free its private copy of the fs_devices.  However
we do not need to call close_fs_devices(), because at this point there
are no devices left to close as we've closed the last one.  The only
thing that close_fs_devices() does is decrement ->opened, which should
be 1.  We want to avoid calling close_fs_devices() here because it has a
lockdep_assert_held(&uuid_mutex), and we are going to stop holding the
uuid_mutex in this path.

So simply decrement the  ->opened counter like we should, and then clean
up like normal.  Also add a comment explaining what we're doing here as
I initially removed this code erroneously.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/volumes.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0941f61d8071..5f19d0863094 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2211,9 +2211,16 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	synchronize_rcu();
 	btrfs_free_device(device);
 
+	/*
+	 * This can happen if cur_devices is the private seed devices list.  We
+	 * cannot call close_fs_devices() here because it expects the uuid_mutex
+	 * to be held, but in fact we don't need that for the private
+	 * seed_devices, we can simply decrement cur_devices->opened and then
+	 * remove it from our list and free the fs_devices.
+	 */
 	if (cur_devices->num_devices == 0) {
 		list_del_init(&cur_devices->seed_list);
-		close_fs_devices(cur_devices);
+		cur_devices->opened--;
 		free_fs_devices(cur_devices);
 	}
 
-- 
2.26.3

Re: [PATCH v4 3/6] btrfs: do not call close_fs_devices in btrfs_rm_device

[Nikolay Borisov]

On 5.10.21 г. 23:12, Josef Bacik wrote:
> There's a subtle case where if we're removing the seed device from a
> file system we need to free its private copy of the fs_devices.  However
> we do not need to call close_fs_devices(), because at this point there
> are no devices left to close as we've closed the last one.  The only
> thing that close_fs_devices() does is decrement ->opened, which should
> be 1.  We want to avoid calling close_fs_devices() here because it has a
> lockdep_assert_held(&uuid_mutex), and we are going to stop holding the
> uuid_mutex in this path.
> 
> So simply decrement the  ->opened counter like we should, and then clean
> up like normal.  Also add a comment explaining what we're doing here as
> I initially removed this code erroneously.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/volumes.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 0941f61d8071..5f19d0863094 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2211,9 +2211,16 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  	synchronize_rcu();
>  	btrfs_free_device(device);
>  
> +	/*
> +	 * This can happen if cur_devices is the private seed devices list.  We
> +	 * cannot call close_fs_devices() here because it expects the uuid_mutex
> +	 * to be held, but in fact we don't need that for the private
> +	 * seed_devices, we can simply decrement cur_devices->opened and then
> +	 * remove it from our list and free the fs_devices.
> +	 */
>  	if (cur_devices->num_devices == 0) {
>  		list_del_init(&cur_devices->seed_list);
> -		close_fs_devices(cur_devices);
> +		cur_devices->opened--;

I think an ASSERT(opened == 1) is warranted as this is a special case
and there is very specific expectation of the state of opened.

>  		free_fs_devices(cur_devices);
>  	}
>  
> 

Re: [PATCH v4 3/6] btrfs: do not call close_fs_devices in btrfs_rm_device

[David Sterba]

On Wed, Oct 06, 2021 at 09:40:00AM +0300, Nikolay Borisov wrote:
> 
> 
> On 5.10.21 г. 23:12, Josef Bacik wrote:
> > There's a subtle case where if we're removing the seed device from a
> > file system we need to free its private copy of the fs_devices.  However
> > we do not need to call close_fs_devices(), because at this point there
> > are no devices left to close as we've closed the last one.  The only
> > thing that close_fs_devices() does is decrement ->opened, which should
> > be 1.  We want to avoid calling close_fs_devices() here because it has a
> > lockdep_assert_held(&uuid_mutex), and we are going to stop holding the
> > uuid_mutex in this path.
> > 
> > So simply decrement the  ->opened counter like we should, and then clean
> > up like normal.  Also add a comment explaining what we're doing here as
> > I initially removed this code erroneously.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/volumes.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 0941f61d8071..5f19d0863094 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -2211,9 +2211,16 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
> >  	synchronize_rcu();
> >  	btrfs_free_device(device);
> >  
> > +	/*
> > +	 * This can happen if cur_devices is the private seed devices list.  We
> > +	 * cannot call close_fs_devices() here because it expects the uuid_mutex
> > +	 * to be held, but in fact we don't need that for the private
> > +	 * seed_devices, we can simply decrement cur_devices->opened and then
> > +	 * remove it from our list and free the fs_devices.
> > +	 */
> >  	if (cur_devices->num_devices == 0) {
> >  		list_del_init(&cur_devices->seed_list);
> > -		close_fs_devices(cur_devices);
> > +		cur_devices->opened--;
> 
> I think an ASSERT(opened == 1) is warranted as this is a special case
> and there is very specific expectation of the state of opened.

Assert added, not tested yet.

Re: [PATCH v4 3/6] btrfs: do not call close_fs_devices in btrfs_rm_device

[Anand Jain]

On 06/10/2021 04:12, Josef Bacik wrote:
> There's a subtle case where if we're removing the seed device from a
> file system we need to free its private copy of the fs_devices.  However
> we do not need to call close_fs_devices(), because at this point there
> are no devices left to close as we've closed the last one.  The only
> thing that close_fs_devices() does is decrement ->opened, which should
> be 1.  We want to avoid calling close_fs_devices() here because it has a
> lockdep_assert_held(&uuid_mutex), and we are going to stop holding the
> uuid_mutex in this path.
> 
> So simply decrement the  ->opened counter like we should, and then clean
> up like normal.  Also add a comment explaining what we're doing here as
> I initially removed this code erroneously.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>


> ---
>   fs/btrfs/volumes.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 0941f61d8071..5f19d0863094 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2211,9 +2211,16 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>   	synchronize_rcu();
>   	btrfs_free_device(device);
>   
> +	/*
> +	 * This can happen if cur_devices is the private seed devices list.  We
> +	 * cannot call close_fs_devices() here because it expects the uuid_mutex
> +	 * to be held, but in fact we don't need that for the private
> +	 * seed_devices, we can simply decrement cur_devices->opened and then
> +	 * remove it from our list and free the fs_devices.
> +	 */
>   	if (cur_devices->num_devices == 0) {
>   		list_del_init(&cur_devices->seed_list);
> -		close_fs_devices(cur_devices);
> +		cur_devices->opened--;
>   		free_fs_devices(cur_devices);
>   	}
>   
> 

[PATCH v4 4/6] btrfs: handle device lookup with btrfs_dev_lookup_args

[Josef Bacik]

We have a lot of device lookup functions that all do something slightly
different.  Clean this up by adding a struct to hold the different
lookup criteria, and then pass this around to btrfs_find_device() so it
can do the proper matching based on the lookup criteria.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/dev-replace.c |  18 +++----
 fs/btrfs/ioctl.c       |  13 ++---
 fs/btrfs/scrub.c       |   6 ++-
 fs/btrfs/volumes.c     | 120 +++++++++++++++++++++++++----------------
 fs/btrfs/volumes.h     |  15 +++++-
 5 files changed, 108 insertions(+), 64 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index d029be40ea6f..ff25da2dbd59 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -70,6 +70,7 @@ static int btrfs_dev_replace_kthread(void *data);
 
 int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct btrfs_key key;
 	struct btrfs_root *dev_root = fs_info->dev_root;
 	struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
@@ -84,6 +85,8 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 	if (!dev_root)
 		return 0;
 
+	args.devid = BTRFS_DEV_REPLACE_DEVID;
+
 	path = btrfs_alloc_path();
 	if (!path) {
 		ret = -ENOMEM;
@@ -100,8 +103,7 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 		 * We don't have a replace item or it's corrupted.  If there is
 		 * a replace target, fail the mount.
 		 */
-		if (btrfs_find_device(fs_info->fs_devices,
-				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+		if (btrfs_find_device(fs_info->fs_devices, &args)) {
 			btrfs_err(fs_info,
 			"found replace target device without a valid replace item");
 			ret = -EUCLEAN;
@@ -163,8 +165,7 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 		 * We don't have an active replace item but if there is a
 		 * replace target, fail the mount.
 		 */
-		if (btrfs_find_device(fs_info->fs_devices,
-				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
+		if (btrfs_find_device(fs_info->fs_devices, &args)) {
 			btrfs_err(fs_info,
 			"replace devid present without an active replace item");
 			ret = -EUCLEAN;
@@ -175,11 +176,10 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
 		break;
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
-		dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
-						src_devid, NULL, NULL);
-		dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
-							BTRFS_DEV_REPLACE_DEVID,
-							NULL, NULL);
+		dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices, &args);
+		args.devid = src_devid;
+		dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices, &args);
+
 		/*
 		 * allow 'btrfs dev replace_cancel' if src/tgt device is
 		 * missing
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9eb0c1eb568e..b8508af4e539 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1602,6 +1602,7 @@ static int exclop_start_or_cancel_reloc(struct btrfs_fs_info *fs_info,
 static noinline int btrfs_ioctl_resize(struct file *file,
 					void __user *arg)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct inode *inode = file_inode(file);
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	u64 new_size;
@@ -1657,7 +1658,8 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 		btrfs_info(fs_info, "resizing devid %llu", devid);
 	}
 
-	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
+	args.devid = devid;
+	device = btrfs_find_device(fs_info->fs_devices, &args);
 	if (!device) {
 		btrfs_info(fs_info, "resizer unable to find device %llu",
 			   devid);
@@ -3317,22 +3319,21 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info,
 static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
 				 void __user *arg)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct btrfs_ioctl_dev_info_args *di_args;
 	struct btrfs_device *dev;
 	int ret = 0;
-	char *s_uuid = NULL;
 
 	di_args = memdup_user(arg, sizeof(*di_args));
 	if (IS_ERR(di_args))
 		return PTR_ERR(di_args);
 
+	args.devid = di_args->devid;
 	if (!btrfs_is_empty_uuid(di_args->uuid))
-		s_uuid = di_args->uuid;
+		args.uuid = di_args->uuid;
 
 	rcu_read_lock();
-	dev = btrfs_find_device(fs_info->fs_devices, di_args->devid, s_uuid,
-				NULL);
-
+	dev = btrfs_find_device(fs_info->fs_devices, &args);
 	if (!dev) {
 		ret = -ENODEV;
 		goto out;
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index bd3cd7427391..1e29b9aa45df 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -4067,6 +4067,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
 		    u64 end, struct btrfs_scrub_progress *progress,
 		    int readonly, int is_dev_replace)
 {
+	struct btrfs_dev_lookup_args args = { .devid = devid };
 	struct scrub_ctx *sctx;
 	int ret;
 	struct btrfs_device *dev;
@@ -4114,7 +4115,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
 		goto out_free_ctx;
 
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
+	dev = btrfs_find_device(fs_info->fs_devices, &args);
 	if (!dev || (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) &&
 		     !is_dev_replace)) {
 		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
@@ -4287,11 +4288,12 @@ int btrfs_scrub_cancel_dev(struct btrfs_device *dev)
 int btrfs_scrub_progress(struct btrfs_fs_info *fs_info, u64 devid,
 			 struct btrfs_scrub_progress *progress)
 {
+	struct btrfs_dev_lookup_args args = { .devid = devid };
 	struct btrfs_device *dev;
 	struct scrub_ctx *sctx = NULL;
 
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
+	dev = btrfs_find_device(fs_info->fs_devices, &args);
 	if (dev)
 		sctx = dev->scrub_ctx;
 	if (sctx)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5f19d0863094..a729f532749d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -812,9 +812,13 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 
 		device = NULL;
 	} else {
+		struct btrfs_dev_lookup_args args = {
+			.devid = devid,
+			.uuid = disk_super->dev_item.uuid,
+		};
+
 		mutex_lock(&fs_devices->device_list_mutex);
-		device = btrfs_find_device(fs_devices, devid,
-				disk_super->dev_item.uuid, NULL);
+		device = btrfs_find_device(fs_devices, &args);
 
 		/*
 		 * If this disk has been pulled into an fs devices created by
@@ -2323,10 +2327,9 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
 static struct btrfs_device *btrfs_find_device_by_path(
 		struct btrfs_fs_info *fs_info, const char *device_path)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	int ret = 0;
 	struct btrfs_super_block *disk_super;
-	u64 devid;
-	u8 *dev_uuid;
 	struct block_device *bdev;
 	struct btrfs_device *device;
 
@@ -2335,14 +2338,14 @@ static struct btrfs_device *btrfs_find_device_by_path(
 	if (ret)
 		return ERR_PTR(ret);
 
-	devid = btrfs_stack_device_id(&disk_super->dev_item);
-	dev_uuid = disk_super->dev_item.uuid;
+	args.devid = btrfs_stack_device_id(&disk_super->dev_item);
+	args.uuid = disk_super->dev_item.uuid;
 	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
-		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->metadata_uuid);
+		args.fsid = disk_super->metadata_uuid;
 	else
-		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   disk_super->fsid);
+		args.fsid = disk_super->fsid;
+
+	device = btrfs_find_device(fs_info->fs_devices, &args);
 
 	btrfs_release_disk_super(disk_super);
 	if (!device)
@@ -2358,11 +2361,12 @@ struct btrfs_device *btrfs_find_device_by_devspec(
 		struct btrfs_fs_info *fs_info, u64 devid,
 		const char *device_path)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct btrfs_device *device;
 
 	if (devid) {
-		device = btrfs_find_device(fs_info->fs_devices, devid, NULL,
-					   NULL);
+		args.devid = devid;
+		device = btrfs_find_device(fs_info->fs_devices, &args);
 		if (!device)
 			return ERR_PTR(-ENOENT);
 		return device;
@@ -2372,14 +2376,11 @@ struct btrfs_device *btrfs_find_device_by_devspec(
 		return ERR_PTR(-EINVAL);
 
 	if (strcmp(device_path, "missing") == 0) {
-		/* Find first missing device */
-		list_for_each_entry(device, &fs_info->fs_devices->devices,
-				    dev_list) {
-			if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
-				     &device->dev_state) && !device->bdev)
-				return device;
-		}
-		return ERR_PTR(-ENOENT);
+		args.missing = true;
+		device = btrfs_find_device(fs_info->fs_devices, &args);
+		if (!device)
+			return ERR_PTR(-ENOENT);
+		return device;
 	}
 
 	return btrfs_find_device_by_path(fs_info, device_path);
@@ -2459,6 +2460,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
  */
 static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_root *root = fs_info->chunk_root;
 	struct btrfs_path *path;
@@ -2468,7 +2470,6 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 	struct btrfs_key key;
 	u8 fs_uuid[BTRFS_FSID_SIZE];
 	u8 dev_uuid[BTRFS_UUID_SIZE];
-	u64 devid;
 	int ret;
 
 	path = btrfs_alloc_path();
@@ -2505,13 +2506,14 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 
 		dev_item = btrfs_item_ptr(leaf, path->slots[0],
 					  struct btrfs_dev_item);
-		devid = btrfs_device_id(leaf, dev_item);
+		args.devid = btrfs_device_id(leaf, dev_item);
 		read_extent_buffer(leaf, dev_uuid, btrfs_device_uuid(dev_item),
 				   BTRFS_UUID_SIZE);
 		read_extent_buffer(leaf, fs_uuid, btrfs_device_fsid(dev_item),
 				   BTRFS_FSID_SIZE);
-		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-					   fs_uuid);
+		args.uuid = dev_uuid;
+		args.fsid = fs_uuid;
+		device = btrfs_find_device(fs_info->fs_devices, &args);
 		BUG_ON(!device); /* Logic error */
 
 		if (device->fs_devices->seeding) {
@@ -6753,6 +6755,32 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 	return BLK_STS_OK;
 }
 
+static inline bool dev_args_match_fs_devices(struct btrfs_dev_lookup_args *args,
+					     struct btrfs_fs_devices *fs_devices)
+{
+	if (args->fsid == NULL)
+		return true;
+	if (!memcmp(fs_devices->metadata_uuid, args->fsid, BTRFS_FSID_SIZE))
+		return true;
+	return false;
+}
+
+static inline bool dev_args_match_device(struct btrfs_dev_lookup_args *args,
+					 struct btrfs_device *device)
+{
+	ASSERT((args->devid != (u64)-1) || args->missing);
+	if ((args->devid != (u64)-1) && device->devid != args->devid)
+		return false;
+	if (args->uuid && memcmp(device->uuid, args->uuid, BTRFS_UUID_SIZE))
+		return false;
+	if (!args->missing)
+		return true;
+	if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state) &&
+	    !device->bdev)
+		return true;
+	return false;
+}
+
 /*
  * Find a device specified by @devid or @uuid in the list of @fs_devices, or
  * return NULL.
@@ -6761,30 +6789,24 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
  * only devid is used.
  */
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid)
+				       struct btrfs_dev_lookup_args *args)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *seed_devs;
 
-	if (!fsid || !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
+	if (dev_args_match_fs_devices(args, fs_devices)) {
 		list_for_each_entry(device, &fs_devices->devices, dev_list) {
-			if (device->devid == devid &&
-			    (!uuid || memcmp(device->uuid, uuid,
-					     BTRFS_UUID_SIZE) == 0))
+			if (dev_args_match_device(args, device))
 				return device;
 		}
 	}
 
 	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
-		if (!fsid ||
-		    !memcmp(seed_devs->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
-			list_for_each_entry(device, &seed_devs->devices,
-					    dev_list) {
-				if (device->devid == devid &&
-				    (!uuid || memcmp(device->uuid, uuid,
-						     BTRFS_UUID_SIZE) == 0))
-					return device;
-			}
+		if (!dev_args_match_fs_devices(args, seed_devs))
+			continue;
+		list_for_each_entry(device, &seed_devs->devices, dev_list) {
+			if (dev_args_match_device(args, device))
+				return device;
 		}
 	}
 
@@ -6950,6 +6972,7 @@ static void warn_32bit_meta_chunk(struct btrfs_fs_info *fs_info,
 static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
 			  struct btrfs_chunk *chunk)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct btrfs_fs_info *fs_info = leaf->fs_info;
 	struct extent_map_tree *map_tree = &fs_info->mapping_tree;
 	struct map_lookup *map;
@@ -7026,12 +7049,12 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
 	for (i = 0; i < num_stripes; i++) {
 		map->stripes[i].physical =
 			btrfs_stripe_offset_nr(leaf, chunk, i);
-		devid = btrfs_stripe_devid_nr(leaf, chunk, i);
+		devid = args.devid = btrfs_stripe_devid_nr(leaf, chunk, i);
 		read_extent_buffer(leaf, uuid, (unsigned long)
 				   btrfs_stripe_dev_uuid_nr(chunk, i),
 				   BTRFS_UUID_SIZE);
-		map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices,
-							devid, uuid, NULL);
+		args.uuid = uuid;
+		map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices, &args);
 		if (!map->stripes[i].dev &&
 		    !btrfs_test_opt(fs_info, DEGRADED)) {
 			free_extent_map(em);
@@ -7149,6 +7172,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
 static int read_one_dev(struct extent_buffer *leaf,
 			struct btrfs_dev_item *dev_item)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct btrfs_fs_info *fs_info = leaf->fs_info;
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	struct btrfs_device *device;
@@ -7157,11 +7181,13 @@ static int read_one_dev(struct extent_buffer *leaf,
 	u8 fs_uuid[BTRFS_FSID_SIZE];
 	u8 dev_uuid[BTRFS_UUID_SIZE];
 
-	devid = btrfs_device_id(leaf, dev_item);
+	devid = args.devid = btrfs_device_id(leaf, dev_item);
 	read_extent_buffer(leaf, dev_uuid, btrfs_device_uuid(dev_item),
 			   BTRFS_UUID_SIZE);
 	read_extent_buffer(leaf, fs_uuid, btrfs_device_fsid(dev_item),
 			   BTRFS_FSID_SIZE);
+	args.uuid = dev_uuid;
+	args.fsid = fs_uuid;
 
 	if (memcmp(fs_uuid, fs_devices->metadata_uuid, BTRFS_FSID_SIZE)) {
 		fs_devices = open_seed_devices(fs_info, fs_uuid);
@@ -7169,8 +7195,7 @@ static int read_one_dev(struct extent_buffer *leaf,
 			return PTR_ERR(fs_devices);
 	}
 
-	device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
-				   fs_uuid);
+	device = btrfs_find_device(fs_info->fs_devices, &args);
 	if (!device) {
 		if (!btrfs_test_opt(fs_info, DEGRADED)) {
 			btrfs_report_missing_device(fs_info, devid,
@@ -7839,12 +7864,14 @@ static void btrfs_dev_stat_print_on_load(struct btrfs_device *dev)
 int btrfs_get_dev_stats(struct btrfs_fs_info *fs_info,
 			struct btrfs_ioctl_get_dev_stats *stats)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct btrfs_device *dev;
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	int i;
 
 	mutex_lock(&fs_devices->device_list_mutex);
-	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL);
+	args.devid = stats->devid;
+	dev = btrfs_find_device(fs_info->fs_devices, &args);
 	mutex_unlock(&fs_devices->device_list_mutex);
 
 	if (!dev) {
@@ -7920,6 +7947,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 				 u64 chunk_offset, u64 devid,
 				 u64 physical_offset, u64 physical_len)
 {
+	struct btrfs_dev_lookup_args args = { .devid = devid };
 	struct extent_map_tree *em_tree = &fs_info->mapping_tree;
 	struct extent_map *em;
 	struct map_lookup *map;
@@ -7975,7 +8003,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 	}
 
 	/* Make sure no dev extent is beyond device boundary */
-	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
+	dev = btrfs_find_device(fs_info->fs_devices, &args);
 	if (!dev) {
 		btrfs_err(fs_info, "failed to find devid %llu", devid);
 		ret = -EUCLEAN;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index c7ac43d8a7e8..fa9a56c37d45 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -452,6 +452,19 @@ struct btrfs_balance_control {
 	struct btrfs_balance_progress stat;
 };
 
+struct btrfs_dev_lookup_args {
+	u64 devid;
+	u8 *uuid;
+	u8 *fsid;
+	bool missing;
+};
+
+/* We have to init to -1 because BTRFS_DEV_REPLACE_DEVID is 0 */
+#define BTRFS_DEV_LOOKUP_ARGS_INIT { .devid = (u64)-1 }
+
+#define BTRFS_DEV_LOOKUP_ARGS(name) \
+	struct btrfs_dev_lookup_args name = BTRFS_DEV_LOOKUP_ARGS_INIT
+
 enum btrfs_map_op {
 	BTRFS_MAP_READ,
 	BTRFS_MAP_WRITE,
@@ -517,7 +530,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
 int btrfs_grow_device(struct btrfs_trans_handle *trans,
 		      struct btrfs_device *device, u64 new_size);
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
-				       u64 devid, u8 *uuid, u8 *fsid);
+				       struct btrfs_dev_lookup_args *args);
 int btrfs_shrink_device(struct btrfs_device *device, u64 new_size);
 int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
 int btrfs_balance(struct btrfs_fs_info *fs_info,
-- 
2.26.3

Re: [PATCH v4 4/6] btrfs: handle device lookup with btrfs_dev_lookup_args

[Nikolay Borisov]

On 5.10.21 г. 23:12, Josef Bacik wrote:
> We have a lot of device lookup functions that all do something slightly
> different.  Clean this up by adding a struct to hold the different
> lookup criteria, and then pass this around to btrfs_find_device() so it
> can do the proper matching based on the lookup criteria.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Overall looks good, I have only some minor stylistic comments which can
be handled by David as well. In any case:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/dev-replace.c |  18 +++----
>  fs/btrfs/ioctl.c       |  13 ++---
>  fs/btrfs/scrub.c       |   6 ++-
>  fs/btrfs/volumes.c     | 120 +++++++++++++++++++++++++----------------
>  fs/btrfs/volumes.h     |  15 +++++-
>  5 files changed, 108 insertions(+), 64 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index d029be40ea6f..ff25da2dbd59 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -70,6 +70,7 @@ static int btrfs_dev_replace_kthread(void *data);
>  
>  int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>  {
> +	BTRFS_DEV_LOOKUP_ARGS(args);

nit: This line can be:

struct btrfs_dev_lookup_args args = {.devid = BTRFS_DEV_REPLACE_DEVID};

as it doesn't go over the 76 char limit, the only reason I'm suggesting
it is for consistency sake, since in
btrfs_scrub_progress/btrfs_scrub_dev the latter style is preferred.

>  	struct btrfs_key key;
>  	struct btrfs_root *dev_root = fs_info->dev_root;
>  	struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
> @@ -84,6 +85,8 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>  	if (!dev_root)
>  		return 0;
>  
> +	args.devid = BTRFS_DEV_REPLACE_DEVID;
> +
>  	path = btrfs_alloc_path();
>  	if (!path) {
>  		ret = -ENOMEM;

<snip>

> @@ -6753,6 +6755,32 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>  	return BLK_STS_OK;
>  }
>  
> +static inline bool dev_args_match_fs_devices(struct btrfs_dev_lookup_args *args,
> +					     struct btrfs_fs_devices *fs_devices)
> +{
> +	if (args->fsid == NULL)
> +		return true;
> +	if (!memcmp(fs_devices->metadata_uuid, args->fsid, BTRFS_FSID_SIZE))
> +		return true;
> +	return false;

Make last 3 lines into:

return !memcmp(fs_devices->metadata_uuid, args->fsid, BTRFS_FSID_SIZE)

> +}
> +
> +static inline bool dev_args_match_device(struct btrfs_dev_lookup_args *args,
> +					 struct btrfs_device *device)
> +{
> +	ASSERT((args->devid != (u64)-1) || args->missing);
> +	if ((args->devid != (u64)-1) && device->devid != args->devid)
> +		return false;
> +	if (args->uuid && memcmp(device->uuid, args->uuid, BTRFS_UUID_SIZE))
> +		return false;
> +	if (!args->missing)
> +		return true;
> +	if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state) &&
> +	    !device->bdev)

That's a new check, under what conditions does it happen, when a device
is forcefully removed from the system?

> +		return true;
> +	return false;
> +}
> +
>  /*
>   * Find a device specified by @devid or @uuid in the list of @fs_devices, or
>   * return NULL.

<snip>

> @@ -517,7 +530,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
>  int btrfs_grow_device(struct btrfs_trans_handle *trans,
>  		      struct btrfs_device *device, u64 new_size);
>  struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
> -				       u64 devid, u8 *uuid, u8 *fsid);
> +				       struct btrfs_dev_lookup_args *args);

Let's annotate this pointer with const to be more explicit about this
being really an input-only struct.

>  int btrfs_shrink_device(struct btrfs_device *device, u64 new_size);
>  int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
>  int btrfs_balance(struct btrfs_fs_info *fs_info,
> 

Re: [PATCH v4 4/6] btrfs: handle device lookup with btrfs_dev_lookup_args

[David Sterba]

On Wed, Oct 06, 2021 at 10:34:41AM +0300, Nikolay Borisov wrote:
> >  int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
> >  {
> > +	BTRFS_DEV_LOOKUP_ARGS(args);
> 
> nit: This line can be:
> 
> struct btrfs_dev_lookup_args args = {.devid = BTRFS_DEV_REPLACE_DEVID};
> 
> as it doesn't go over the 76 char limit, the only reason I'm suggesting

The limit is 80-85.

> it is for consistency sake, since in
> btrfs_scrub_progress/btrfs_scrub_dev the latter style is preferred.

Agreed, after seeing the rest of the patchset, if the members (most
often devid) can be set right away like from the parameters, the
explicit initialization should be used, so I've switched it to what
you've suggested.

> > +static inline bool dev_args_match_fs_devices(struct btrfs_dev_lookup_args *args,

Btw static inline that's not in a header does not make much sense, so
it's plain static. Also for read-only parameters it's a good habit to
declare them const, also changed.

> > +					     struct btrfs_fs_devices *fs_devices)
> > +{
> > +	if (args->fsid == NULL)
> > +		return true;
> > +	if (!memcmp(fs_devices->metadata_uuid, args->fsid, BTRFS_FSID_SIZE))
> > +		return true;
> > +	return false;
> 
> Make last 3 lines into:
> 
> return !memcmp(fs_devices->metadata_uuid, args->fsid, BTRFS_FSID_SIZE)

I really don't like the short form where memcmp (or strcmp for that
matter) is done like !memcmp as it reads "if the don't compare as
equeal" and requires the mental switch to if memcmp() == 0 "they're
equal", so I've been switching that to the == 0 form whenever I see it.

In this function, if ther's a chain of if/if/return, it reads better if
all the branches are either implicit or explicit, so mixing both styles
is slightly less preferred.

> > @@ -517,7 +530,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
> >  int btrfs_grow_device(struct btrfs_trans_handle *trans,
> >  		      struct btrfs_device *device, u64 new_size);
> >  struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
> > -				       u64 devid, u8 *uuid, u8 *fsid);
> > +				       struct btrfs_dev_lookup_args *args);
> 
> Let's annotate this pointer with const to be more explicit about this
> being really an input-only struct.

Agreed, const added.

Re: [PATCH v4 4/6] btrfs: handle device lookup with btrfs_dev_lookup_args

[Anand Jain]

On 06/10/2021 04:12, Josef Bacik wrote:
> We have a lot of device lookup functions that all do something slightly
> different.  Clean this up by adding a struct to hold the different
> lookup criteria, and then pass this around to btrfs_find_device() so it
> can do the proper matching based on the lookup criteria.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

> ---
>   fs/btrfs/dev-replace.c |  18 +++----
>   fs/btrfs/ioctl.c       |  13 ++---
>   fs/btrfs/scrub.c       |   6 ++-
>   fs/btrfs/volumes.c     | 120 +++++++++++++++++++++++++----------------
>   fs/btrfs/volumes.h     |  15 +++++-
>   5 files changed, 108 insertions(+), 64 deletions(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index d029be40ea6f..ff25da2dbd59 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -70,6 +70,7 @@ static int btrfs_dev_replace_kthread(void *data);
>   
>   int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct btrfs_key key;
>   	struct btrfs_root *dev_root = fs_info->dev_root;
>   	struct btrfs_dev_replace *dev_replace = &fs_info->dev_replace;
> @@ -84,6 +85,8 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>   	if (!dev_root)
>   		return 0;
>   
> +	args.devid = BTRFS_DEV_REPLACE_DEVID;
> +
>   	path = btrfs_alloc_path();
>   	if (!path) {
>   		ret = -ENOMEM;
> @@ -100,8 +103,7 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>   		 * We don't have a replace item or it's corrupted.  If there is
>   		 * a replace target, fail the mount.
>   		 */
> -		if (btrfs_find_device(fs_info->fs_devices,
> -				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
> +		if (btrfs_find_device(fs_info->fs_devices, &args)) {
>   			btrfs_err(fs_info,
>   			"found replace target device without a valid replace item");
>   			ret = -EUCLEAN;
> @@ -163,8 +165,7 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>   		 * We don't have an active replace item but if there is a
>   		 * replace target, fail the mount.
>   		 */
> -		if (btrfs_find_device(fs_info->fs_devices,
> -				      BTRFS_DEV_REPLACE_DEVID, NULL, NULL)) {
> +		if (btrfs_find_device(fs_info->fs_devices, &args)) {
>   			btrfs_err(fs_info,
>   			"replace devid present without an active replace item");
>   			ret = -EUCLEAN;
> @@ -175,11 +176,10 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
>   		break;
>   	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
>   	case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED:
> -		dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices,
> -						src_devid, NULL, NULL);
> -		dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices,
> -							BTRFS_DEV_REPLACE_DEVID,
> -							NULL, NULL);
> +		dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices, &args);
> +		args.devid = src_devid;
> +		dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices, &args);
> +
>   		/*
>   		 * allow 'btrfs dev replace_cancel' if src/tgt device is
>   		 * missing
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9eb0c1eb568e..b8508af4e539 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1602,6 +1602,7 @@ static int exclop_start_or_cancel_reloc(struct btrfs_fs_info *fs_info,
>   static noinline int btrfs_ioctl_resize(struct file *file,
>   					void __user *arg)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct inode *inode = file_inode(file);
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   	u64 new_size;
> @@ -1657,7 +1658,8 @@ static noinline int btrfs_ioctl_resize(struct file *file,
>   		btrfs_info(fs_info, "resizing devid %llu", devid);
>   	}
>   
> -	device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
> +	args.devid = devid;
> +	device = btrfs_find_device(fs_info->fs_devices, &args);
>   	if (!device) {
>   		btrfs_info(fs_info, "resizer unable to find device %llu",
>   			   devid);
> @@ -3317,22 +3319,21 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info,
>   static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
>   				 void __user *arg)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct btrfs_ioctl_dev_info_args *di_args;
>   	struct btrfs_device *dev;
>   	int ret = 0;
> -	char *s_uuid = NULL;
>   
>   	di_args = memdup_user(arg, sizeof(*di_args));
>   	if (IS_ERR(di_args))
>   		return PTR_ERR(di_args);
>   
> +	args.devid = di_args->devid;
>   	if (!btrfs_is_empty_uuid(di_args->uuid))
> -		s_uuid = di_args->uuid;
> +		args.uuid = di_args->uuid;
>   
>   	rcu_read_lock();
> -	dev = btrfs_find_device(fs_info->fs_devices, di_args->devid, s_uuid,
> -				NULL);
> -
> +	dev = btrfs_find_device(fs_info->fs_devices, &args);
>   	if (!dev) {
>   		ret = -ENODEV;
>   		goto out;
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index bd3cd7427391..1e29b9aa45df 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -4067,6 +4067,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
>   		    u64 end, struct btrfs_scrub_progress *progress,
>   		    int readonly, int is_dev_replace)
>   {
> +	struct btrfs_dev_lookup_args args = { .devid = devid };
>   	struct scrub_ctx *sctx;
>   	int ret;
>   	struct btrfs_device *dev;
> @@ -4114,7 +4115,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
>   		goto out_free_ctx;
>   
>   	mutex_lock(&fs_info->fs_devices->device_list_mutex);
> -	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
> +	dev = btrfs_find_device(fs_info->fs_devices, &args);
>   	if (!dev || (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) &&
>   		     !is_dev_replace)) {
>   		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
> @@ -4287,11 +4288,12 @@ int btrfs_scrub_cancel_dev(struct btrfs_device *dev)
>   int btrfs_scrub_progress(struct btrfs_fs_info *fs_info, u64 devid,
>   			 struct btrfs_scrub_progress *progress)
>   {
> +	struct btrfs_dev_lookup_args args = { .devid = devid };
>   	struct btrfs_device *dev;
>   	struct scrub_ctx *sctx = NULL;
>   
>   	mutex_lock(&fs_info->fs_devices->device_list_mutex);
> -	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
> +	dev = btrfs_find_device(fs_info->fs_devices, &args);
>   	if (dev)
>   		sctx = dev->scrub_ctx;
>   	if (sctx)
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5f19d0863094..a729f532749d 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -812,9 +812,13 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   
>   		device = NULL;
>   	} else {
> +		struct btrfs_dev_lookup_args args = {
> +			.devid = devid,
> +			.uuid = disk_super->dev_item.uuid,
> +		};
> +
>   		mutex_lock(&fs_devices->device_list_mutex);
> -		device = btrfs_find_device(fs_devices, devid,
> -				disk_super->dev_item.uuid, NULL);
> +		device = btrfs_find_device(fs_devices, &args);
>   
>   		/*
>   		 * If this disk has been pulled into an fs devices created by
> @@ -2323,10 +2327,9 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
>   static struct btrfs_device *btrfs_find_device_by_path(
>   		struct btrfs_fs_info *fs_info, const char *device_path)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	int ret = 0;
>   	struct btrfs_super_block *disk_super;
> -	u64 devid;
> -	u8 *dev_uuid;
>   	struct block_device *bdev;
>   	struct btrfs_device *device;
>   
> @@ -2335,14 +2338,14 @@ static struct btrfs_device *btrfs_find_device_by_path(
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> -	devid = btrfs_stack_device_id(&disk_super->dev_item);
> -	dev_uuid = disk_super->dev_item.uuid;
> +	args.devid = btrfs_stack_device_id(&disk_super->dev_item);
> +	args.uuid = disk_super->dev_item.uuid;
>   	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
> -		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
> -					   disk_super->metadata_uuid);
> +		args.fsid = disk_super->metadata_uuid;
>   	else
> -		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
> -					   disk_super->fsid);
> +		args.fsid = disk_super->fsid;
> +
> +	device = btrfs_find_device(fs_info->fs_devices, &args);
>   
>   	btrfs_release_disk_super(disk_super);
>   	if (!device)
> @@ -2358,11 +2361,12 @@ struct btrfs_device *btrfs_find_device_by_devspec(
>   		struct btrfs_fs_info *fs_info, u64 devid,
>   		const char *device_path)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct btrfs_device *device;
>   
>   	if (devid) {
> -		device = btrfs_find_device(fs_info->fs_devices, devid, NULL,
> -					   NULL);
> +		args.devid = devid;
> +		device = btrfs_find_device(fs_info->fs_devices, &args);
>   		if (!device)
>   			return ERR_PTR(-ENOENT);
>   		return device;
> @@ -2372,14 +2376,11 @@ struct btrfs_device *btrfs_find_device_by_devspec(
>   		return ERR_PTR(-EINVAL);
>   
>   	if (strcmp(device_path, "missing") == 0) {
> -		/* Find first missing device */
> -		list_for_each_entry(device, &fs_info->fs_devices->devices,
> -				    dev_list) {
> -			if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA,
> -				     &device->dev_state) && !device->bdev)
> -				return device;
> -		}
> -		return ERR_PTR(-ENOENT);
> +		args.missing = true;
> +		device = btrfs_find_device(fs_info->fs_devices, &args);
> +		if (!device)
> +			return ERR_PTR(-ENOENT);
> +		return device;
>   	}
>   
>   	return btrfs_find_device_by_path(fs_info, device_path);
> @@ -2459,6 +2460,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info)
>    */
>   static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct btrfs_fs_info *fs_info = trans->fs_info;
>   	struct btrfs_root *root = fs_info->chunk_root;
>   	struct btrfs_path *path;
> @@ -2468,7 +2470,6 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
>   	struct btrfs_key key;
>   	u8 fs_uuid[BTRFS_FSID_SIZE];
>   	u8 dev_uuid[BTRFS_UUID_SIZE];
> -	u64 devid;
>   	int ret;
>   
>   	path = btrfs_alloc_path();
> @@ -2505,13 +2506,14 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
>   
>   		dev_item = btrfs_item_ptr(leaf, path->slots[0],
>   					  struct btrfs_dev_item);
> -		devid = btrfs_device_id(leaf, dev_item);
> +		args.devid = btrfs_device_id(leaf, dev_item);
>   		read_extent_buffer(leaf, dev_uuid, btrfs_device_uuid(dev_item),
>   				   BTRFS_UUID_SIZE);
>   		read_extent_buffer(leaf, fs_uuid, btrfs_device_fsid(dev_item),
>   				   BTRFS_FSID_SIZE);
> -		device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
> -					   fs_uuid);
> +		args.uuid = dev_uuid;
> +		args.fsid = fs_uuid;
> +		device = btrfs_find_device(fs_info->fs_devices, &args);
>   		BUG_ON(!device); /* Logic error */
>   
>   		if (device->fs_devices->seeding) {
> @@ -6753,6 +6755,32 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>   	return BLK_STS_OK;
>   }
>   
> +static inline bool dev_args_match_fs_devices(struct btrfs_dev_lookup_args *args,
> +					     struct btrfs_fs_devices *fs_devices)
> +{
> +	if (args->fsid == NULL)
> +		return true;
> +	if (!memcmp(fs_devices->metadata_uuid, args->fsid, BTRFS_FSID_SIZE))
> +		return true;
> +	return false;
> +}
> +
> +static inline bool dev_args_match_device(struct btrfs_dev_lookup_args *args,
> +					 struct btrfs_device *device)
> +{
> +	ASSERT((args->devid != (u64)-1) || args->missing);
> +	if ((args->devid != (u64)-1) && device->devid != args->devid)
> +		return false;
> +	if (args->uuid && memcmp(device->uuid, args->uuid, BTRFS_UUID_SIZE))
> +		return false;
> +	if (!args->missing)
> +		return true;
> +	if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state) &&
> +	    !device->bdev)
> +		return true;
> +	return false;
> +}
> +
>   /*
>    * Find a device specified by @devid or @uuid in the list of @fs_devices, or
>    * return NULL.
> @@ -6761,30 +6789,24 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>    * only devid is used.
>    */
>   struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
> -				       u64 devid, u8 *uuid, u8 *fsid)
> +				       struct btrfs_dev_lookup_args *args)
>   {
>   	struct btrfs_device *device;
>   	struct btrfs_fs_devices *seed_devs;
>   
> -	if (!fsid || !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
> +	if (dev_args_match_fs_devices(args, fs_devices)) {
>   		list_for_each_entry(device, &fs_devices->devices, dev_list) {
> -			if (device->devid == devid &&
> -			    (!uuid || memcmp(device->uuid, uuid,
> -					     BTRFS_UUID_SIZE) == 0))
> +			if (dev_args_match_device(args, device))
>   				return device;
>   		}
>   	}
>   
>   	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
> -		if (!fsid ||
> -		    !memcmp(seed_devs->metadata_uuid, fsid, BTRFS_FSID_SIZE)) {
> -			list_for_each_entry(device, &seed_devs->devices,
> -					    dev_list) {
> -				if (device->devid == devid &&
> -				    (!uuid || memcmp(device->uuid, uuid,
> -						     BTRFS_UUID_SIZE) == 0))
> -					return device;
> -			}
> +		if (!dev_args_match_fs_devices(args, seed_devs))
> +			continue;
> +		list_for_each_entry(device, &seed_devs->devices, dev_list) {
> +			if (dev_args_match_device(args, device))
> +				return device;
>   		}
>   	}
>   
> @@ -6950,6 +6972,7 @@ static void warn_32bit_meta_chunk(struct btrfs_fs_info *fs_info,
>   static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
>   			  struct btrfs_chunk *chunk)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct btrfs_fs_info *fs_info = leaf->fs_info;
>   	struct extent_map_tree *map_tree = &fs_info->mapping_tree;
>   	struct map_lookup *map;
> @@ -7026,12 +7049,12 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf,
>   	for (i = 0; i < num_stripes; i++) {
>   		map->stripes[i].physical =
>   			btrfs_stripe_offset_nr(leaf, chunk, i);
> -		devid = btrfs_stripe_devid_nr(leaf, chunk, i);
> +		devid = args.devid = btrfs_stripe_devid_nr(leaf, chunk, i);
>   		read_extent_buffer(leaf, uuid, (unsigned long)
>   				   btrfs_stripe_dev_uuid_nr(chunk, i),
>   				   BTRFS_UUID_SIZE);
> -		map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices,
> -							devid, uuid, NULL);
> +		args.uuid = uuid;
> +		map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices, &args);
>   		if (!map->stripes[i].dev &&
>   		    !btrfs_test_opt(fs_info, DEGRADED)) {
>   			free_extent_map(em);
> @@ -7149,6 +7172,7 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info,
>   static int read_one_dev(struct extent_buffer *leaf,
>   			struct btrfs_dev_item *dev_item)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct btrfs_fs_info *fs_info = leaf->fs_info;
>   	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>   	struct btrfs_device *device;
> @@ -7157,11 +7181,13 @@ static int read_one_dev(struct extent_buffer *leaf,
>   	u8 fs_uuid[BTRFS_FSID_SIZE];
>   	u8 dev_uuid[BTRFS_UUID_SIZE];
>   
> -	devid = btrfs_device_id(leaf, dev_item);
> +	devid = args.devid = btrfs_device_id(leaf, dev_item);
>   	read_extent_buffer(leaf, dev_uuid, btrfs_device_uuid(dev_item),
>   			   BTRFS_UUID_SIZE);
>   	read_extent_buffer(leaf, fs_uuid, btrfs_device_fsid(dev_item),
>   			   BTRFS_FSID_SIZE);
> +	args.uuid = dev_uuid;
> +	args.fsid = fs_uuid;
>   
>   	if (memcmp(fs_uuid, fs_devices->metadata_uuid, BTRFS_FSID_SIZE)) {
>   		fs_devices = open_seed_devices(fs_info, fs_uuid);
> @@ -7169,8 +7195,7 @@ static int read_one_dev(struct extent_buffer *leaf,
>   			return PTR_ERR(fs_devices);
>   	}
>   
> -	device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid,
> -				   fs_uuid);
> +	device = btrfs_find_device(fs_info->fs_devices, &args);
>   	if (!device) {
>   		if (!btrfs_test_opt(fs_info, DEGRADED)) {
>   			btrfs_report_missing_device(fs_info, devid,
> @@ -7839,12 +7864,14 @@ static void btrfs_dev_stat_print_on_load(struct btrfs_device *dev)
>   int btrfs_get_dev_stats(struct btrfs_fs_info *fs_info,
>   			struct btrfs_ioctl_get_dev_stats *stats)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct btrfs_device *dev;
>   	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>   	int i;
>   
>   	mutex_lock(&fs_devices->device_list_mutex);
> -	dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL);
> +	args.devid = stats->devid;
> +	dev = btrfs_find_device(fs_info->fs_devices, &args);
>   	mutex_unlock(&fs_devices->device_list_mutex);
>   
>   	if (!dev) {
> @@ -7920,6 +7947,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>   				 u64 chunk_offset, u64 devid,
>   				 u64 physical_offset, u64 physical_len)
>   {
> +	struct btrfs_dev_lookup_args args = { .devid = devid };
>   	struct extent_map_tree *em_tree = &fs_info->mapping_tree;
>   	struct extent_map *em;
>   	struct map_lookup *map;
> @@ -7975,7 +8003,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>   	}
>   
>   	/* Make sure no dev extent is beyond device boundary */
> -	dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL);
> +	dev = btrfs_find_device(fs_info->fs_devices, &args);
>   	if (!dev) {
>   		btrfs_err(fs_info, "failed to find devid %llu", devid);
>   		ret = -EUCLEAN;
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index c7ac43d8a7e8..fa9a56c37d45 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -452,6 +452,19 @@ struct btrfs_balance_control {
>   	struct btrfs_balance_progress stat;
>   };
>   
> +struct btrfs_dev_lookup_args {
> +	u64 devid;
> +	u8 *uuid;
> +	u8 *fsid;
> +	bool missing;
> +};
> +
> +/* We have to init to -1 because BTRFS_DEV_REPLACE_DEVID is 0 */
> +#define BTRFS_DEV_LOOKUP_ARGS_INIT { .devid = (u64)-1 }
> +
> +#define BTRFS_DEV_LOOKUP_ARGS(name) \
> +	struct btrfs_dev_lookup_args name = BTRFS_DEV_LOOKUP_ARGS_INIT
> +
>   enum btrfs_map_op {
>   	BTRFS_MAP_READ,
>   	BTRFS_MAP_WRITE,
> @@ -517,7 +530,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
>   int btrfs_grow_device(struct btrfs_trans_handle *trans,
>   		      struct btrfs_device *device, u64 new_size);
>   struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
> -				       u64 devid, u8 *uuid, u8 *fsid);
> +				       struct btrfs_dev_lookup_args *args);
>   int btrfs_shrink_device(struct btrfs_device *device, u64 new_size);
>   int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path);
>   int btrfs_balance(struct btrfs_fs_info *fs_info,
> 

[PATCH v4 5/6] btrfs: add a btrfs_get_dev_args_from_path helper

[Josef Bacik]

We are going to want to populate our device lookup args outside of any
locks and then do the actual device lookup later, so add a helper to do
this work and make btrfs_find_device_by_devspec() use this helper for
now.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/volumes.c | 95 ++++++++++++++++++++++++++++++----------------
 fs/btrfs/volumes.h |  4 ++
 2 files changed, 67 insertions(+), 32 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a729f532749d..e490414e8987 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2324,45 +2324,80 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
 	btrfs_free_device(tgtdev);
 }
 
-static struct btrfs_device *btrfs_find_device_by_path(
-		struct btrfs_fs_info *fs_info, const char *device_path)
+/**
+ * btrfs_get_dev_args_from_path - populate args from device at path
+ *
+ * @fs_info: the filesystem
+ * @args: the args to populate
+ * @path: the path to the device
+ * Return: 0 for success, -errno for failure
+ *
+ * This will read the super block of the device at @path and populate @args with
+ * the devid, fsid, and uuid.  This is meant to be used for ioctl's that need to
+ * lookup a device to operate on, but need to do it before we take any locks.
+ * This properly handles the special case of "missing" that a user may pass in,
+ * and does some basic sanity checks.  The caller must make sure that @path is
+ * properly NULL terminated before calling in, and must call
+ * btrfs_put_dev_args_from_path() in order to free up the temporary fsid and
+ * uuid buffers.
+ */
+int btrfs_get_dev_args_from_path(struct btrfs_fs_info *fs_info,
+				 struct btrfs_dev_lookup_args *args,
+				 const char *path)
 {
-	BTRFS_DEV_LOOKUP_ARGS(args);
-	int ret = 0;
 	struct btrfs_super_block *disk_super;
 	struct block_device *bdev;
-	struct btrfs_device *device;
+	int ret;
 
-	ret = btrfs_get_bdev_and_sb(device_path, FMODE_READ,
-				    fs_info->bdev_holder, 0, &bdev, &disk_super);
-	if (ret)
-		return ERR_PTR(ret);
+	if (!path || !path[0])
+		return -EINVAL;
+	if (!strcmp(path, "missing")) {
+		args->missing = true;
+		return 0;
+	}
+
+	args->uuid = kzalloc(BTRFS_UUID_SIZE, GFP_KERNEL);
+	args->fsid = kzalloc(BTRFS_FSID_SIZE, GFP_KERNEL);
+	if (!args->uuid || !args->fsid) {
+		btrfs_put_dev_args_from_path(args);
+		return -ENOMEM;
+	}
 
-	args.devid = btrfs_stack_device_id(&disk_super->dev_item);
-	args.uuid = disk_super->dev_item.uuid;
+	ret = btrfs_get_bdev_and_sb(path, FMODE_READ, fs_info->bdev_holder, 0,
+				    &bdev, &disk_super);
+	if (ret)
+		return ret;
+	args->devid = btrfs_stack_device_id(&disk_super->dev_item);
+	memcpy(args->uuid, disk_super->dev_item.uuid, BTRFS_UUID_SIZE);
 	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
-		args.fsid = disk_super->metadata_uuid;
+		memcpy(args->fsid, disk_super->metadata_uuid, BTRFS_FSID_SIZE);
 	else
-		args.fsid = disk_super->fsid;
-
-	device = btrfs_find_device(fs_info->fs_devices, &args);
-
+		memcpy(args->fsid, disk_super->fsid, BTRFS_FSID_SIZE);
 	btrfs_release_disk_super(disk_super);
-	if (!device)
-		device = ERR_PTR(-ENOENT);
 	blkdev_put(bdev, FMODE_READ);
-	return device;
+	return 0;
 }
 
 /*
- * Lookup a device given by device id, or the path if the id is 0.
+ * Only use this jointly with btrfs_get_dev_args_from_path() because we will
+ * allocate our ->uuid and ->fsid pointers, everybody else uses local variables
+ * that don't need to be freed.
  */
+void btrfs_put_dev_args_from_path(struct btrfs_dev_lookup_args *args)
+{
+	kfree(args->uuid);
+	kfree(args->fsid);
+	args->uuid = NULL;
+	args->fsid = NULL;
+}
+
 struct btrfs_device *btrfs_find_device_by_devspec(
 		struct btrfs_fs_info *fs_info, u64 devid,
 		const char *device_path)
 {
 	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct btrfs_device *device;
+	int ret;
 
 	if (devid) {
 		args.devid = devid;
@@ -2372,18 +2407,14 @@ struct btrfs_device *btrfs_find_device_by_devspec(
 		return device;
 	}
 
-	if (!device_path || !device_path[0])
-		return ERR_PTR(-EINVAL);
-
-	if (strcmp(device_path, "missing") == 0) {
-		args.missing = true;
-		device = btrfs_find_device(fs_info->fs_devices, &args);
-		if (!device)
-			return ERR_PTR(-ENOENT);
-		return device;
-	}
-
-	return btrfs_find_device_by_path(fs_info, device_path);
+	ret = btrfs_get_dev_args_from_path(fs_info, &args, device_path);
+	if (ret)
+		return ERR_PTR(ret);
+	device = btrfs_find_device(fs_info->fs_devices, &args);
+	btrfs_put_dev_args_from_path(&args);
+	if (!device)
+		return ERR_PTR(-ENOENT);
+	return device;
 }
 
 /*
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index fa9a56c37d45..3fe5ac683f98 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -518,9 +518,13 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
 struct btrfs_device *btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info,
 						  u64 devid,
 						  const char *devpath);
+int btrfs_get_dev_args_from_path(struct btrfs_fs_info *fs_info,
+				 struct btrfs_dev_lookup_args *args,
+				 const char *path);
 struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 					const u64 *devid,
 					const u8 *uuid);
+void btrfs_put_dev_args_from_path(struct btrfs_dev_lookup_args *args);
 void btrfs_free_device(struct btrfs_device *device);
 int btrfs_rm_device(struct btrfs_fs_info *fs_info,
 		    const char *device_path, u64 devid,
-- 
2.26.3

Re: [PATCH v4 5/6] btrfs: add a btrfs_get_dev_args_from_path helper

[Nikolay Borisov]

On 5.10.21 г. 23:12, Josef Bacik wrote:
> We are going to want to populate our device lookup args outside of any
> locks and then do the actual device lookup later, so add a helper to do
> this work and make btrfs_find_device_by_devspec() use this helper for
> now.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH v4 5/6] btrfs: add a btrfs_get_dev_args_from_path helper

[Anand Jain]

On 06/10/2021 04:12, Josef Bacik wrote:
> We are going to want to populate our device lookup args outside of any
> locks and then do the actual device lookup later, so add a helper to do
> this work and make btrfs_find_device_by_devspec() use this helper for
> now.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand



> ---
>   fs/btrfs/volumes.c | 95 ++++++++++++++++++++++++++++++----------------
>   fs/btrfs/volumes.h |  4 ++
>   2 files changed, 67 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a729f532749d..e490414e8987 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2324,45 +2324,80 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_device *tgtdev)
>   	btrfs_free_device(tgtdev);
>   }
>   
> -static struct btrfs_device *btrfs_find_device_by_path(
> -		struct btrfs_fs_info *fs_info, const char *device_path)
> +/**
> + * btrfs_get_dev_args_from_path - populate args from device at path
> + *
> + * @fs_info: the filesystem
> + * @args: the args to populate
> + * @path: the path to the device
> + * Return: 0 for success, -errno for failure
> + *
> + * This will read the super block of the device at @path and populate @args with
> + * the devid, fsid, and uuid.  This is meant to be used for ioctl's that need to
> + * lookup a device to operate on, but need to do it before we take any locks.
> + * This properly handles the special case of "missing" that a user may pass in,
> + * and does some basic sanity checks.  The caller must make sure that @path is
> + * properly NULL terminated before calling in, and must call
> + * btrfs_put_dev_args_from_path() in order to free up the temporary fsid and
> + * uuid buffers.
> + */
> +int btrfs_get_dev_args_from_path(struct btrfs_fs_info *fs_info,
> +				 struct btrfs_dev_lookup_args *args,
> +				 const char *path)
>   {
> -	BTRFS_DEV_LOOKUP_ARGS(args);
> -	int ret = 0;
>   	struct btrfs_super_block *disk_super;
>   	struct block_device *bdev;
> -	struct btrfs_device *device;
> +	int ret;
>   
> -	ret = btrfs_get_bdev_and_sb(device_path, FMODE_READ,
> -				    fs_info->bdev_holder, 0, &bdev, &disk_super);
> -	if (ret)
> -		return ERR_PTR(ret);
> +	if (!path || !path[0])
> +		return -EINVAL;
> +	if (!strcmp(path, "missing")) {
> +		args->missing = true;
> +		return 0;
> +	}
> +
> +	args->uuid = kzalloc(BTRFS_UUID_SIZE, GFP_KERNEL);
> +	args->fsid = kzalloc(BTRFS_FSID_SIZE, GFP_KERNEL);
> +	if (!args->uuid || !args->fsid) {
> +		btrfs_put_dev_args_from_path(args);
> +		return -ENOMEM;
> +	}
>   
> -	args.devid = btrfs_stack_device_id(&disk_super->dev_item);
> -	args.uuid = disk_super->dev_item.uuid;
> +	ret = btrfs_get_bdev_and_sb(path, FMODE_READ, fs_info->bdev_holder, 0,
> +				    &bdev, &disk_super);
> +	if (ret)
> +		return ret;
> +	args->devid = btrfs_stack_device_id(&disk_super->dev_item);
> +	memcpy(args->uuid, disk_super->dev_item.uuid, BTRFS_UUID_SIZE);
>   	if (btrfs_fs_incompat(fs_info, METADATA_UUID))
> -		args.fsid = disk_super->metadata_uuid;
> +		memcpy(args->fsid, disk_super->metadata_uuid, BTRFS_FSID_SIZE);
>   	else
> -		args.fsid = disk_super->fsid;
> -
> -	device = btrfs_find_device(fs_info->fs_devices, &args);
> -
> +		memcpy(args->fsid, disk_super->fsid, BTRFS_FSID_SIZE);
>   	btrfs_release_disk_super(disk_super);
> -	if (!device)
> -		device = ERR_PTR(-ENOENT);
>   	blkdev_put(bdev, FMODE_READ);
> -	return device;
> +	return 0;
>   }
>   
>   /*
> - * Lookup a device given by device id, or the path if the id is 0.
> + * Only use this jointly with btrfs_get_dev_args_from_path() because we will
> + * allocate our ->uuid and ->fsid pointers, everybody else uses local variables
> + * that don't need to be freed.
>    */
> +void btrfs_put_dev_args_from_path(struct btrfs_dev_lookup_args *args)
> +{
> +	kfree(args->uuid);
> +	kfree(args->fsid);
> +	args->uuid = NULL;
> +	args->fsid = NULL;
> +}
> +
>   struct btrfs_device *btrfs_find_device_by_devspec(
>   		struct btrfs_fs_info *fs_info, u64 devid,
>   		const char *device_path)
>   {
>   	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct btrfs_device *device;
> +	int ret;
>   
>   	if (devid) {
>   		args.devid = devid;
> @@ -2372,18 +2407,14 @@ struct btrfs_device *btrfs_find_device_by_devspec(
>   		return device;
>   	}
>   
> -	if (!device_path || !device_path[0])
> -		return ERR_PTR(-EINVAL);
> -
> -	if (strcmp(device_path, "missing") == 0) {
> -		args.missing = true;
> -		device = btrfs_find_device(fs_info->fs_devices, &args);
> -		if (!device)
> -			return ERR_PTR(-ENOENT);
> -		return device;
> -	}
> -
> -	return btrfs_find_device_by_path(fs_info, device_path);
> +	ret = btrfs_get_dev_args_from_path(fs_info, &args, device_path);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	device = btrfs_find_device(fs_info->fs_devices, &args);
> +	btrfs_put_dev_args_from_path(&args);
> +	if (!device)
> +		return ERR_PTR(-ENOENT);
> +	return device;
>   }
>   
>   /*
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index fa9a56c37d45..3fe5ac683f98 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -518,9 +518,13 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
>   struct btrfs_device *btrfs_find_device_by_devspec(struct btrfs_fs_info *fs_info,
>   						  u64 devid,
>   						  const char *devpath);
> +int btrfs_get_dev_args_from_path(struct btrfs_fs_info *fs_info,
> +				 struct btrfs_dev_lookup_args *args,
> +				 const char *path);
>   struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
>   					const u64 *devid,
>   					const u8 *uuid);
> +void btrfs_put_dev_args_from_path(struct btrfs_dev_lookup_args *args);
>   void btrfs_free_device(struct btrfs_device *device);
>   int btrfs_rm_device(struct btrfs_fs_info *fs_info,
>   		    const char *device_path, u64 devid,
> 

[PATCH v4 6/6] btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls

[Josef Bacik]

For device removal and replace we call btrfs_find_device_by_devspec,
which if we give it a device path and nothing else will call
btrfs_get_dev_args_from_path, which opens the block device and reads the
super block and then looks up our device based on that.

However at this point we're holding the sb write "lock", so reading the
block device pulls in the dependency of ->open_mutex, which produces the
following lockdep splat

======================================================
WARNING: possible circular locking dependency detected
5.14.0-rc2+ #405 Not tainted
------------------------------------------------------
losetup/11576 is trying to acquire lock:
ffff9bbe8cded938 ((wq_completion)loop0){+.+.}-{0:0}, at: flush_workqueue+0x67/0x5e0

but task is already holding lock:
ffff9bbe88e4fc68 (&lo->lo_mutex){+.+.}-{3:3}, at: __loop_clr_fd+0x41/0x660 [loop]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #4 (&lo->lo_mutex){+.+.}-{3:3}:
       __mutex_lock+0x7d/0x750
       lo_open+0x28/0x60 [loop]
       blkdev_get_whole+0x25/0xf0
       blkdev_get_by_dev.part.0+0x168/0x3c0
       blkdev_open+0xd2/0xe0
       do_dentry_open+0x161/0x390
       path_openat+0x3cc/0xa20
       do_filp_open+0x96/0x120
       do_sys_openat2+0x7b/0x130
       __x64_sys_openat+0x46/0x70
       do_syscall_64+0x38/0x90
       entry_SYSCALL_64_after_hwframe+0x44/0xae

-> #3 (&disk->open_mutex){+.+.}-{3:3}:
       __mutex_lock+0x7d/0x750
       blkdev_get_by_dev.part.0+0x56/0x3c0
       blkdev_get_by_path+0x98/0xa0
       btrfs_get_bdev_and_sb+0x1b/0xb0
       btrfs_find_device_by_devspec+0x12b/0x1c0
       btrfs_rm_device+0x127/0x610
       btrfs_ioctl+0x2a31/0x2e70
       __x64_sys_ioctl+0x80/0xb0
       do_syscall_64+0x38/0x90
       entry_SYSCALL_64_after_hwframe+0x44/0xae

-> #2 (sb_writers#12){.+.+}-{0:0}:
       lo_write_bvec+0xc2/0x240 [loop]
       loop_process_work+0x238/0xd00 [loop]
       process_one_work+0x26b/0x560
       worker_thread+0x55/0x3c0
       kthread+0x140/0x160
       ret_from_fork+0x1f/0x30

-> #1 ((work_completion)(&lo->rootcg_work)){+.+.}-{0:0}:
       process_one_work+0x245/0x560
       worker_thread+0x55/0x3c0
       kthread+0x140/0x160
       ret_from_fork+0x1f/0x30

-> #0 ((wq_completion)loop0){+.+.}-{0:0}:
       __lock_acquire+0x10ea/0x1d90
       lock_acquire+0xb5/0x2b0
       flush_workqueue+0x91/0x5e0
       drain_workqueue+0xa0/0x110
       destroy_workqueue+0x36/0x250
       __loop_clr_fd+0x9a/0x660 [loop]
       block_ioctl+0x3f/0x50
       __x64_sys_ioctl+0x80/0xb0
       do_syscall_64+0x38/0x90
       entry_SYSCALL_64_after_hwframe+0x44/0xae

other info that might help us debug this:

Chain exists of:
  (wq_completion)loop0 --> &disk->open_mutex --> &lo->lo_mutex

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&lo->lo_mutex);
                               lock(&disk->open_mutex);
                               lock(&lo->lo_mutex);
  lock((wq_completion)loop0);

 *** DEADLOCK ***

1 lock held by losetup/11576:
 #0: ffff9bbe88e4fc68 (&lo->lo_mutex){+.+.}-{3:3}, at: __loop_clr_fd+0x41/0x660 [loop]

stack backtrace:
CPU: 0 PID: 11576 Comm: losetup Not tainted 5.14.0-rc2+ #405
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014
Call Trace:
 dump_stack_lvl+0x57/0x72
 check_noncircular+0xcf/0xf0
 ? stack_trace_save+0x3b/0x50
 __lock_acquire+0x10ea/0x1d90
 lock_acquire+0xb5/0x2b0
 ? flush_workqueue+0x67/0x5e0
 ? lockdep_init_map_type+0x47/0x220
 flush_workqueue+0x91/0x5e0
 ? flush_workqueue+0x67/0x5e0
 ? verify_cpu+0xf0/0x100
 drain_workqueue+0xa0/0x110
 destroy_workqueue+0x36/0x250
 __loop_clr_fd+0x9a/0x660 [loop]
 ? blkdev_ioctl+0x8d/0x2a0
 block_ioctl+0x3f/0x50
 __x64_sys_ioctl+0x80/0xb0
 do_syscall_64+0x38/0x90
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f31b02404cb

Instead what we want to do is populate our device lookup args before we
grab any locks, and then pass these args into btrfs_rm_device().  From
there we can find the device and do the appropriate removal.

Suggested-by: Anand Jain <anand.jain@oracle.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ioctl.c   | 67 +++++++++++++++++++++++++++-------------------
 fs/btrfs/volumes.c | 15 +++++------
 fs/btrfs/volumes.h |  2 +-
 3 files changed, 48 insertions(+), 36 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b8508af4e539..c9d3f375df83 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3160,6 +3160,7 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
 
 static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct inode *inode = file_inode(file);
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_ioctl_vol_args_v2 *vol_args;
@@ -3171,35 +3172,39 @@ static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	ret = mnt_want_write_file(file);
-	if (ret)
-		return ret;
-
 	vol_args = memdup_user(arg, sizeof(*vol_args));
 	if (IS_ERR(vol_args)) {
 		ret = PTR_ERR(vol_args);
-		goto err_drop;
+		goto out;
 	}
 
 	if (vol_args->flags & ~BTRFS_DEVICE_REMOVE_ARGS_MASK) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
+
 	vol_args->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
-	if (!(vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID) &&
-	    strcmp("cancel", vol_args->name) == 0)
+	if (vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID) {
+		args.devid = vol_args->devid;
+	} else if (!strcmp("cancel", vol_args->name)) {
 		cancel = true;
+	} else {
+		ret = btrfs_get_dev_args_from_path(fs_info, &args, vol_args->name);
+		if (ret)
+			goto out;
+	}
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		goto out;
 
 	ret = exclop_start_or_cancel_reloc(fs_info, BTRFS_EXCLOP_DEV_REMOVE,
 					   cancel);
 	if (ret)
-		goto out;
-	/* Exclusive operation is now claimed */
+		goto err_drop;
 
-	if (vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID)
-		ret = btrfs_rm_device(fs_info, NULL, vol_args->devid, &bdev, &mode);
-	else
-		ret = btrfs_rm_device(fs_info, vol_args->name, 0, &bdev, &mode);
+	/* Exclusive operation is now claimed */
+	ret = btrfs_rm_device(fs_info, &args, &bdev, &mode);
 
 	btrfs_exclop_finish(fs_info);
 
@@ -3211,17 +3216,19 @@ static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
 			btrfs_info(fs_info, "device deleted: %s",
 					vol_args->name);
 	}
-out:
-	kfree(vol_args);
 err_drop:
 	mnt_drop_write_file(file);
 	if (bdev)
 		blkdev_put(bdev, mode);
+out:
+	btrfs_put_dev_args_from_path(&args);
+	kfree(vol_args);
 	return ret;
 }
 
 static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 {
+	BTRFS_DEV_LOOKUP_ARGS(args);
 	struct inode *inode = file_inode(file);
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_ioctl_vol_args *vol_args;
@@ -3233,32 +3240,38 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	ret = mnt_want_write_file(file);
-	if (ret)
-		return ret;
-
 	vol_args = memdup_user(arg, sizeof(*vol_args));
-	if (IS_ERR(vol_args)) {
-		ret = PTR_ERR(vol_args);
-		goto out_drop_write;
-	}
+	if (IS_ERR(vol_args))
+		return PTR_ERR(vol_args);
+
 	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
-	cancel = (strcmp("cancel", vol_args->name) == 0);
+	if (!strcmp("cancel", vol_args->name)) {
+		cancel = true;
+	} else {
+		ret = btrfs_get_dev_args_from_path(fs_info, &args, vol_args->name);
+		if (ret)
+			goto out;
+	}
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		goto out;
 
 	ret = exclop_start_or_cancel_reloc(fs_info, BTRFS_EXCLOP_DEV_REMOVE,
 					   cancel);
 	if (ret == 0) {
-		ret = btrfs_rm_device(fs_info, vol_args->name, 0, &bdev, &mode);
+		ret = btrfs_rm_device(fs_info, &args, &bdev, &mode);
 		if (!ret)
 			btrfs_info(fs_info, "disk deleted %s", vol_args->name);
 		btrfs_exclop_finish(fs_info);
 	}
 
-	kfree(vol_args);
-out_drop_write:
 	mnt_drop_write_file(file);
 	if (bdev)
 		blkdev_put(bdev, mode);
+out:
+	btrfs_put_dev_args_from_path(&args);
+	kfree(vol_args);
 	return ret;
 }
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e490414e8987..3262e75fbb1c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2076,8 +2076,9 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info,
 	update_dev_time(bdev);
 }
 
-int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
-		    u64 devid, struct block_device **bdev, fmode_t *mode)
+int btrfs_rm_device(struct btrfs_fs_info *fs_info,
+		    struct btrfs_dev_lookup_args *args,
+		    struct block_device **bdev, fmode_t *mode)
 {
 	struct btrfs_device *device;
 	struct btrfs_fs_devices *cur_devices;
@@ -2096,14 +2097,12 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	if (ret)
 		goto out;
 
-	device = btrfs_find_device_by_devspec(fs_info, devid, device_path);
-
-	if (IS_ERR(device)) {
-		if (PTR_ERR(device) == -ENOENT &&
-		    device_path && strcmp(device_path, "missing") == 0)
+	device = btrfs_find_device(fs_info->fs_devices, args);
+	if (!device) {
+		if (args->missing)
 			ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
 		else
-			ret = PTR_ERR(device);
+			ret = -ENOENT;
 		goto out;
 	}
 
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 3fe5ac683f98..223c390ec057 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -527,7 +527,7 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
 void btrfs_put_dev_args_from_path(struct btrfs_dev_lookup_args *args);
 void btrfs_free_device(struct btrfs_device *device);
 int btrfs_rm_device(struct btrfs_fs_info *fs_info,
-		    const char *device_path, u64 devid,
+		    struct btrfs_dev_lookup_args *args,
 		    struct block_device **bdev, fmode_t *mode);
 void __exit btrfs_cleanup_fs_uuids(void);
 int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
-- 
2.26.3

Re: [PATCH v4 6/6] btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls

[Anand Jain]

On 06/10/2021 04:12, Josef Bacik wrote:
> For device removal and replace we call btrfs_find_device_by_devspec,
> which if we give it a device path and nothing else will call
> btrfs_get_dev_args_from_path, which opens the block device and reads the
> super block and then looks up our device based on that.
> 
> However at this point we're holding the sb write "lock", so reading the
> block device pulls in the dependency of ->open_mutex, which produces the
> following lockdep splat
> 
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.14.0-rc2+ #405 Not tainted
> ------------------------------------------------------
> losetup/11576 is trying to acquire lock:
> ffff9bbe8cded938 ((wq_completion)loop0){+.+.}-{0:0}, at: flush_workqueue+0x67/0x5e0
> 
> but task is already holding lock:
> ffff9bbe88e4fc68 (&lo->lo_mutex){+.+.}-{3:3}, at: __loop_clr_fd+0x41/0x660 [loop]
> 
> which lock already depends on the new lock.
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #4 (&lo->lo_mutex){+.+.}-{3:3}:
>         __mutex_lock+0x7d/0x750
>         lo_open+0x28/0x60 [loop]
>         blkdev_get_whole+0x25/0xf0
>         blkdev_get_by_dev.part.0+0x168/0x3c0
>         blkdev_open+0xd2/0xe0
>         do_dentry_open+0x161/0x390
>         path_openat+0x3cc/0xa20
>         do_filp_open+0x96/0x120
>         do_sys_openat2+0x7b/0x130
>         __x64_sys_openat+0x46/0x70
>         do_syscall_64+0x38/0x90
>         entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> -> #3 (&disk->open_mutex){+.+.}-{3:3}:
>         __mutex_lock+0x7d/0x750
>         blkdev_get_by_dev.part.0+0x56/0x3c0
>         blkdev_get_by_path+0x98/0xa0
>         btrfs_get_bdev_and_sb+0x1b/0xb0
>         btrfs_find_device_by_devspec+0x12b/0x1c0
>         btrfs_rm_device+0x127/0x610
>         btrfs_ioctl+0x2a31/0x2e70
>         __x64_sys_ioctl+0x80/0xb0
>         do_syscall_64+0x38/0x90
>         entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> -> #2 (sb_writers#12){.+.+}-{0:0}:
>         lo_write_bvec+0xc2/0x240 [loop]
>         loop_process_work+0x238/0xd00 [loop]
>         process_one_work+0x26b/0x560
>         worker_thread+0x55/0x3c0
>         kthread+0x140/0x160
>         ret_from_fork+0x1f/0x30
> 
> -> #1 ((work_completion)(&lo->rootcg_work)){+.+.}-{0:0}:
>         process_one_work+0x245/0x560
>         worker_thread+0x55/0x3c0
>         kthread+0x140/0x160
>         ret_from_fork+0x1f/0x30
> 
> -> #0 ((wq_completion)loop0){+.+.}-{0:0}:
>         __lock_acquire+0x10ea/0x1d90
>         lock_acquire+0xb5/0x2b0
>         flush_workqueue+0x91/0x5e0
>         drain_workqueue+0xa0/0x110
>         destroy_workqueue+0x36/0x250
>         __loop_clr_fd+0x9a/0x660 [loop]
>         block_ioctl+0x3f/0x50
>         __x64_sys_ioctl+0x80/0xb0
>         do_syscall_64+0x38/0x90
>         entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> other info that might help us debug this:
> 
> Chain exists of:
>    (wq_completion)loop0 --> &disk->open_mutex --> &lo->lo_mutex
> 
>   Possible unsafe locking scenario:
> 
>         CPU0                    CPU1
>         ----                    ----
>    lock(&lo->lo_mutex);
>                                 lock(&disk->open_mutex);
>                                 lock(&lo->lo_mutex);
>    lock((wq_completion)loop0);
> 
>   *** DEADLOCK ***
> 
> 1 lock held by losetup/11576:
>   #0: ffff9bbe88e4fc68 (&lo->lo_mutex){+.+.}-{3:3}, at: __loop_clr_fd+0x41/0x660 [loop]
> 
> stack backtrace:
> CPU: 0 PID: 11576 Comm: losetup Not tainted 5.14.0-rc2+ #405
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014
> Call Trace:
>   dump_stack_lvl+0x57/0x72
>   check_noncircular+0xcf/0xf0
>   ? stack_trace_save+0x3b/0x50
>   __lock_acquire+0x10ea/0x1d90
>   lock_acquire+0xb5/0x2b0
>   ? flush_workqueue+0x67/0x5e0
>   ? lockdep_init_map_type+0x47/0x220
>   flush_workqueue+0x91/0x5e0
>   ? flush_workqueue+0x67/0x5e0
>   ? verify_cpu+0xf0/0x100
>   drain_workqueue+0xa0/0x110
>   destroy_workqueue+0x36/0x250
>   __loop_clr_fd+0x9a/0x660 [loop]
>   ? blkdev_ioctl+0x8d/0x2a0
>   block_ioctl+0x3f/0x50
>   __x64_sys_ioctl+0x80/0xb0
>   do_syscall_64+0x38/0x90
>   entry_SYSCALL_64_after_hwframe+0x44/0xae
> RIP: 0033:0x7f31b02404cb
> 
> Instead what we want to do is populate our device lookup args before we
> grab any locks, and then pass these args into btrfs_rm_device().  From
> there we can find the device and do the appropriate removal.
> 
> Suggested-by: Anand Jain <anand.jain@oracle.com>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>   fs/btrfs/ioctl.c   | 67 +++++++++++++++++++++++++++-------------------
>   fs/btrfs/volumes.c | 15 +++++------
>   fs/btrfs/volumes.h |  2 +-
>   3 files changed, 48 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index b8508af4e539..c9d3f375df83 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3160,6 +3160,7 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
>   
>   static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct inode *inode = file_inode(file);
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   	struct btrfs_ioctl_vol_args_v2 *vol_args;
> @@ -3171,35 +3172,39 @@ static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
>   	if (!capable(CAP_SYS_ADMIN))
>   		return -EPERM;
>   
> -	ret = mnt_want_write_file(file);
> -	if (ret)
> -		return ret;
> -
>   	vol_args = memdup_user(arg, sizeof(*vol_args));
>   	if (IS_ERR(vol_args)) {

>   		ret = PTR_ERR(vol_args);
> -		goto err_drop;
> +		goto out;

  	return  PTR_ERR(vol_args);

is fine here.

>   	}
>   
>   	if (vol_args->flags & ~BTRFS_DEVICE_REMOVE_ARGS_MASK) {
>   		ret = -EOPNOTSUPP;
>   		goto out;

At the label out, we call, btrfs_put_dev_args_from_path(&args).
However, the args.fsid and args.uuid might not have initialized
to NULL or a valid kmem address.

>   	}
> +
>   	vol_args->name[BTRFS_SUBVOL_NAME_MAX] = '\0';
> -	if (!(vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID) &&
> -	    strcmp("cancel", vol_args->name) == 0)
> +	if (vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID) {
> +		args.devid = vol_args->devid;
> +	} else if (!strcmp("cancel", vol_args->name)) {
>   		cancel = true;
> +	} else {
> +		ret = btrfs_get_dev_args_from_path(fs_info, &args, vol_args->name);
> +		if (ret)
> +			goto out;


Same as above.

> +	}
> +
> +	ret = mnt_want_write_file(file);
> +	if (ret)
> +		goto out;
>   
>   	ret = exclop_start_or_cancel_reloc(fs_info, BTRFS_EXCLOP_DEV_REMOVE,
>   					   cancel);
>   	if (ret)
> -		goto out;
> -	/* Exclusive operation is now claimed */
> +		goto err_drop;
>   
> -	if (vol_args->flags & BTRFS_DEVICE_SPEC_BY_ID)
> -		ret = btrfs_rm_device(fs_info, NULL, vol_args->devid, &bdev, &mode);
> -	else
> -		ret = btrfs_rm_device(fs_info, vol_args->name, 0, &bdev, &mode);
> +	/* Exclusive operation is now claimed */
> +	ret = btrfs_rm_device(fs_info, &args, &bdev, &mode);
>   
>   	btrfs_exclop_finish(fs_info);
>   
> @@ -3211,17 +3216,19 @@ static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user *arg)
>   			btrfs_info(fs_info, "device deleted: %s",
>   					vol_args->name);
>   	}
> -out:
> -	kfree(vol_args);
>   err_drop:
>   	mnt_drop_write_file(file);
>   	if (bdev)
>   		blkdev_put(bdev, mode);
> +out:
> +	btrfs_put_dev_args_from_path(&args);
> +	kfree(vol_args);
>   	return ret;
>   }
>   
>   static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
>   {
> +	BTRFS_DEV_LOOKUP_ARGS(args);
>   	struct inode *inode = file_inode(file);
>   	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>   	struct btrfs_ioctl_vol_args *vol_args;
> @@ -3233,32 +3240,38 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
>   	if (!capable(CAP_SYS_ADMIN))
>   		return -EPERM;
>   
> -	ret = mnt_want_write_file(file);
> -	if (ret)
> -		return ret;
> -
>   	vol_args = memdup_user(arg, sizeof(*vol_args));
> -	if (IS_ERR(vol_args)) {
> -		ret = PTR_ERR(vol_args);
> -		goto out_drop_write;
> -	}
> +	if (IS_ERR(vol_args))
> +		return PTR_ERR(vol_args);
> +
>   	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
> -	cancel = (strcmp("cancel", vol_args->name) == 0);
> +	if (!strcmp("cancel", vol_args->name)) {
> +		cancel = true;
> +	} else {
> +		ret = btrfs_get_dev_args_from_path(fs_info, &args, vol_args->name);
> +		if (ret)
> +			goto out;


Here too.

Thanks, Anand

> +	}
> +
> +	ret = mnt_want_write_file(file);
> +	if (ret)
> +		goto out;
>   
>   	ret = exclop_start_or_cancel_reloc(fs_info, BTRFS_EXCLOP_DEV_REMOVE,
>   					   cancel);
>   	if (ret == 0) {
> -		ret = btrfs_rm_device(fs_info, vol_args->name, 0, &bdev, &mode);
> +		ret = btrfs_rm_device(fs_info, &args, &bdev, &mode);
>   		if (!ret)
>   			btrfs_info(fs_info, "disk deleted %s", vol_args->name);
>   		btrfs_exclop_finish(fs_info);
>   	}
>   
> -	kfree(vol_args);
> -out_drop_write:
>   	mnt_drop_write_file(file);
>   	if (bdev)
>   		blkdev_put(bdev, mode);
> +out:
> +	btrfs_put_dev_args_from_path(&args);
> +	kfree(vol_args);
>   	return ret;
>   }
>   
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e490414e8987..3262e75fbb1c 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2076,8 +2076,9 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info,
>   	update_dev_time(bdev);
>   }
>   
> -int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
> -		    u64 devid, struct block_device **bdev, fmode_t *mode)
> +int btrfs_rm_device(struct btrfs_fs_info *fs_info,
> +		    struct btrfs_dev_lookup_args *args,
> +		    struct block_device **bdev, fmode_t *mode)
>   {
>   	struct btrfs_device *device;
>   	struct btrfs_fs_devices *cur_devices;
> @@ -2096,14 +2097,12 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>   	if (ret)
>   		goto out;
>   
> -	device = btrfs_find_device_by_devspec(fs_info, devid, device_path);
> -
> -	if (IS_ERR(device)) {
> -		if (PTR_ERR(device) == -ENOENT &&
> -		    device_path && strcmp(device_path, "missing") == 0)
> +	device = btrfs_find_device(fs_info->fs_devices, args);
> +	if (!device) {
> +		if (args->missing)
>   			ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
>   		else
> -			ret = PTR_ERR(device);
> +			ret = -ENOENT;
>   		goto out;
>   	}
>   
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 3fe5ac683f98..223c390ec057 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -527,7 +527,7 @@ struct btrfs_device *btrfs_alloc_device(struct btrfs_fs_info *fs_info,
>   void btrfs_put_dev_args_from_path(struct btrfs_dev_lookup_args *args);
>   void btrfs_free_device(struct btrfs_device *device);
>   int btrfs_rm_device(struct btrfs_fs_info *fs_info,
> -		    const char *device_path, u64 devid,
> +		    struct btrfs_dev_lookup_args *args,
>   		    struct block_device **bdev, fmode_t *mode);
>   void __exit btrfs_cleanup_fs_uuids(void);
>   int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
> 

Re: [PATCH v4 6/6] btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls

[Nikolay Borisov]

On 6.10.21 г. 11:54, Anand Jain wrote:
<snip>

>> Suggested-by: Anand Jain <anand.jain@oracle.com>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/ioctl.c   | 67 +++++++++++++++++++++++++++-------------------
>>   fs/btrfs/volumes.c | 15 +++++------
>>   fs/btrfs/volumes.h |  2 +-
>>   3 files changed, 48 insertions(+), 36 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index b8508af4e539..c9d3f375df83 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3160,6 +3160,7 @@ static long btrfs_ioctl_add_dev(struct
>> btrfs_fs_info *fs_info, void __user *arg)
>>     static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user
>> *arg)
>>   {
>> +    BTRFS_DEV_LOOKUP_ARGS(args);
>>       struct inode *inode = file_inode(file);
>>       struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>>       struct btrfs_ioctl_vol_args_v2 *vol_args;
>> @@ -3171,35 +3172,39 @@ static long btrfs_ioctl_rm_dev_v2(struct file
>> *file, void __user *arg)
>>       if (!capable(CAP_SYS_ADMIN))
>>           return -EPERM;
>>   -    ret = mnt_want_write_file(file);
>> -    if (ret)
>> -        return ret;
>> -
>>       vol_args = memdup_user(arg, sizeof(*vol_args));
>>       if (IS_ERR(vol_args)) {
> 
>>           ret = PTR_ERR(vol_args);
>> -        goto err_drop;
>> +        goto out;
> 
>      return  PTR_ERR(vol_args);
> 
> is fine here.
> 
>>       }
>>         if (vol_args->flags & ~BTRFS_DEVICE_REMOVE_ARGS_MASK) {
>>           ret = -EOPNOTSUPP;
>>           goto out;
> 
> At the label out, we call, btrfs_put_dev_args_from_path(&args).
> However, the args.fsid and args.uuid might not have initialized
> to NULL or a valid kmem address.

On tha contrary, the fact that args is initialized via a designated
initializer guarantees that the rest of the members of the struct are
going to be zero initialized, or more precisley as if they had a "static
lifetime"

https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

<snip>

Re: [PATCH v4 6/6] btrfs: use btrfs_get_dev_args_from_path in dev removal ioctls

[Anand Jain]

On 06/10/2021 17:05, Nikolay Borisov wrote:
> 
> 
> On 6.10.21 г. 11:54, Anand Jain wrote:
> <snip>
> 
>>> Suggested-by: Anand Jain <anand.jain@oracle.com>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>>    fs/btrfs/ioctl.c   | 67 +++++++++++++++++++++++++++-------------------
>>>    fs/btrfs/volumes.c | 15 +++++------
>>>    fs/btrfs/volumes.h |  2 +-
>>>    3 files changed, 48 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index b8508af4e539..c9d3f375df83 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -3160,6 +3160,7 @@ static long btrfs_ioctl_add_dev(struct
>>> btrfs_fs_info *fs_info, void __user *arg)
>>>      static long btrfs_ioctl_rm_dev_v2(struct file *file, void __user
>>> *arg)
>>>    {
>>> +    BTRFS_DEV_LOOKUP_ARGS(args);
>>>        struct inode *inode = file_inode(file);
>>>        struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>>>        struct btrfs_ioctl_vol_args_v2 *vol_args;
>>> @@ -3171,35 +3172,39 @@ static long btrfs_ioctl_rm_dev_v2(struct file
>>> *file, void __user *arg)
>>>        if (!capable(CAP_SYS_ADMIN))
>>>            return -EPERM;
>>>    -    ret = mnt_want_write_file(file);
>>> -    if (ret)
>>> -        return ret;
>>> -
>>>        vol_args = memdup_user(arg, sizeof(*vol_args));
>>>        if (IS_ERR(vol_args)) {
>>
>>>            ret = PTR_ERR(vol_args);
>>> -        goto err_drop;
>>> +        goto out;
>>
>>       return  PTR_ERR(vol_args);
>>
>> is fine here.
>>
>>>        }
>>>          if (vol_args->flags & ~BTRFS_DEVICE_REMOVE_ARGS_MASK) {
>>>            ret = -EOPNOTSUPP;
>>>            goto out;
>>
>> At the label out, we call, btrfs_put_dev_args_from_path(&args).
>> However, the args.fsid and args.uuid might not have initialized
>> to NULL or a valid kmem address.
> 
> On tha contrary, the fact that args is initialized via a designated
> initializer guarantees that the rest of the members of the struct are
> going to be zero initialized, or more precisley as if they had a "static
> lifetime"
> 
> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
> 
> <snip>

Sorry for the late reply, I see the patch made it to the misc-next.
I missed both this email and the point that the rest of the members
initialize to zero. Yep. Looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

Re: [PATCH v4 0/6] Fix lockdep issues around device removal

[David Sterba]

On Tue, Oct 05, 2021 at 04:12:38PM -0400, Josef Bacik wrote:
> v3->v4:
> - I had a fixup for assinging devid that I mis-merged into the wrong patch,
>   fixed this up by putting it in the correct patch.

Added to misc-next, thanks.