[PATCH v2 0/4] Misc volume patch set part2

[PATCH v2 0/4] Misc volume patch set part2

[Anand Jain]

In this set:
v1->v2:
1/4 no changes.
2/4 no changes.
3/4 added new. Which shall replace the warn_on with bug_on. As the
redundency is after this check. So I decided to keep bug_on for now.
4/4 added comments that btrfs_num_devices() will get num_devices
excluding the repalce device.

Anand Jain (4):
  btrfs: drop uuid_mutex in btrfs_free_extra_devids()
  btrfs: fix race between free_stale_devices and close_fs_devices
  btrfs: bug_on for num_devices below 0
  btrfs: add helper btrfs_num_devices() to deduce num_devices

 fs/btrfs/volumes.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

-- 
2.7.0

[PATCH v4 1/4] btrfs: drop uuid_mutex in btrfs_free_extra_devids()

[Anand Jain]

btrfs_free_extra_devids() is called only in the mount context which
traverses through the fs_devices::devices and frees the orphan devices
devices in the given %fs_devices if any. As the search for the orphan
device is limited to fs_devices::devices so we don't need the global
uuid_mutex.

There can't be any mount-point based ioctl threads in this context as
the mount thread is not yet returned. But there can be the btrfs-control
based scan ioctls thread which calls device_list_add().

Here in the mount thread the fs_devices::opened is incremented way before
btrfs_free_extra_devids() is called and in the scan context the fs_devices
which are already opened neither be freed or alloc-able at
device_list_add().

But lets say you change the device-path and call the scan again, then scan
would update the new device path and this operation could race against the
btrfs_free_extra_devids() thread, which might be in the process of
free-ing the same device. So synchronize it by using the
device_list_mutex.

This scenario is a very corner case, and practically the scan and mount
are anyway serialized by the usage so unless the race is instrumented its
very difficult to achieve.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.com>
---
v3->v4: As we traverse through the seed device, fs_device gets updated with
	the child seed fs_devices, so make sure we use the same fs_devices
	pointer for the mutex_unlock as used for the mutex_lock.
v2->v3: Update change log.
	(Currently device_list_add() is very lean on its device_list_mutex usage,
	a cleanup and fix is wip. Given the practicality of the above race
	condition this patch is good to merge).
v1->v2: replace uuid_mutex with device_list_mutex instead of delete.
	change log updated.

 fs/btrfs/volumes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 66f5ac5e8058..2166c5e7cc01 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -934,8 +934,9 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 {
 	struct btrfs_device *device, *next;
 	struct btrfs_device *latest_dev = NULL;
+	struct btrfs_fs_devices *parent_fs_devices = fs_devices;
 
-	mutex_lock(&uuid_mutex);
+	mutex_lock(&parent_fs_devices->device_list_mutex);
 again:
 	/* This is the initialized path, it is safe to release the devices. */
 	list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) {
@@ -989,8 +990,7 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step)
 	}
 
 	fs_devices->latest_bdev = latest_dev->bdev;
-
-	mutex_unlock(&uuid_mutex);
+	mutex_unlock(&parent_fs_devices->device_list_mutex);
 }
 
 static void free_device_rcu(struct rcu_head *head)
-- 
2.7.0

[PATCH v2 2/4] btrfs: fix race between free_stale_devices and close_fs_devices

[Anand Jain]

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

%fs_devices can be free-ed by btrfs_free_stale_devices() when the
close_fs_devices() drops fs_devices::opened to zero, but close_fs_devices
tries to access the %fs_devices again without the device_list_mutex.

Fix this by bringing the %fs_devices access with in the device_list_mutex.

Stack trace as below.

WARNING: CPU: 1 PID: 4499 at fs/btrfs/volumes.c:1071 close_fs_devices+0xbc7/0xfa0 fs/btrfs/volumes.c:1071
Kernel panic - not syncing: panic_on_warn set ...
::
RIP: 0010:close_fs_devices+0xbc7/0xfa0 fs/btrfs/volumes.c:1071
::
 btrfs_close_devices+0x29/0x150 fs/btrfs/volumes.c:1085
 open_ctree+0x589/0x7898 fs/btrfs/disk-io.c:3358
 btrfs_fill_super fs/btrfs/super.c:1202 [inline]
 btrfs_mount_root+0x16df/0x1e70 fs/btrfs/super.c:1593
 mount_fs+0xae/0x328 fs/super.c:1277
 vfs_kern_mount.part.34+0xd4/0x4d0 fs/namespace.c:1037
 vfs_kern_mount+0x40/0x60 fs/namespace.c:1027
 btrfs_mount+0x4a1/0x213e fs/btrfs/super.c:1661
 mount_fs+0xae/0x328 fs/super.c:1277

Reported-by: syzbot+ceb2606025ec1cc3479c@syzkaller.appspotmail.com
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2: Commit log update. Satisfy checkpatch.pl. Remove HEAD..

 fs/btrfs/volumes.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2166c5e7cc01..c62b5e46792e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1062,13 +1062,13 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
 		btrfs_close_one_device(device);
 	}
-	mutex_unlock(&fs_devices->device_list_mutex);
-
 	WARN_ON(fs_devices->open_devices);
 	WARN_ON(fs_devices->rw_devices);
 	fs_devices->opened = 0;
 	fs_devices->seeding = 0;
 
+	mutex_unlock(&fs_devices->device_list_mutex);
+
 	return 0;
 }
 
-- 
2.7.0

[PATCH 3/4] btrfs: bug_on for num_devices below 0

[Anand Jain]

In preparation to add helper function to deduce the num_devices with
replace running, use bug_on instead of warn_on. In btrfs_rm_device()
the check for min devices happens in btrfs_check_raid_min_devices()
but its after its been decremented so a check here is better.

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 c62b5e46792e..fe74fefc75f7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1868,7 +1868,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	num_devices = fs_devices->num_devices;
 	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
 	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		WARN_ON(num_devices < 1);
+		BUG_ON(num_devices < 1);
 		num_devices--;
 	}
 	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
-- 
2.7.0

[PATCH v2 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

When the replace is running the fs_devices::num_devices also includes
the replace device, however in some operations like device delete and
balance it needs the actual num_devices without the repalce devices, so
now the function btrfs_num_devices() just provides that.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2: add comments
 fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index fe74fefc75f7..8844904f9009 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1854,6 +1854,21 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
 		fs_info->fs_devices->latest_bdev = next_device->bdev;
 }
 
+/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
+static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
+{
+	u64 num_devices = fs_info->fs_devices->num_devices;
+
+	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
+	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
+		BUG_ON(num_devices < 1);
+		num_devices--;
+	}
+	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+
+	return num_devices;
+}
+
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		u64 devid)
 {
@@ -1865,13 +1880,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 
 	mutex_lock(&uuid_mutex);
 
-	num_devices = fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
 
 	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
 	if (ret)
@@ -3721,13 +3730,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		}
 	}
 
-	num_devices = fs_info->fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
+
 	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
 	if (num_devices > 1)
 		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
-- 
2.7.0

Re: [PATCH v2 2/4] btrfs: fix race between free_stale_devices and close_fs_devices

[David Sterba]

On Thu, Jul 26, 2018 at 02:53:32PM +0800, Anand Jain wrote:
> From: Anand Jain <Anand.Jain@oracle.com>
> 
> %fs_devices can be free-ed by btrfs_free_stale_devices() when the
> close_fs_devices() drops fs_devices::opened to zero, but close_fs_devices
> tries to access the %fs_devices again without the device_list_mutex.
> 
> Fix this by bringing the %fs_devices access with in the device_list_mutex.

AFAICS this cannot happen anymore because the two calls are serialized
by the uuid_mutex. But this was not the case when syzbot reported the
problem where your patch would apply.

The parallell access to opened and device list cannot happen when:

* btrfs_scan_one_device that wants to call btrfs_free_stale_devices
* btrfs_close_devices calls close_fs_devices

Fixed by the series:

btrfs: lift uuid_mutex to callers of btrfs_scan_one_device
btrfs: lift uuid_mutex to callers of btrfs_open_devices
btrfs: lift uuid_mutex to callers of btrfs_parse_early_options
btrfs: reorder initialization before the mount locks uuid_mutex
btrfs: fix mount and ioctl device scan ioctl race

If there's a race I don't see, please describe in more detail.

Re: [PATCH v2 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[David Sterba]

On Thu, Jul 26, 2018 at 02:53:34PM +0800, Anand Jain wrote:
> When the replace is running the fs_devices::num_devices also includes
> the replace device, however in some operations like device delete and
> balance it needs the actual num_devices without the repalce devices, so
> now the function btrfs_num_devices() just provides that.

The part how concurrent balance and dev-replace can be active at the
same time is missing. As this is not obvious, it's desired to have that
in the changelog.

If there are questions and comments in past patchset revisions, that's a
good material for changelogs and when you send an update you can enhance
the changelogs. I do simple fixups or additions but when you send a new
revision it the right time to save time on both sides.

Re: [PATCH v2 2/4] btrfs: fix race between free_stale_devices and close_fs_devices

[Anand Jain]

On 08/01/2018 10:29 PM, David Sterba wrote:
> On Thu, Jul 26, 2018 at 02:53:32PM +0800, Anand Jain wrote:
>> From: Anand Jain <Anand.Jain@oracle.com>
>>
>> %fs_devices can be free-ed by btrfs_free_stale_devices() when the
>> close_fs_devices() drops fs_devices::opened to zero, but close_fs_devices
>> tries to access the %fs_devices again without the device_list_mutex.
>>
>> Fix this by bringing the %fs_devices access with in the device_list_mutex.
> 
> AFAICS this cannot happen anymore because the two calls are serialized
> by the uuid_mutex. But this was not the case when syzbot reported the
> problem where your patch would apply.
> 
> The parallell access to opened and device list cannot happen when:
> 
> * btrfs_scan_one_device that wants to call btrfs_free_stale_devices
> * btrfs_close_devices calls close_fs_devices
> 
> Fixed by the series:
> 
> btrfs: lift uuid_mutex to callers of btrfs_scan_one_device
> btrfs: lift uuid_mutex to callers of btrfs_open_devices
> btrfs: lift uuid_mutex to callers of btrfs_parse_early_options
> btrfs: reorder initialization before the mount locks uuid_mutex
> btrfs: fix mount and ioctl device scan ioctl race
> 
> If there's a race I don't see, please describe in more detail.

  Right. There is no race with the uuid_mutex patches as above.

  And I just found this- can we make close be consistent with its
  open part.
  btrfs_open_devices() hold device_list_mutex before the update to
  fs_devices::opened. So close_fs_device() could do the same, and be
  theoretically correct.

Thanks, Anand


> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v3 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

When the replace is running the fs_devices::num_devices also includes
the replace device, however in some operations like device delete and
balance it needs the actual num_devices without the repalce devices, so
now the function btrfs_num_devices() just provides that.

And here is a scenario how balance and repalce items could co-exist.
Consider balance is started and paused, now start the replace
followed by a power-recycle of the system. During following mount,
the open_ctree() first restarts the balance so it must check for the
replace device otherwise our num_devices calculation will be wrong.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2->v3: update changelog with not so obvious balance and repalce
co-existance secnario
v1->v2: add comments

 fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index fe74fefc75f7..8844904f9009 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1854,6 +1854,21 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
 		fs_info->fs_devices->latest_bdev = next_device->bdev;
 }
 
+/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
+static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
+{
+	u64 num_devices = fs_info->fs_devices->num_devices;
+
+	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
+	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
+		BUG_ON(num_devices < 1);
+		num_devices--;
+	}
+	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+
+	return num_devices;
+}
+
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		u64 devid)
 {
@@ -1865,13 +1880,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 
 	mutex_lock(&uuid_mutex);
 
-	num_devices = fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
 
 	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
 	if (ret)
@@ -3721,13 +3730,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		}
 	}
 
-	num_devices = fs_info->fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
+
 	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
 	if (num_devices > 1)
 		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
-- 
2.7.0

Re: [PATCH v2 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

On 08/01/2018 10:41 PM, David Sterba wrote:
> On Thu, Jul 26, 2018 at 02:53:34PM +0800, Anand Jain wrote:
>> When the replace is running the fs_devices::num_devices also includes
>> the replace device, however in some operations like device delete and
>> balance it needs the actual num_devices without the repalce devices, so
>> now the function btrfs_num_devices() just provides that.
> 
> The part how concurrent balance and dev-replace can be active at the
> same time is missing. As this is not obvious, it's desired to have that
> in the changelog.

  Will do.

> If there are questions and comments in past patchset revisions, that's a
> good material for changelogs and when you send an update you can enhance
> the changelogs. I do simple fixups or additions but when you send a new
> revision it the right time to save time on both sides.

  Thanks for mentioning it.

-Anand

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Nikolay Borisov]

On  2.08.2018 13:09, Anand Jain wrote:
> When the replace is running the fs_devices::num_devices also includes
> the replace device, however in some operations like device delete and
> balance it needs the actual num_devices without the repalce devices, so
> now the function btrfs_num_devices() just provides that.
> 
> And here is a scenario how balance and repalce items could co-exist.
> Consider balance is started and paused, now start the replace
> followed by a power-recycle of the system. During following mount,
> the open_ctree() first restarts the balance so it must check for the
> replace device otherwise our num_devices calculation will be wrong.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2->v3: update changelog with not so obvious balance and repalce
> co-existance secnario
> v1->v2: add comments
> 
>  fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index fe74fefc75f7..8844904f9009 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1854,6 +1854,21 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
>  		fs_info->fs_devices->latest_bdev = next_device->bdev;
>  }
>  
> +/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
> +{
> +	u64 num_devices = fs_info->fs_devices->num_devices;
> +
> +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> +		BUG_ON(num_devices < 1);
> +		num_devices--;
> +	}
> +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> +
> +	return num_devices;
> +}
> +
>  int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  		u64 devid)
>  {
> @@ -1865,13 +1880,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  
>  	mutex_lock(&uuid_mutex);
>  
> -	num_devices = fs_devices->num_devices;
> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> -		BUG_ON(num_devices < 1);
> -		num_devices--;
> -	}
> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> +	num_devices = btrfs_num_devices(fs_info);

How about lifting the BUG_ON from btrfs_num_devices into a check in this
function, so if num_devices < 1 then we just exit with -EINVAL or some
such. We should be aiming at eliminating BUG_ONs.

>  
>  	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>  	if (ret)
> @@ -3721,13 +3730,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		}
>  	}
>  
> -	num_devices = fs_info->fs_devices->num_devices;
> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> -		BUG_ON(num_devices < 1);
> -		num_devices--;
> -	}
> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> +	num_devices = btrfs_num_devices(fs_info);
> +
>  	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>  	if (num_devices > 1)
>  		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
> 

Re: [PATCH v3 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[David Sterba]

On Thu, Aug 02, 2018 at 01:11:40PM +0300, Nikolay Borisov wrote:
> 
> 
> On  2.08.2018 13:09, Anand Jain wrote:
> > When the replace is running the fs_devices::num_devices also includes
> > the replace device, however in some operations like device delete and
> > balance it needs the actual num_devices without the repalce devices, so
> > now the function btrfs_num_devices() just provides that.
> > 
> > And here is a scenario how balance and repalce items could co-exist.
> > Consider balance is started and paused, now start the replace
> > followed by a power-recycle of the system. During following mount,
> > the open_ctree() first restarts the balance so it must check for the
> > replace device otherwise our num_devices calculation will be wrong.
> > 
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > ---
> > v2->v3: update changelog with not so obvious balance and repalce
> > co-existance secnario
> > v1->v2: add comments
> > 
> >  fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
> >  1 file changed, 18 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index fe74fefc75f7..8844904f9009 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -1854,6 +1854,21 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
> >  		fs_info->fs_devices->latest_bdev = next_device->bdev;
> >  }
> >  
> > +/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
> > +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)

This does not need to be static inline, it's not in a header.

> > +{
> > +	u64 num_devices = fs_info->fs_devices->num_devices;
> > +
> > +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> > +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> > +		BUG_ON(num_devices < 1);
> > +		num_devices--;
> > +	}
> > +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> > +
> > +	return num_devices;
> > +}
> > +
> >  int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
> >  		u64 devid)
> >  {
> > @@ -1865,13 +1880,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
> >  
> >  	mutex_lock(&uuid_mutex);
> >  
> > -	num_devices = fs_devices->num_devices;
> > -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> > -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> > -		BUG_ON(num_devices < 1);
> > -		num_devices--;
> > -	}
> > -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> > +	num_devices = btrfs_num_devices(fs_info);
> 
> How about lifting the BUG_ON from btrfs_num_devices into a check in this
> function, so if num_devices < 1 then we just exit with -EINVAL or some
> such. We should be aiming at eliminating BUG_ONs.

Right, in both cases it's possible to return with an error instead of
the BUG_ON.

Re: [PATCH v3 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

On 08/02/2018 08:21 PM, David Sterba wrote:
> On Thu, Aug 02, 2018 at 01:11:40PM +0300, Nikolay Borisov wrote:
>>
>>
>> On  2.08.2018 13:09, Anand Jain wrote:
>>> When the replace is running the fs_devices::num_devices also includes
>>> the replace device, however in some operations like device delete and
>>> balance it needs the actual num_devices without the repalce devices, so
>>> now the function btrfs_num_devices() just provides that.
>>>
>>> And here is a scenario how balance and repalce items could co-exist.
>>> Consider balance is started and paused, now start the replace
>>> followed by a power-recycle of the system. During following mount,
>>> the open_ctree() first restarts the balance so it must check for the
>>> replace device otherwise our num_devices calculation will be wrong.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>> v2->v3: update changelog with not so obvious balance and repalce
>>> co-existance secnario
>>> v1->v2: add comments
>>>
>>>   fs/btrfs/volumes.c | 32 ++++++++++++++++++--------------
>>>   1 file changed, 18 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index fe74fefc75f7..8844904f9009 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -1854,6 +1854,21 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
>>>   		fs_info->fs_devices->latest_bdev = next_device->bdev;
>>>   }
>>>   
>>> +/* Returns btrfs_fs_devices::num_devices excluding replace device if any */
>>> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
> 
> This does not need to be static inline, it's not in a header.

ok will fix.

>>> +{
>>> +	u64 num_devices = fs_info->fs_devices->num_devices;
>>> +
>>> +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>> +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>> +		BUG_ON(num_devices < 1);
>>> +		num_devices--;
>>> +	}
>>> +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>> +
>>> +	return num_devices;
>>> +}
>>> +
>>>   int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>>   		u64 devid)
>>>   {
>>> @@ -1865,13 +1880,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>>   
>>>   	mutex_lock(&uuid_mutex);
>>>   
>>> -	num_devices = fs_devices->num_devices;
>>> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>> -		BUG_ON(num_devices < 1);
>>> -		num_devices--;
>>> -	}
>>> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>> +	num_devices = btrfs_num_devices(fs_info);
>>
>> How about lifting the BUG_ON from btrfs_num_devices into a check in this
>> function, so if num_devices < 1 then we just exit with -EINVAL or some
>> such. We should be aiming at eliminating BUG_ONs.
> 
> Right, in both cases it's possible to return with an error instead of
> the BUG_ON.

Actually we should just remove it as its a logical bug if num_devices < 
1, so long we didn't hit this bug which means its stable OR keep BUG_ON 
it until we add RAID-N. ?

Thanks, Anand


> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v4 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

When the replace is running the fs_devices::num_devices also includes
the replace device, however in some operations like device delete and
balance it needs the actual num_devices without the repalce devices, so
now the function btrfs_num_devices() just provides that.

And here is a scenario how balance and repalce items could co-exist.
Consider balance is started and paused, now start the replace
followed by a power-recycle of the system. During following mount,
the open_ctree() first restarts the balance so it must check for the
replace device otherwise our num_devices calculation will be wrong.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3->v4: add comment and drop the inline (sorry missed it before)
v2->v3: update changelog with not so obvious balance and repalce
co-existance secnario
v1->v2: add comments

 fs/btrfs/volumes.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6dad9257d4ba..7359596ac8eb 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1854,6 +1854,27 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
 		fs_info->fs_devices->latest_bdev = next_device->bdev;
 }
 
+/* Returns btrfs_fs_devices::num_devices minus replace device if any */
+static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
+{
+	u64 num_devices = fs_info->fs_devices->num_devices;
+
+	/*
+	 * balance and replace co-exists in a scenario as below..
+	 * start balance and pause, start replace and reboot.
+	 * now open_ctree() restarts the balance first and checks for the
+	 * num_devices which should exclude the replace target.
+	 */
+	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
+	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
+		BUG_ON(num_devices < 1);
+		num_devices--;
+	}
+	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+
+	return num_devices;
+}
+
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		u64 devid)
 {
@@ -1865,13 +1886,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 
 	mutex_lock(&uuid_mutex);
 
-	num_devices = fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
 
 	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
 	if (ret)
@@ -3740,13 +3755,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		}
 	}
 
-	num_devices = fs_info->fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
+
 	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
 	if (num_devices > 1)
 		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
-- 
2.7.0

[PATCH] btrfs: handle the BUG_ON in btrfs_num_devices()

[Anand Jain]

Its a logical bug if we hit fs_devices::num_devices == 1 and if the
replace is running because, as fs_devices::num_devices counts the in memory
devices, so it should include the replace target which is running as
indicated by the flag. If this happens return the -EINVAL back.

Suggested-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Hi,
 As it fixes the BUG_ON I have spun a new patch for this.
 Instead of -EINVAL should we use ASSERT?

 fs/btrfs/volumes.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7359596ac8eb..ed2399caff80 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1855,9 +1855,11 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
 }
 
 /* Returns btrfs_fs_devices::num_devices minus replace device if any */
-static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
+static int btrfs_num_devices(struct btrfs_fs_info *fs_info, u64 *num_devices)
 {
-	u64 num_devices = fs_info->fs_devices->num_devices;
+	int ret = 0;
+
+	*num_devices = fs_info->fs_devices->num_devices;
 
 	/*
 	 * balance and replace co-exists in a scenario as below..
@@ -1867,12 +1869,13 @@ static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
 	 */
 	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
 	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
-		num_devices--;
+		if (*num_devices < 1)
+			ret = -EINVAL;
+		(*num_devices)--;
 	}
 	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
 
-	return num_devices;
+	return ret;
 }
 
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
@@ -1886,7 +1889,12 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 
 	mutex_lock(&uuid_mutex);
 
-	num_devices = btrfs_num_devices(fs_info);
+	ret = btrfs_num_devices(fs_info, &num_devices);
+	if (ret) {
+		btrfs_err(fs_info, "logical bug num_devices %llu < 0",
+			  num_devices);
+		return ret;
+	}
 
 	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
 	if (ret)
@@ -3755,7 +3763,12 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		}
 	}
 
-	num_devices = btrfs_num_devices(fs_info);
+	ret = btrfs_num_devices(fs_info, &num_devices);
+	if (ret) {
+		btrfs_err(fs_info, "hits a logical bug num_devices %llu < 0",
+			  num_devices);
+		return ret;
+	}
 
 	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
 	if (num_devices > 1)
-- 
2.7.0

Re: [PATCH] btrfs: handle the BUG_ON in btrfs_num_devices()

[Nikolay Borisov]

On  3.08.2018 15:45, Anand Jain wrote:
> Its a logical bug if we hit fs_devices::num_devices == 1 and if the
> replace is running because, as fs_devices::num_devices counts the in memory
> devices, so it should include the replace target which is running as
> indicated by the flag. If this happens return the -EINVAL back.
> 
> Suggested-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> Hi,
>  As it fixes the BUG_ON I have spun a new patch for this.
>  Instead of -EINVAL should we use ASSERT?
> 
>  fs/btrfs/volumes.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 7359596ac8eb..ed2399caff80 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1855,9 +1855,11 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
>  }
>  
>  /* Returns btrfs_fs_devices::num_devices minus replace device if any */
> -static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
> +static int btrfs_num_devices(struct btrfs_fs_info *fs_info, u64 *num_devices)

Why do you resort to this travesty of returning the value in an input
parameter? Having the function return int, assuming that we will always
have a positive device num and in case of an error return a negative
value. In the worst case when we get to see a btrfs fs consisting of 2
billion devices then we can start worrying that an int here won't do it.

>  {
> -	u64 num_devices = fs_info->fs_devices->num_devices;
> +	int ret = 0;
> +
> +	*num_devices = fs_info->fs_devices->num_devices;
>  
>  	/*
>  	 * balance and replace co-exists in a scenario as below..
> @@ -1867,12 +1869,13 @@ static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>  	 */
>  	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>  	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> -		BUG_ON(num_devices < 1);
> -		num_devices--;
> +		if (*num_devices < 1)
> +			ret = -EINVAL;
> +		(*num_devices)--;
>  	}
>  	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>  
> -	return num_devices;
> +	return ret;
>  }
>  
>  int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
> @@ -1886,7 +1889,12 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  
>  	mutex_lock(&uuid_mutex);
>  
> -	num_devices = btrfs_num_devices(fs_info);
> +	ret = btrfs_num_devices(fs_info, &num_devices);
> +	if (ret) {

The canonical form, used across the whole code base of btrfs, for
checking for an error is 'if (ret <0)' as such please stick to it in
this and all future patches.

(I have a vague recollection this is not the first time I have given you
this feedback)

> +		btrfs_err(fs_info, "logical bug num_devices %llu < 0",
> +			  num_devices);
> +		return ret;
> +	}
>  
>  	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>  	if (ret)
> @@ -3755,7 +3763,12 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		}
>  	}
>  
> -	num_devices = btrfs_num_devices(fs_info);
> +	ret = btrfs_num_devices(fs_info, &num_devices);
> +	if (ret) {
ditto
> +		btrfs_err(fs_info, "hits a logical bug num_devices %llu < 0",
> +			  num_devices);
> +		return ret;
> +	}
>  
>  	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>  	if (num_devices > 1)
> 

Re: [PATCH] btrfs: handle the BUG_ON in btrfs_num_devices()

[Anand Jain]

On 08/03/2018 09:33 PM, Nikolay Borisov wrote:
> 
> 
> On  3.08.2018 15:45, Anand Jain wrote:
>> Its a logical bug if we hit fs_devices::num_devices == 1 and if the
>> replace is running because, as fs_devices::num_devices counts the in memory
>> devices, so it should include the replace target which is running as
>> indicated by the flag. If this happens return the -EINVAL back.
>>
>> Suggested-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> Hi,
>>   As it fixes the BUG_ON I have spun a new patch for this.
>>   Instead of -EINVAL should we use ASSERT?
>>
>>   fs/btrfs/volumes.c | 27 ++++++++++++++++++++-------
>>   1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 7359596ac8eb..ed2399caff80 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1855,9 +1855,11 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
>>   }
>>   
>>   /* Returns btrfs_fs_devices::num_devices minus replace device if any */
>> -static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>> +static int btrfs_num_devices(struct btrfs_fs_info *fs_info, u64 *num_devices)
> 
> Why do you resort to this travesty of returning the value in an input
> parameter? Having the function return int, assuming that we will always
> have a positive device num and in case of an error return a negative
> value. In the worst case when we get to see a btrfs fs consisting of 2
> billion devices then we can start worrying that an int here won't do it.

  Its theoretically wrong. I wonder if David is OK with this? Will wait
  for his comments.

>>   {
>> -	u64 num_devices = fs_info->fs_devices->num_devices;
>> +	int ret = 0;
>> +
>> +	*num_devices = fs_info->fs_devices->num_devices;
>>   
>>   	/*
>>   	 * balance and replace co-exists in a scenario as below..
>> @@ -1867,12 +1869,13 @@ static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>>   	 */
>>   	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>   	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> -		BUG_ON(num_devices < 1);
>> -		num_devices--;
>> +		if (*num_devices < 1)
>> +			ret = -EINVAL;
>> +		(*num_devices)--;
>>   	}
>>   	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>   
>> -	return num_devices;
>> +	return ret;
>>   }
>>   
>>   int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>> @@ -1886,7 +1889,12 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   
>>   	mutex_lock(&uuid_mutex);
>>   
>> -	num_devices = btrfs_num_devices(fs_info);
>> +	ret = btrfs_num_devices(fs_info, &num_devices);
>> +	if (ret) {
> 
> The canonical form, used across the whole code base of btrfs, for
> checking for an error is 'if (ret <0)' as such please stick to it in
> this and all future patches.

  Not a big deal will fix.

> (I have a vague recollection this is not the first time I have given you
> this feedback)


>> +		btrfs_err(fs_info, "logical bug num_devices %llu < 0",
>> +			  num_devices);
>> +		return ret;
>> +	}
>>   
>>   	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>>   	if (ret)
>> @@ -3755,7 +3763,12 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>   		}
>>   	}
>>   
>> -	num_devices = btrfs_num_devices(fs_info);
>> +	ret = btrfs_num_devices(fs_info, &num_devices);
>> +	if (ret) {
> ditto

  ok.

Thanks, Anand

>> +		btrfs_err(fs_info, "hits a logical bug num_devices %llu < 0",
>> +			  num_devices);
>> +		return ret;
>> +	}
>>   
>>   	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>>   	if (num_devices > 1)
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 2/4] btrfs: fix race between free_stale_devices and close_fs_devices

[David Sterba]

On Thu, Aug 02, 2018 at 05:29:12PM +0800, Anand Jain wrote:
> 
> 
> On 08/01/2018 10:29 PM, David Sterba wrote:
> > On Thu, Jul 26, 2018 at 02:53:32PM +0800, Anand Jain wrote:
> >> From: Anand Jain <Anand.Jain@oracle.com>
> >>
> >> %fs_devices can be free-ed by btrfs_free_stale_devices() when the
> >> close_fs_devices() drops fs_devices::opened to zero, but close_fs_devices
> >> tries to access the %fs_devices again without the device_list_mutex.
> >>
> >> Fix this by bringing the %fs_devices access with in the device_list_mutex.
> > 
> > AFAICS this cannot happen anymore because the two calls are serialized
> > by the uuid_mutex. But this was not the case when syzbot reported the
> > problem where your patch would apply.
> > 
> > The parallell access to opened and device list cannot happen when:
> > 
> > * btrfs_scan_one_device that wants to call btrfs_free_stale_devices
> > * btrfs_close_devices calls close_fs_devices
> > 
> > Fixed by the series:
> > 
> > btrfs: lift uuid_mutex to callers of btrfs_scan_one_device
> > btrfs: lift uuid_mutex to callers of btrfs_open_devices
> > btrfs: lift uuid_mutex to callers of btrfs_parse_early_options
> > btrfs: reorder initialization before the mount locks uuid_mutex
> > btrfs: fix mount and ioctl device scan ioctl race
> > 
> > If there's a race I don't see, please describe in more detail.
> 
>   Right. There is no race with the uuid_mutex patches as above.
> 
>   And I just found this- can we make close be consistent with its
>   open part.
>   btrfs_open_devices() hold device_list_mutex before the update to
>   fs_devices::opened. So close_fs_device() could do the same, and be
>   theoretically correct.

Or it can be the other way around, to push the device_list_mutex only
around the list_sort and open_fs_devices like:

--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1144,15 +1144,15 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 
        lockdep_assert_held(&uuid_mutex);
 
-       mutex_lock(&fs_devices->device_list_mutex);
        if (fs_devices->opened) {
                fs_devices->opened++;
                ret = 0;
        } else {
+               mutex_lock(&fs_devices->device_list_mutex);
                list_sort(NULL, &fs_devices->devices, devid_cmp);
                ret = open_fs_devices(fs_devices, flags, holder);
+               mutex_unlock(&fs_devices->device_list_mutex);
        }
-       mutex_unlock(&fs_devices->device_list_mutex);
 
        return ret;
 }

Re: [PATCH v3 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[David Sterba]

On Thu, Aug 02, 2018 at 09:07:00PM +0800, Anand Jain wrote:
> >>> -	num_devices = fs_devices->num_devices;
> >>> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> >>> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> >>> -		BUG_ON(num_devices < 1);
> >>> -		num_devices--;
> >>> -	}
> >>> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> >>> +	num_devices = btrfs_num_devices(fs_info);
> >>
> >> How about lifting the BUG_ON from btrfs_num_devices into a check in this
> >> function, so if num_devices < 1 then we just exit with -EINVAL or some
> >> such. We should be aiming at eliminating BUG_ONs.
> > 
> > Right, in both cases it's possible to return with an error instead of
> > the BUG_ON.
> 
> Actually we should just remove it as its a logical bug if num_devices < 
> 1, so long we didn't hit this bug which means its stable OR keep BUG_ON 
> it until we add RAID-N. ?

I think the BUG_ON is redundant, all the sanity checks at mount time or
ioctl remove/replace make sure that there are enough devices to perform
the action. Still, it can be an assert, such things do not hurt and may
be catch other errors someday.

Re: [PATCH] btrfs: handle the BUG_ON in btrfs_num_devices()

[David Sterba]

On Mon, Aug 06, 2018 at 04:57:45PM +0800, Anand Jain wrote:
> 
> 
> On 08/03/2018 09:33 PM, Nikolay Borisov wrote:
> > 
> > 
> > On  3.08.2018 15:45, Anand Jain wrote:
> >> Its a logical bug if we hit fs_devices::num_devices == 1 and if the
> >> replace is running because, as fs_devices::num_devices counts the in memory
> >> devices, so it should include the replace target which is running as
> >> indicated by the flag. If this happens return the -EINVAL back.
> >>
> >> Suggested-by: Nikolay Borisov <nborisov@suse.com>
> >> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> >> ---
> >> Hi,
> >>   As it fixes the BUG_ON I have spun a new patch for this.
> >>   Instead of -EINVAL should we use ASSERT?
> >>
> >>   fs/btrfs/volumes.c | 27 ++++++++++++++++++++-------
> >>   1 file changed, 20 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >> index 7359596ac8eb..ed2399caff80 100644
> >> --- a/fs/btrfs/volumes.c
> >> +++ b/fs/btrfs/volumes.c
> >> @@ -1855,9 +1855,11 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
> >>   }
> >>   
> >>   /* Returns btrfs_fs_devices::num_devices minus replace device if any */
> >> -static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
> >> +static int btrfs_num_devices(struct btrfs_fs_info *fs_info, u64 *num_devices)
> > 
> > Why do you resort to this travesty of returning the value in an input
> > parameter? Having the function return int, assuming that we will always
> > have a positive device num and in case of an error return a negative
> > value. In the worst case when we get to see a btrfs fs consisting of 2
> > billion devices then we can start worrying that an int here won't do it.
> 
>   Its theoretically wrong. I wonder if David is OK with this? Will wait
>   for his comments.

Theoretically, as in having 2^64 devices where the numbers would clash
with error code. Practically we're talking about tens maybe hundreds of
devices, anything that fits to 32 bits in the forseeable future. Yes I
agree with Nikolai.

But the BUG_ON can be removed, or the number of devices can be checked
in advance as mentioned in other mails, I'm starting to lose track of
what's the last version.

Re: [PATCH v3 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

On 08/07/2018 11:02 PM, David Sterba wrote:
> On Thu, Aug 02, 2018 at 09:07:00PM +0800, Anand Jain wrote:
>>>>> -	num_devices = fs_devices->num_devices;
>>>>> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>>>> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>>>> -		BUG_ON(num_devices < 1);
>>>>> -		num_devices--;
>>>>> -	}
>>>>> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>>>> +	num_devices = btrfs_num_devices(fs_info);
>>>>
>>>> How about lifting the BUG_ON from btrfs_num_devices into a check in this
>>>> function, so if num_devices < 1 then we just exit with -EINVAL or some
>>>> such. We should be aiming at eliminating BUG_ONs.
>>>
>>> Right, in both cases it's possible to return with an error instead of
>>> the BUG_ON.
>>
>> Actually we should just remove it as its a logical bug if num_devices <
>> 1, so long we didn't hit this bug which means its stable OR keep BUG_ON
>> it until we add RAID-N. ?
> 
> I think the BUG_ON is redundant, all the sanity checks at mount time or
> ioctl remove/replace make sure that there are enough devices to perform
> the action. Still, it can be an assert, such things do not hurt and may
> be catch other errors someday.

  Assert makes sense to me. Patch [1] shall be dropped
  [1]
  [PATCH] btrfs: handle the BUG_ON in btrfs_num_devices()

Thanks, Anand


> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] btrfs: handle the BUG_ON in btrfs_num_devices()

[Anand Jain]

On 08/08/2018 01:09 AM, David Sterba wrote:
> On Mon, Aug 06, 2018 at 04:57:45PM +0800, Anand Jain wrote:
>>
>>
>> On 08/03/2018 09:33 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On  3.08.2018 15:45, Anand Jain wrote:
>>>> Its a logical bug if we hit fs_devices::num_devices == 1 and if the
>>>> replace is running because, as fs_devices::num_devices counts the in memory
>>>> devices, so it should include the replace target which is running as
>>>> indicated by the flag. If this happens return the -EINVAL back.
>>>>
>>>> Suggested-by: Nikolay Borisov <nborisov@suse.com>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>> Hi,
>>>>    As it fixes the BUG_ON I have spun a new patch for this.
>>>>    Instead of -EINVAL should we use ASSERT?
>>>>
>>>>    fs/btrfs/volumes.c | 27 ++++++++++++++++++++-------
>>>>    1 file changed, 20 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index 7359596ac8eb..ed2399caff80 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -1855,9 +1855,11 @@ void btrfs_assign_next_active_device(struct btrfs_device *device,
>>>>    }
>>>>    
>>>>    /* Returns btrfs_fs_devices::num_devices minus replace device if any */
>>>> -static u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>>>> +static int btrfs_num_devices(struct btrfs_fs_info *fs_info, u64 *num_devices)
>>>
>>> Why do you resort to this travesty of returning the value in an input
>>> parameter? Having the function return int, assuming that we will always
>>> have a positive device num and in case of an error return a negative
>>> value. In the worst case when we get to see a btrfs fs consisting of 2
>>> billion devices then we can start worrying that an int here won't do it.
>>
>>    Its theoretically wrong. I wonder if David is OK with this? Will wait
>>    for his comments.
> 
> Theoretically, as in having 2^64 devices where the numbers would clash
> with error code. Practically we're talking about tens maybe hundreds of
> devices, anything that fits to 32 bits in the forseeable future. Yes I
> agree with Nikolai.
> 
> But the BUG_ON can be removed, or the number of devices can be checked
> in advance as mentioned in other mails, I'm starting to lose track of
> what's the last version.

  Other email thread [1]  were talking about replacing the BUG_ON with
  ASSERT, which means we don't need this patch.

[1]
[PATCH v3 4/4] btrfs: add helper btrfs_num_devices() to deduce num_devices


Thanks, Anand

Re: [PATCH v2 2/4] btrfs: fix race between free_stale_devices and close_fs_devices

[Anand Jain]

On 08/07/2018 10:59 PM, David Sterba wrote:
> On Thu, Aug 02, 2018 at 05:29:12PM +0800, Anand Jain wrote:
>>
>>
>> On 08/01/2018 10:29 PM, David Sterba wrote:
>>> On Thu, Jul 26, 2018 at 02:53:32PM +0800, Anand Jain wrote:
>>>> From: Anand Jain <Anand.Jain@oracle.com>
>>>>
>>>> %fs_devices can be free-ed by btrfs_free_stale_devices() when the
>>>> close_fs_devices() drops fs_devices::opened to zero, but close_fs_devices
>>>> tries to access the %fs_devices again without the device_list_mutex.
>>>>
>>>> Fix this by bringing the %fs_devices access with in the device_list_mutex.
>>>
>>> AFAICS this cannot happen anymore because the two calls are serialized
>>> by the uuid_mutex. But this was not the case when syzbot reported the
>>> problem where your patch would apply.
>>>
>>> The parallell access to opened and device list cannot happen when:
>>>
>>> * btrfs_scan_one_device that wants to call btrfs_free_stale_devices
>>> * btrfs_close_devices calls close_fs_devices
>>>
>>> Fixed by the series:
>>>
>>> btrfs: lift uuid_mutex to callers of btrfs_scan_one_device
>>> btrfs: lift uuid_mutex to callers of btrfs_open_devices
>>> btrfs: lift uuid_mutex to callers of btrfs_parse_early_options
>>> btrfs: reorder initialization before the mount locks uuid_mutex
>>> btrfs: fix mount and ioctl device scan ioctl race
>>>
>>> If there's a race I don't see, please describe in more detail.
>>
>>    Right. There is no race with the uuid_mutex patches as above.
>>
>>    And I just found this- can we make close be consistent with its
>>    open part.
>>    btrfs_open_devices() hold device_list_mutex before the update to
>>    fs_devices::opened. So close_fs_device() could do the same, and be
>>    theoretically correct.
> 
> Or it can be the other way around, to push the device_list_mutex only
> around the list_sort and open_fs_devices like:

> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1144,15 +1144,15 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>   
>          lockdep_assert_held(&uuid_mutex);
>   
> -       mutex_lock(&fs_devices->device_list_mutex);
>          if (fs_devices->opened) {
>                  fs_devices->opened++;
>                  ret = 0;
>          } else {
> +               mutex_lock(&fs_devices->device_list_mutex);
>                  list_sort(NULL, &fs_devices->devices, devid_cmp);
>                  ret = open_fs_devices(fs_devices, flags, holder);
> +               mutex_unlock(&fs_devices->device_list_mutex);
>          }
> -       mutex_unlock(&fs_devices->device_list_mutex);
>   
>          return ret;
>   }
> 

  Right. That it will be also true in terms of consistency. And I looked
  around I don't find any reason why not.

  Next question - btrfs_free_stale_devices() need a better way to check
  if the device is opened by the FS. Currently it relays on the
  fs_devices::opened.
  And while doing that, both uuid_mutex and device_list_mutex are held
  and since we are depend on uuid_mutex we don't need device_list_mutex.
  uuid_mutex is too gross could stall operation on other FSID.

  We can drop this patch. Do you want a patch to fix the
  btrfs_open_devices() consistency?

Thanks, Anand