* [PATCH] btrfs: add a warning to check on the leaking device close
@ 2020-09-14 9:11 Anand Jain
2020-09-16 10:10 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2020-09-14 9:11 UTC (permalink / raw)
To: linux-btrfs; +Cc: Anand Jain
To help better understand the device-close leaks, add a warning if the
device freed is still open.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/volumes.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2d5cc644741e..c0dfc0b569e9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -371,6 +371,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
void btrfs_free_device(struct btrfs_device *device)
{
WARN_ON(!list_empty(&device->post_commit_list));
+ WARN_ON(device->bdev);
rcu_string_free(device->name);
extent_io_tree_release(&device->alloc_state);
bio_put(device->flush_bio);
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: add a warning to check on the leaking device close
2020-09-14 9:11 [PATCH] btrfs: add a warning to check on the leaking device close Anand Jain
@ 2020-09-16 10:10 ` David Sterba
2020-09-16 11:51 ` Anand Jain
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2020-09-16 10:10 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs
On Mon, Sep 14, 2020 at 05:11:14PM +0800, Anand Jain wrote:
> To help better understand the device-close leaks, add a warning if the
> device freed is still open.
Have you seen that happen or is it just a precaution? I've checked where
the bdev is set to NULL and all paths seem to be covered, so the warn_on
does not harm anything just that it does not seem to be possible to hit.
For that an assert would be better.
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> fs/btrfs/volumes.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 2d5cc644741e..c0dfc0b569e9 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -371,6 +371,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
> void btrfs_free_device(struct btrfs_device *device)
> {
> WARN_ON(!list_empty(&device->post_commit_list));
> + WARN_ON(device->bdev);
> rcu_string_free(device->name);
> extent_io_tree_release(&device->alloc_state);
> bio_put(device->flush_bio);
> --
> 2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: add a warning to check on the leaking device close
2020-09-16 10:10 ` David Sterba
@ 2020-09-16 11:51 ` Anand Jain
2020-09-28 18:16 ` Josef Bacik
0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2020-09-16 11:51 UTC (permalink / raw)
To: dsterba, linux-btrfs
On 16/9/20 6:10 pm, David Sterba wrote:
> On Mon, Sep 14, 2020 at 05:11:14PM +0800, Anand Jain wrote:
>> To help better understand the device-close leaks, add a warning if the
>> device freed is still open.
>
> Have you seen that happen or is it just a precaution? I've checked where
> the bdev is set to NULL and all paths seem to be covered, so the warn_on
> does not harm anything just that it does not seem to be possible to hit.
> For that an assert would be better.
There is an early/unconfirmed report [1] that after the forget
sub-command a device had partition changes and the new partitions failed
to recognize by the kernel.
[1]
https://lore.kernel.org/linux-btrfs/40e2315e-e60e-6161-ceb7-acd8b8a4e4a0@oracle.com/T/#t
The mount thread can't use device_list_mutex (because of bd_mutex),
and we rely on the uuid_mutex during mount.
The forget thread used both uuid_mutex and device_list_mutex.
So there isn't race between these two.
As of now we don't know. So the warning will help to know if we are
missing something.
Thanks, Anand
>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> fs/btrfs/volumes.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 2d5cc644741e..c0dfc0b569e9 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -371,6 +371,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
>> void btrfs_free_device(struct btrfs_device *device)
>> {
>> WARN_ON(!list_empty(&device->post_commit_list));
>> + WARN_ON(device->bdev);
>> rcu_string_free(device->name);
>> extent_io_tree_release(&device->alloc_state);
>> bio_put(device->flush_bio);
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: add a warning to check on the leaking device close
2020-09-16 11:51 ` Anand Jain
@ 2020-09-28 18:16 ` Josef Bacik
2021-11-08 19:58 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Josef Bacik @ 2020-09-28 18:16 UTC (permalink / raw)
To: Anand Jain, dsterba, linux-btrfs
On 9/16/20 7:51 AM, Anand Jain wrote:
>
>
> On 16/9/20 6:10 pm, David Sterba wrote:
>> On Mon, Sep 14, 2020 at 05:11:14PM +0800, Anand Jain wrote:
>>> To help better understand the device-close leaks, add a warning if the
>>> device freed is still open.
>>
>> Have you seen that happen or is it just a precaution? I've checked where
>> the bdev is set to NULL and all paths seem to be covered, so the warn_on
>> does not harm anything just that it does not seem to be possible to hit.
>> For that an assert would be better.
>
> There is an early/unconfirmed report [1] that after the forget
> sub-command a device had partition changes and the new partitions failed
> to recognize by the kernel.
> [1]
> https://lore.kernel.org/linux-btrfs/40e2315e-e60e-6161-ceb7-acd8b8a4e4a0@oracle.com/T/#t
>
>
> The mount thread can't use device_list_mutex (because of bd_mutex),
> and we rely on the uuid_mutex during mount.
>
> The forget thread used both uuid_mutex and device_list_mutex.
>
> So there isn't race between these two.
>
> As of now we don't know. So the warning will help to know if we are
> missing something.
>
It is clear that it can't really happen, but if we're worried about it I'd
rather it be an ASSERT(). Thanks,
Josef
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: add a warning to check on the leaking device close
2020-09-28 18:16 ` Josef Bacik
@ 2021-11-08 19:58 ` David Sterba
2021-11-09 8:55 ` Anand Jain
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2021-11-08 19:58 UTC (permalink / raw)
To: Josef Bacik; +Cc: Anand Jain, dsterba, linux-btrfs
On Mon, Sep 28, 2020 at 02:16:34PM -0400, Josef Bacik wrote:
> On 9/16/20 7:51 AM, Anand Jain wrote:
> >
> >
> > On 16/9/20 6:10 pm, David Sterba wrote:
> >> On Mon, Sep 14, 2020 at 05:11:14PM +0800, Anand Jain wrote:
> >>> To help better understand the device-close leaks, add a warning if the
> >>> device freed is still open.
> >>
> >> Have you seen that happen or is it just a precaution? I've checked where
> >> the bdev is set to NULL and all paths seem to be covered, so the warn_on
> >> does not harm anything just that it does not seem to be possible to hit.
> >> For that an assert would be better.
> >
> > There is an early/unconfirmed report [1] that after the forget
> > sub-command a device had partition changes and the new partitions failed
> > to recognize by the kernel.
> > [1]
> > https://lore.kernel.org/linux-btrfs/40e2315e-e60e-6161-ceb7-acd8b8a4e4a0@oracle.com/T/#t
> >
> >
> > The mount thread can't use device_list_mutex (because of bd_mutex),
> > and we rely on the uuid_mutex during mount.
> >
> > The forget thread used both uuid_mutex and device_list_mutex.
> >
> > So there isn't race between these two.
> >
> > As of now we don't know. So the warning will help to know if we are
> > missing something.
> >
>
> It is clear that it can't really happen, but if we're worried about it I'd
> rather it be an ASSERT(). Thanks,
I'm going through the patch backlog and the patch may be still relevant
but a lot of device locking code has changed recently.
Anand, if this is still relevant, please resend, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: add a warning to check on the leaking device close
2021-11-08 19:58 ` David Sterba
@ 2021-11-09 8:55 ` Anand Jain
0 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2021-11-09 8:55 UTC (permalink / raw)
To: dsterba, Josef Bacik, linux-btrfs
On 9/11/21 3:58 am, David Sterba wrote:
> On Mon, Sep 28, 2020 at 02:16:34PM -0400, Josef Bacik wrote:
>> On 9/16/20 7:51 AM, Anand Jain wrote:
>>>
>>>
>>> On 16/9/20 6:10 pm, David Sterba wrote:
>>>> On Mon, Sep 14, 2020 at 05:11:14PM +0800, Anand Jain wrote:
>>>>> To help better understand the device-close leaks, add a warning if the
>>>>> device freed is still open.
>>>>
>>>> Have you seen that happen or is it just a precaution? I've checked where
>>>> the bdev is set to NULL and all paths seem to be covered, so the warn_on
>>>> does not harm anything just that it does not seem to be possible to hit.
>>>> For that an assert would be better.
>>>
>>> There is an early/unconfirmed report [1] that after the forget
>>> sub-command a device had partition changes and the new partitions failed
>>> to recognize by the kernel.
>>> [1]
>>> https://lore.kernel.org/linux-btrfs/40e2315e-e60e-6161-ceb7-acd8b8a4e4a0@oracle.com/T/#t
>>>
>>>
>>> The mount thread can't use device_list_mutex (because of bd_mutex),
>>> and we rely on the uuid_mutex during mount.
>>>
>>> The forget thread used both uuid_mutex and device_list_mutex.
>>>
>>> So there isn't race between these two.
>>>
>>> As of now we don't know. So the warning will help to know if we are
>>> missing something.
>>>
>>
>> It is clear that it can't really happen, but if we're worried about it I'd
>> rather it be an ASSERT(). Thanks,
>
> I'm going through the patch backlog and the patch may be still relevant
> but a lot of device locking code has changed recently.
>
> Anand, if this is still relevant, please resend, thanks.
>
This patch is not relevant anymore. The commit 3fa421dedbc8 ("btrfs:
delay blkdev_put until after the device removes") introduced blkdev_put
after the device-free. So pls ignore this patch.
Thanks, Anand
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-09 8:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-14 9:11 [PATCH] btrfs: add a warning to check on the leaking device close Anand Jain
2020-09-16 10:10 ` David Sterba
2020-09-16 11:51 ` Anand Jain
2020-09-28 18:16 ` Josef Bacik
2021-11-08 19:58 ` David Sterba
2021-11-09 8:55 ` Anand Jain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).