[PATCH v2] Revert "btrfs: fix a possible umount deadlock"

[PATCH v2] Revert "btrfs: fix a possible umount deadlock"

[Nikolay Borisov]

Since commit 88c14590cdd6 ("btrfs: use RCU in btrfs_show_devname for
device list traversal") btrfs_show_devname no longer takes
device_list_mutex. As such the deadlock that 0ccd05285e7f ("btrfs: fix
a possible umount deadlock") aimed to fix no longer exists. So remove
the extra code that commit added.

This reverts commit 0ccd05285e7f5a8e297e1d6dfc41e7c65757d6fa.

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

V2: 
 * Fixed build failure due to using old name of free_device_rcu function. 
 fs/btrfs/volumes.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5bd6f3a40f9c..011a19b7930f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1004,7 +1004,7 @@ static void btrfs_close_bdev(struct btrfs_device *device)
 	blkdev_put(device->bdev, device->mode);
 }
 
-static void btrfs_prepare_close_one_device(struct btrfs_device *device)
+static void btrfs_close_one_device(struct btrfs_device *device)
 {
 	struct btrfs_fs_devices *fs_devices = device->fs_devices;
 	struct btrfs_device *new_device;
@@ -1022,6 +1022,8 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device)
 	if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
 		fs_devices->missing_devices--;
 
+	btrfs_close_bdev(device);
+
 	new_device = btrfs_alloc_device(NULL, &device->devid,
 					device->uuid);
 	BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
@@ -1035,39 +1037,23 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device)
 
 	list_replace_rcu(&device->dev_list, &new_device->dev_list);
 	new_device->fs_devices = device->fs_devices;
+
+	call_rcu(&device->rcu, free_device_rcu);
 }
 
 static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_device *device, *tmp;
-	struct list_head pending_put;
-
-	INIT_LIST_HEAD(&pending_put);
 
 	if (--fs_devices->opened > 0)
 		return 0;
 
 	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
-		btrfs_prepare_close_one_device(device);
-		list_add(&device->dev_list, &pending_put);
+		btrfs_close_one_device(device);
 	}
 	mutex_unlock(&fs_devices->device_list_mutex);
 
-	/*
-	 * btrfs_show_devname() is using the device_list_mutex,
-	 * sometimes call to blkdev_put() leads vfs calling
-	 * into this func. So do put outside of device_list_mutex,
-	 * as of now.
-	 */
-	while (!list_empty(&pending_put)) {
-		device = list_first_entry(&pending_put,
-				struct btrfs_device, dev_list);
-		list_del(&device->dev_list);
-		btrfs_close_bdev(device);
-		call_rcu(&device->rcu, free_device_rcu);
-	}
-
 	WARN_ON(fs_devices->open_devices);
 	WARN_ON(fs_devices->rw_devices);
 	fs_devices->opened = 0;
-- 
2.7.4

Re: [PATCH v2] Revert "btrfs: fix a possible umount deadlock"

[Anand Jain]

On 06/29/2018 01:26 PM, Nikolay Borisov wrote:
> Since commit 88c14590cdd6 ("btrfs: use RCU in btrfs_show_devname for
> device list traversal") btrfs_show_devname no longer takes
> device_list_mutex. As such the deadlock that 0ccd05285e7f ("btrfs: fix
> a possible umount deadlock") aimed to fix no longer exists. So remove
> the extra code that commit added.
> 
> This reverts commit 0ccd05285e7f5a8e297e1d6dfc41e7c65757d6fa.

  Its not exactly a revert.

  Because before 88c14590cdd6 was written, 0ccd05285e7f was already their
  for the right purpose OR a new title is even better.


 > Why do we first replace the closed device with a copy in
 > btrfs_close_one_device, then dispose of the copied
 > devices in btrfs_close_devices IFF we had fs_devices->seed not being
 > NULL?

  I doubt if its for the fs_devices->seed purpose, we could have
  just NULLed few items in device and it would have worked
  as well. I guess it for the purpose of RCU satisfactions,
  I remember running into that issues while trying to fix.
  If you have plans to fix that pls go ahead. Thanks. While
  my time is occupied with volume locks as of now. Otherwise
  its in the list to fix.

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

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

Thanks, Anand

> ---
> 
> V2:
>   * Fixed build failure due to using old name of free_device_rcu function.
>   fs/btrfs/volumes.c | 26 ++++++--------------------
>   1 file changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5bd6f3a40f9c..011a19b7930f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1004,7 +1004,7 @@ static void btrfs_close_bdev(struct btrfs_device *device)
>   	blkdev_put(device->bdev, device->mode);
>   }
>   
> -static void btrfs_prepare_close_one_device(struct btrfs_device *device)
> +static void btrfs_close_one_device(struct btrfs_device *device)
>   {
>   	struct btrfs_fs_devices *fs_devices = device->fs_devices;
>   	struct btrfs_device *new_device;
> @@ -1022,6 +1022,8 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device)
>   	if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
>   		fs_devices->missing_devices--;
>   
> +	btrfs_close_bdev(device);
> +
>   	new_device = btrfs_alloc_device(NULL, &device->devid,
>   					device->uuid);
>   	BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
> @@ -1035,39 +1037,23 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device)
>   
>   	list_replace_rcu(&device->dev_list, &new_device->dev_list);
>   	new_device->fs_devices = device->fs_devices;
> +
> +	call_rcu(&device->rcu, free_device_rcu);
>   }
>   
>   static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
>   {
>   	struct btrfs_device *device, *tmp;
> -	struct list_head pending_put;
> -
> -	INIT_LIST_HEAD(&pending_put);
>   
>   	if (--fs_devices->opened > 0)
>   		return 0;
>   
>   	mutex_lock(&fs_devices->device_list_mutex);
>   	list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) {
> -		btrfs_prepare_close_one_device(device);
> -		list_add(&device->dev_list, &pending_put);
> +		btrfs_close_one_device(device);
>   	}
>   	mutex_unlock(&fs_devices->device_list_mutex);
>   
> -	/*
> -	 * btrfs_show_devname() is using the device_list_mutex,
> -	 * sometimes call to blkdev_put() leads vfs calling
> -	 * into this func. So do put outside of device_list_mutex,
> -	 * as of now.
> -	 */
> -	while (!list_empty(&pending_put)) {
> -		device = list_first_entry(&pending_put,
> -				struct btrfs_device, dev_list);
> -		list_del(&device->dev_list);
> -		btrfs_close_bdev(device);
> -		call_rcu(&device->rcu, free_device_rcu);
> -	}
> -
>   	WARN_ON(fs_devices->open_devices);
>   	WARN_ON(fs_devices->rw_devices);
>   	fs_devices->opened = 0;
> 

Re: [PATCH v2] Revert "btrfs: fix a possible umount deadlock"

[Nikolay Borisov]

On 29.06.2018 10:11, Anand Jain wrote:
> 
> On 06/29/2018 01:26 PM, Nikolay Borisov wrote:
>> Since commit 88c14590cdd6 ("btrfs: use RCU in btrfs_show_devname for
>> device list traversal") btrfs_show_devname no longer takes
>> device_list_mutex. As such the deadlock that 0ccd05285e7f ("btrfs: fix
>> a possible umount deadlock") aimed to fix no longer exists. So remove
>> the extra code that commit added.
>>
>> This reverts commit 0ccd05285e7f5a8e297e1d6dfc41e7c65757d6fa.
> 
>  Its not exactly a revert.
> 
>  Because before 88c14590cdd6 was written, 0ccd05285e7f was already their
>  for the right purpose OR a new title is even better.

What I'm saying is that commit 88c14590cdd6 obsoleted 0ccd05285e7f,
hence this patch reverts the latter. I've literally generated it with
git revert 0ccd05285e7f + minor fixups.

> 
> 
>> Why do we first replace the closed device with a copy in
>> btrfs_close_one_device, then dispose of the copied
>> devices in btrfs_close_devices IFF we had fs_devices->seed not being
>> NULL?
> 
>  I doubt if its for the fs_devices->seed purpose, we could have
>  just NULLed few items in device and it would have worked
>  as well. I guess it for the purpose of RCU satisfactions,
>  I remember running into that issues while trying to fix.
>  If you have plans to fix that pls go ahead. Thanks. While
>  my time is occupied with volume locks as of now. Otherwise
>  its in the list to fix.

Yeah , I spoke with David about that and he said it was related to RCU
and of course the commit that introduced that had nothing about RCU in
the change log. Device close logic seems to be a train wreck atm.
> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> Reviewed-by: Anand Jain <anand.jain@oracle.com>
> 
> Thanks, Anand
> 
>> ---
>>
>> V2:
>>   * Fixed build failure due to using old name of free_device_rcu
>> function.
>>   fs/btrfs/volumes.c | 26 ++++++--------------------
>>   1 file changed, 6 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 5bd6f3a40f9c..011a19b7930f 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1004,7 +1004,7 @@ static void btrfs_close_bdev(struct btrfs_device
>> *device)
>>       blkdev_put(device->bdev, device->mode);
>>   }
>>   -static void btrfs_prepare_close_one_device(struct btrfs_device
>> *device)
>> +static void btrfs_close_one_device(struct btrfs_device *device)
>>   {
>>       struct btrfs_fs_devices *fs_devices = device->fs_devices;
>>       struct btrfs_device *new_device;
>> @@ -1022,6 +1022,8 @@ static void
>> btrfs_prepare_close_one_device(struct btrfs_device *device)
>>       if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
>>           fs_devices->missing_devices--;
>>   +    btrfs_close_bdev(device);
>> +
>>       new_device = btrfs_alloc_device(NULL, &device->devid,
>>                       device->uuid);
>>       BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
>> @@ -1035,39 +1037,23 @@ static void
>> btrfs_prepare_close_one_device(struct btrfs_device *device)
>>         list_replace_rcu(&device->dev_list, &new_device->dev_list);
>>       new_device->fs_devices = device->fs_devices;
>> +
>> +    call_rcu(&device->rcu, free_device_rcu);
>>   }
>>     static int close_fs_devices(struct btrfs_fs_devices *fs_devices)
>>   {
>>       struct btrfs_device *device, *tmp;
>> -    struct list_head pending_put;
>> -
>> -    INIT_LIST_HEAD(&pending_put);
>>         if (--fs_devices->opened > 0)
>>           return 0;
>>         mutex_lock(&fs_devices->device_list_mutex);
>>       list_for_each_entry_safe(device, tmp, &fs_devices->devices,
>> dev_list) {
>> -        btrfs_prepare_close_one_device(device);
>> -        list_add(&device->dev_list, &pending_put);
>> +        btrfs_close_one_device(device);
>>       }
>>       mutex_unlock(&fs_devices->device_list_mutex);
>>   -    /*
>> -     * btrfs_show_devname() is using the device_list_mutex,
>> -     * sometimes call to blkdev_put() leads vfs calling
>> -     * into this func. So do put outside of device_list_mutex,
>> -     * as of now.
>> -     */
>> -    while (!list_empty(&pending_put)) {
>> -        device = list_first_entry(&pending_put,
>> -                struct btrfs_device, dev_list);
>> -        list_del(&device->dev_list);
>> -        btrfs_close_bdev(device);
>> -        call_rcu(&device->rcu, free_device_rcu);
>> -    }
>> -
>>       WARN_ON(fs_devices->open_devices);
>>       WARN_ON(fs_devices->rw_devices);
>>       fs_devices->opened = 0;
>>
> 

Re: [PATCH v2] Revert "btrfs: fix a possible umount deadlock"

[David Sterba]

On Fri, Jun 29, 2018 at 10:13:44AM +0300, Nikolay Borisov wrote:
> On 29.06.2018 10:11, Anand Jain wrote:
> > 
> > On 06/29/2018 01:26 PM, Nikolay Borisov wrote:
> >> Since commit 88c14590cdd6 ("btrfs: use RCU in btrfs_show_devname for
> >> device list traversal") btrfs_show_devname no longer takes
> >> device_list_mutex. As such the deadlock that 0ccd05285e7f ("btrfs: fix
> >> a possible umount deadlock") aimed to fix no longer exists. So remove
> >> the extra code that commit added.
> >>
> >> This reverts commit 0ccd05285e7f5a8e297e1d6dfc41e7c65757d6fa.
> > 
> >  Its not exactly a revert.
> > 
> >  Because before 88c14590cdd6 was written, 0ccd05285e7f was already their
> >  for the right purpose OR a new title is even better.
> 
> What I'm saying is that commit 88c14590cdd6 obsoleted 0ccd05285e7f,
> hence this patch reverts the latter. I've literally generated it with
> git revert 0ccd05285e7f + minor fixups.

I do not recommend to do a revert here.  Even if a patch reverts
functionality because it's not needed anymore, then it's a "forward"
change.  There are also other changes that may affect the behaviour and
in this case it's 47dba17171a76ea2a2a71 that removes rcu barrier, so the
patch has to be put into the context of current code.

The commit is almost 2 years old, the idea of reverts is IMHO more to
provide an easy way do a small step back during one devlopment cycle
when the moving parts are still in sight.

Back then the commit fixed a deadlock, a revert here would read as 'ok,
we want the deadlock back'.

So, the code is ok. The subject needs to drop the word 'revert' and
changelg maybe mention a few more references why the logic is not needed
anymore.