* [PATCH 1/6] btrfs: cleanup btrfs_free_stale_device() usage
2017-12-15 3:47 [PATCH 0/6] preparatory work to add device forget Anand Jain
@ 2017-12-15 3:47 ` Anand Jain
2017-12-15 15:33 ` Nikolay Borisov
2017-12-15 3:47 ` [PATCH 2/6] btrfs: no need to check for btrfs_fs_devices::seeding Anand Jain
` (4 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Anand Jain @ 2017-12-15 3:47 UTC (permalink / raw)
To: linux-btrfs
We call btrfs_free_stale_device() only when we alloc a new
struct btrfs_device (ret=1), so move it closer to where we
alloc the new device. Also drop the comments.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/volumes.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index b8ba3de6e9e6..6317a3561ae1 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -788,6 +788,7 @@ static noinline int device_list_add(const char *path,
ret = 1;
device->fs_devices = fs_devices;
+ btrfs_free_stale_device(device);
} else if (!device->name || strcmp(device->name->str, path)) {
/*
* When FS is already mounted.
@@ -846,13 +847,6 @@ static noinline int device_list_add(const char *path,
if (!fs_devices->opened)
device->generation = found_transid;
- /*
- * if there is new btrfs on an already registered device,
- * then remove the stale device entry.
- */
- if (ret > 0)
- btrfs_free_stale_device(device);
-
*fs_devices_ret = fs_devices;
return ret;
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/6] btrfs: cleanup btrfs_free_stale_device() usage
2017-12-15 3:47 ` [PATCH 1/6] btrfs: cleanup btrfs_free_stale_device() usage Anand Jain
@ 2017-12-15 15:33 ` Nikolay Borisov
2017-12-16 2:15 ` Anand Jain
0 siblings, 1 reply; 20+ messages in thread
From: Nikolay Borisov @ 2017-12-15 15:33 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 15.12.2017 05:47, Anand Jain wrote:
> We call btrfs_free_stale_device() only when we alloc a new
> struct btrfs_device (ret=1), so move it closer to where we
> alloc the new device. Also drop the comments.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> fs/btrfs/volumes.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index b8ba3de6e9e6..6317a3561ae1 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -788,6 +788,7 @@ static noinline int device_list_add(const char *path,
nit: not directly related to the series in question, but I think you can
add one more patch which sinks the devid argument passed to
device_list_add. We already pass the disk_super and we can get the devid
in device_list_add this reduced the cognitive load when reading the code
>
> ret = 1;
> device->fs_devices = fs_devices;
> + btrfs_free_stale_device(device);
> } else if (!device->name || strcmp(device->name->str, path)) {
> /*
> * When FS is already mounted.
> @@ -846,13 +847,6 @@ static noinline int device_list_add(const char *path,
> if (!fs_devices->opened)
> device->generation = found_transid;
>
> - /*
> - * if there is new btrfs on an already registered device,
> - * then remove the stale device entry.
> - */
> - if (ret > 0)
> - btrfs_free_stale_device(device);
> -
> *fs_devices_ret = fs_devices;
>
> return ret;
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/6] btrfs: cleanup btrfs_free_stale_device() usage
2017-12-15 15:33 ` Nikolay Borisov
@ 2017-12-16 2:15 ` Anand Jain
0 siblings, 0 replies; 20+ messages in thread
From: Anand Jain @ 2017-12-16 2:15 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
>> @@ -788,6 +788,7 @@ static noinline int device_list_add(const char *path,
>
> nit: not directly related to the series in question, but I think you can
> add one more patch which sinks the devid argument passed to
> device_list_add. We already pass the disk_super and we can get the devid
> in device_list_add this reduced the cognitive load when reading the code
I notice that too, yes it should be in a new patch. More cleanups
around this is coming up.
Thanks, Anand
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/6] btrfs: no need to check for btrfs_fs_devices::seeding
2017-12-15 3:47 [PATCH 0/6] preparatory work to add device forget Anand Jain
2017-12-15 3:47 ` [PATCH 1/6] btrfs: cleanup btrfs_free_stale_device() usage Anand Jain
@ 2017-12-15 3:47 ` Anand Jain
2017-12-15 3:47 ` [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales Anand Jain
` (3 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Anand Jain @ 2017-12-15 3:47 UTC (permalink / raw)
To: linux-btrfs
There is no need to check for btrfs_fs_devices::seeding when we
have checked for btrfs_fs_devices::opened, because we can't sprout
without its seed FS being opened.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/volumes.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6317a3561ae1..0e89409112d5 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -621,8 +621,6 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
if (fs_devs->opened)
continue;
- if (fs_devs->seeding)
- continue;
list_for_each_entry(dev, &fs_devs->devices, dev_list) {
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales
2017-12-15 3:47 [PATCH 0/6] preparatory work to add device forget Anand Jain
2017-12-15 3:47 ` [PATCH 1/6] btrfs: cleanup btrfs_free_stale_device() usage Anand Jain
2017-12-15 3:47 ` [PATCH 2/6] btrfs: no need to check for btrfs_fs_devices::seeding Anand Jain
@ 2017-12-15 3:47 ` Anand Jain
2017-12-15 15:06 ` Nikolay Borisov
2017-12-15 3:47 ` [PATCH 4/6] btrfs: make btrfs_free_stale_device() argument optional Anand Jain
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Anand Jain @ 2017-12-15 3:47 UTC (permalink / raw)
To: linux-btrfs
Let the list iterator iterate further and find other stale
devices and delete it. This is in preparation to add support
for user land request-able stale devices cleanup.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/volumes.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0e89409112d5..70db6a1d5658 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -610,19 +610,20 @@ static void pending_bios_fn(struct btrfs_work *work)
static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
{
- struct btrfs_fs_devices *fs_devs;
- struct btrfs_device *dev;
+ struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
+ struct btrfs_device *dev, *tmp_dev;
if (!cur_dev->name)
return;
- list_for_each_entry(fs_devs, &fs_uuids, list) {
- int del = 1;
+ list_for_each_entry_safe(fs_devs, tmp_fs_devs, &fs_uuids, list) {
if (fs_devs->opened)
continue;
- list_for_each_entry(dev, &fs_devs->devices, dev_list) {
+ list_for_each_entry_safe(dev, tmp_dev,
+ &fs_devs->devices, dev_list) {
+ int not_found;
if (dev == cur_dev)
continue;
@@ -636,14 +637,12 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
* either use mapper or non mapper path throughout.
*/
rcu_read_lock();
- del = strcmp(rcu_str_deref(dev->name),
+ not_found = strcmp(rcu_str_deref(dev->name),
rcu_str_deref(cur_dev->name));
rcu_read_unlock();
- if (!del)
- break;
- }
+ if (not_found)
+ continue;
- if (!del) {
/* delete the stale device */
if (fs_devs->num_devices == 1) {
btrfs_sysfs_remove_fsid(fs_devs);
@@ -654,7 +653,6 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
list_del(&dev->dev_list);
free_device(dev);
}
- break;
}
}
}
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales
2017-12-15 3:47 ` [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales Anand Jain
@ 2017-12-15 15:06 ` Nikolay Borisov
2017-12-16 2:13 ` Anand Jain
0 siblings, 1 reply; 20+ messages in thread
From: Nikolay Borisov @ 2017-12-15 15:06 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 15.12.2017 05:47, Anand Jain wrote:
> Let the list iterator iterate further and find other stale
> devices and delete it. This is in preparation to add support
> for user land request-able stale devices cleanup.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
What is the lock protection of this function - uuid_mutex, it's not
really obvious. Perhaps adding a lockdep_assert_held for the correct
lock ? I guess David's earlier patch to document the locking in
volumes.c might shed some light on this one.
> ---
> fs/btrfs/volumes.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 0e89409112d5..70db6a1d5658 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -610,19 +610,20 @@ static void pending_bios_fn(struct btrfs_work *work)
>
> static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
> {
> - struct btrfs_fs_devices *fs_devs;
> - struct btrfs_device *dev;
> + struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
> + struct btrfs_device *dev, *tmp_dev;
>
> if (!cur_dev->name)
> return;
>
> - list_for_each_entry(fs_devs, &fs_uuids, list) {
> - int del = 1;
> + list_for_each_entry_safe(fs_devs, tmp_fs_devs, &fs_uuids, list) {
>
> if (fs_devs->opened)
> continue;
>
> - list_for_each_entry(dev, &fs_devs->devices, dev_list) {
> + list_for_each_entry_safe(dev, tmp_dev,
> + &fs_devs->devices, dev_list) {
> + int not_found;
>
> if (dev == cur_dev)
> continue;
> @@ -636,14 +637,12 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
> * either use mapper or non mapper path throughout.
> */
> rcu_read_lock();
> - del = strcmp(rcu_str_deref(dev->name),
> + not_found = strcmp(rcu_str_deref(dev->name),
> rcu_str_deref(cur_dev->name));
> rcu_read_unlock();
> - if (!del)
> - break;
> - }
> + if (not_found)
> + continue;
>
> - if (!del) {
> /* delete the stale device */
> if (fs_devs->num_devices == 1) {
> btrfs_sysfs_remove_fsid(fs_devs);
> @@ -654,7 +653,6 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
> list_del(&dev->dev_list);
> free_device(dev);
> }
> - break;
> }
> }
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales
2017-12-15 15:06 ` Nikolay Borisov
@ 2017-12-16 2:13 ` Anand Jain
2017-12-16 6:54 ` Nikolay Borisov
0 siblings, 1 reply; 20+ messages in thread
From: Anand Jain @ 2017-12-16 2:13 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 12/15/2017 11:06 PM, Nikolay Borisov wrote:
>
>
> On 15.12.2017 05:47, Anand Jain wrote:
>> Let the list iterator iterate further and find other stale
>> devices and delete it. This is in preparation to add support
>> for user land request-able stale devices cleanup.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>
> What is the lock protection of this function - uuid_mutex, it's not
> really obvious. Perhaps adding a lockdep_assert_held for the correct
> lock ? I guess David's earlier patch to document the locking in
> volumes.c might shed some light on this one.
I remembered your earlier similar comments and I thought of adding
lockdep here, but as such I am also working on cleaning up uuid_mutext
and device_list_mutex I would like to include such lockdep assert in
those patches.
Thanks, Anand
>> ---
>> fs/btrfs/volumes.c | 20 +++++++++-----------
>> 1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 0e89409112d5..70db6a1d5658 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -610,19 +610,20 @@ static void pending_bios_fn(struct btrfs_work *work)
>>
>> static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
>> {
>> - struct btrfs_fs_devices *fs_devs;
>> - struct btrfs_device *dev;
>> + struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
>> + struct btrfs_device *dev, *tmp_dev;
>>
>> if (!cur_dev->name)
>> return;
>>
>> - list_for_each_entry(fs_devs, &fs_uuids, list) {
>> - int del = 1;
>> + list_for_each_entry_safe(fs_devs, tmp_fs_devs, &fs_uuids, list) {
>>
>> if (fs_devs->opened)
>> continue;
>>
>> - list_for_each_entry(dev, &fs_devs->devices, dev_list) {
>> + list_for_each_entry_safe(dev, tmp_dev,
>> + &fs_devs->devices, dev_list) {
>> + int not_found;
>>
>> if (dev == cur_dev)
>> continue;
>> @@ -636,14 +637,12 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
>> * either use mapper or non mapper path throughout.
>> */
>> rcu_read_lock();
>> - del = strcmp(rcu_str_deref(dev->name),
>> + not_found = strcmp(rcu_str_deref(dev->name),
>> rcu_str_deref(cur_dev->name));
>> rcu_read_unlock();
>> - if (!del)
>> - break;
>> - }
>> + if (not_found)
>> + continue;
>>
>> - if (!del) {
>> /* delete the stale device */
>> if (fs_devs->num_devices == 1) {
>> btrfs_sysfs_remove_fsid(fs_devs);
>> @@ -654,7 +653,6 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
>> list_del(&dev->dev_list);
>> free_device(dev);
>> }
>> - break;
>> }
>> }
>> }
>>
> --
> 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
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales
2017-12-16 2:13 ` Anand Jain
@ 2017-12-16 6:54 ` Nikolay Borisov
0 siblings, 0 replies; 20+ messages in thread
From: Nikolay Borisov @ 2017-12-16 6:54 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 16.12.2017 04:13, Anand Jain wrote:
>
>
> On 12/15/2017 11:06 PM, Nikolay Borisov wrote:
>>
>>
>> On 15.12.2017 05:47, Anand Jain wrote:
>>> Let the list iterator iterate further and find other stale
>>> devices and delete it. This is in preparation to add support
>>> for user land request-able stale devices cleanup.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> What is the lock protection of this function - uuid_mutex, it's not
>> really obvious. Perhaps adding a lockdep_assert_held for the correct
>> lock ? I guess David's earlier patch to document the locking in
>> volumes.c might shed some light on this one.
>
> I remembered your earlier similar comments and I thought of adding
> lockdep here, but as such I am also working on cleaning up uuid_mutext
> and device_list_mutex I would like to include such lockdep assert in
> those patches.
Makes sense so long as this change lands
>
> Thanks, Anand
>>> ---
>>> fs/btrfs/volumes.c | 20 +++++++++-----------
>>> 1 file changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 0e89409112d5..70db6a1d5658 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -610,19 +610,20 @@ static void pending_bios_fn(struct btrfs_work
>>> *work)
>>> static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
>>> {
>>> - struct btrfs_fs_devices *fs_devs;
>>> - struct btrfs_device *dev;
>>> + struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
>>> + struct btrfs_device *dev, *tmp_dev;
>>> if (!cur_dev->name)
>>> return;
>>> - list_for_each_entry(fs_devs, &fs_uuids, list) {
>>> - int del = 1;
>>> + list_for_each_entry_safe(fs_devs, tmp_fs_devs, &fs_uuids, list) {
>>> if (fs_devs->opened)
>>> continue;
>>> - list_for_each_entry(dev, &fs_devs->devices, dev_list) {
>>> + list_for_each_entry_safe(dev, tmp_dev,
>>> + &fs_devs->devices, dev_list) {
>>> + int not_found;
>>> if (dev == cur_dev)
>>> continue;
>>> @@ -636,14 +637,12 @@ static void btrfs_free_stale_device(struct
>>> btrfs_device *cur_dev)
>>> * either use mapper or non mapper path throughout.
>>> */
>>> rcu_read_lock();
>>> - del = strcmp(rcu_str_deref(dev->name),
>>> + not_found = strcmp(rcu_str_deref(dev->name),
>>> rcu_str_deref(cur_dev->name));
>>> rcu_read_unlock();
>>> - if (!del)
>>> - break;
>>> - }
>>> + if (not_found)
>>> + continue;
>>> - if (!del) {
>>> /* delete the stale device */
>>> if (fs_devs->num_devices == 1) {
>>> btrfs_sysfs_remove_fsid(fs_devs);
>>> @@ -654,7 +653,6 @@ static void btrfs_free_stale_device(struct
>>> btrfs_device *cur_dev)
>>> list_del(&dev->dev_list);
>>> free_device(dev);
>>> }
>>> - break;
>>> }
>>> }
>>> }
>>>
>> --
>> 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
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/6] btrfs: make btrfs_free_stale_device() argument optional
2017-12-15 3:47 [PATCH 0/6] preparatory work to add device forget Anand Jain
` (2 preceding siblings ...)
2017-12-15 3:47 ` [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales Anand Jain
@ 2017-12-15 3:47 ` Anand Jain
2017-12-15 15:07 ` Nikolay Borisov
2017-12-15 3:47 ` [PATCH 5/6] btrfs: make btrfs_free_stale_device() to match the path Anand Jain
2017-12-15 3:47 ` [PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable Anand Jain
5 siblings, 1 reply; 20+ messages in thread
From: Anand Jain @ 2017-12-15 3:47 UTC (permalink / raw)
To: linux-btrfs
This updates btrfs_free_stale_device() helper function to delete all
unmouted devices, when arg is NULL.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/volumes.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 70db6a1d5658..9aee4f987221 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -613,9 +613,6 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
struct btrfs_device *dev, *tmp_dev;
- if (!cur_dev->name)
- return;
-
list_for_each_entry_safe(fs_devs, tmp_fs_devs, &fs_uuids, list) {
if (fs_devs->opened)
@@ -625,9 +622,7 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
&fs_devs->devices, dev_list) {
int not_found;
- if (dev == cur_dev)
- continue;
- if (!dev->name)
+ if (cur_dev && (cur_dev == dev || !dev->name))
continue;
/*
@@ -637,8 +632,11 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
* either use mapper or non mapper path throughout.
*/
rcu_read_lock();
- not_found = strcmp(rcu_str_deref(dev->name),
- rcu_str_deref(cur_dev->name));
+ if (cur_dev)
+ not_found = strcmp(rcu_str_deref(dev->name),
+ rcu_str_deref(cur_dev->name));
+ else
+ not_found = 0;
rcu_read_unlock();
if (not_found)
continue;
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/6] btrfs: make btrfs_free_stale_device() argument optional
2017-12-15 3:47 ` [PATCH 4/6] btrfs: make btrfs_free_stale_device() argument optional Anand Jain
@ 2017-12-15 15:07 ` Nikolay Borisov
2017-12-16 2:14 ` Anand Jain
0 siblings, 1 reply; 20+ messages in thread
From: Nikolay Borisov @ 2017-12-15 15:07 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 15.12.2017 05:47, Anand Jain wrote:
> This updates btrfs_free_stale_device() helper function to delete all
> unmouted devices, when arg is NULL.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> fs/btrfs/volumes.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 70db6a1d5658..9aee4f987221 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -613,9 +613,6 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
> struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
> struct btrfs_device *dev, *tmp_dev;
>
> - if (!cur_dev->name)
> - return;
> -
> list_for_each_entry_safe(fs_devs, tmp_fs_devs, &fs_uuids, list) {
>
> if (fs_devs->opened)
> @@ -625,9 +622,7 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
> &fs_devs->devices, dev_list) {
> int not_found;
>
> - if (dev == cur_dev)
> - continue;
> - if (!dev->name)
> + if (cur_dev && (cur_dev == dev || !dev->name))
> continue;
>
> /*
> @@ -637,8 +632,11 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
> * either use mapper or non mapper path throughout.
> */
> rcu_read_lock();
> - not_found = strcmp(rcu_str_deref(dev->name),
> - rcu_str_deref(cur_dev->name));
> + if (cur_dev)
> + not_found = strcmp(rcu_str_deref(dev->name),
> + rcu_str_deref(cur_dev->name));
> + else
> + not_found = 0;
nit: Perhaps put a proper documentation header at the beginning of the
function detailing this behavior. I.e.
/*
* btrfs_free_stale_device
* ....
> rcu_read_unlock();
> if (not_found)
> continue;
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/6] btrfs: make btrfs_free_stale_device() argument optional
2017-12-15 15:07 ` Nikolay Borisov
@ 2017-12-16 2:14 ` Anand Jain
2017-12-16 6:55 ` Nikolay Borisov
0 siblings, 1 reply; 20+ messages in thread
From: Anand Jain @ 2017-12-16 2:14 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
/*
>> @@ -637,8 +632,11 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
>> * either use mapper or non mapper path throughout.
>> */
>> rcu_read_lock();
>> - not_found = strcmp(rcu_str_deref(dev->name),
>> - rcu_str_deref(cur_dev->name));
>> + if (cur_dev)
>> + not_found = strcmp(rcu_str_deref(dev->name),
>> + rcu_str_deref(cur_dev->name));
>> + else
>> + not_found = 0;
>
> nit: Perhaps put a proper documentation header at the beginning of the
> function detailing this behavior. I.e.
>
> /*
> * btrfs_free_stale_device
> * ....
>
Right. Will add.
Thanks, Anand
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/6] btrfs: make btrfs_free_stale_device() argument optional
2017-12-16 2:14 ` Anand Jain
@ 2017-12-16 6:55 ` Nikolay Borisov
0 siblings, 0 replies; 20+ messages in thread
From: Nikolay Borisov @ 2017-12-16 6:55 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 16.12.2017 04:14, Anand Jain wrote:
> /*
>>> @@ -637,8 +632,11 @@ static void btrfs_free_stale_device(struct
>>> btrfs_device *cur_dev)
>>> * either use mapper or non mapper path throughout.
>>> */
>>> rcu_read_lock();
>>> - not_found = strcmp(rcu_str_deref(dev->name),
>>> - rcu_str_deref(cur_dev->name));
>>> + if (cur_dev)
>>> + not_found = strcmp(rcu_str_deref(dev->name),
>>> + rcu_str_deref(cur_dev->name));
>>> + else
>>> + not_found = 0;
>>
>> nit: Perhaps put a proper documentation header at the beginning of the
>> function detailing this behavior. I.e.
>>
>> /*
>> * btrfs_free_stale_device
>> * ....
>>
>
> Right. Will add.
I posted this comment before reading your other patches, having done
that I'd say it's best to add the documentation once all the cleanups
have been done, since you also add another semantics with the "path"
argument.
>
> Thanks, Anand
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/6] btrfs: make btrfs_free_stale_device() to match the path
2017-12-15 3:47 [PATCH 0/6] preparatory work to add device forget Anand Jain
` (3 preceding siblings ...)
2017-12-15 3:47 ` [PATCH 4/6] btrfs: make btrfs_free_stale_device() argument optional Anand Jain
@ 2017-12-15 3:47 ` Anand Jain
2017-12-15 3:47 ` [PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable Anand Jain
5 siblings, 0 replies; 20+ messages in thread
From: Anand Jain @ 2017-12-15 3:47 UTC (permalink / raw)
To: linux-btrfs
The btrfs_free_stale_device() is updated to match for the given
device path and delete it. (It searchs for only unmounted list of
devices.)
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/volumes.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9aee4f987221..0bf3233859b6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -608,7 +608,7 @@ static void pending_bios_fn(struct btrfs_work *work)
}
-static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
+static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
{
struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
struct btrfs_device *dev, *tmp_dev;
@@ -635,6 +635,8 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
if (cur_dev)
not_found = strcmp(rcu_str_deref(dev->name),
rcu_str_deref(cur_dev->name));
+ else if (path)
+ not_found = strcmp(rcu_str_deref(dev->name), path);
else
not_found = 0;
rcu_read_unlock();
@@ -782,7 +784,7 @@ static noinline int device_list_add(const char *path,
ret = 1;
device->fs_devices = fs_devices;
- btrfs_free_stale_device(device);
+ btrfs_free_stale_device(device, NULL);
} else if (!device->name || strcmp(device->name->str, path)) {
/*
* When FS is already mounted.
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable
2017-12-15 3:47 [PATCH 0/6] preparatory work to add device forget Anand Jain
` (4 preceding siblings ...)
2017-12-15 3:47 ` [PATCH 5/6] btrfs: make btrfs_free_stale_device() to match the path Anand Jain
@ 2017-12-15 3:47 ` Anand Jain
2017-12-15 11:29 ` [PATCH v2 1/6] " Anand Jain
` (2 more replies)
5 siblings, 3 replies; 20+ messages in thread
From: Anand Jain @ 2017-12-15 3:47 UTC (permalink / raw)
To: linux-btrfs
Now as the there is path in arg, so instead of reading the path from
cur_device just get it from the caller, and so the purpose of cur_device
is to skip the device, so rename it to skip_dev. Also drop the comment
about different path being used for the same device, since now we will
have cli to clean any device that's not a concern any more.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/volumes.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0bf3233859b6..f9db59c118a4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -607,8 +607,7 @@ static void pending_bios_fn(struct btrfs_work *work)
run_scheduled_bios(device);
}
-
-static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
+static void btrfs_free_stale_device(const char *path, struct btrfs_device *skip_dev)
{
struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
struct btrfs_device *dev, *tmp_dev;
@@ -622,20 +621,11 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
&fs_devs->devices, dev_list) {
int not_found;
- if (cur_dev && (cur_dev == dev || !dev->name))
+ if (skip_dev && skip_dev == dev)
continue;
- /*
- * Todo: This won't be enough. What if the same device
- * comes back (with new uuid and) with its mapper path?
- * But for now, this does help as mostly an admin will
- * either use mapper or non mapper path throughout.
- */
rcu_read_lock();
- if (cur_dev)
- not_found = strcmp(rcu_str_deref(dev->name),
- rcu_str_deref(cur_dev->name));
- else if (path)
+ if (path)
not_found = strcmp(rcu_str_deref(dev->name), path);
else
not_found = 0;
@@ -784,7 +774,7 @@ static noinline int device_list_add(const char *path,
ret = 1;
device->fs_devices = fs_devices;
- btrfs_free_stale_device(device, NULL);
+ btrfs_free_stale_device(path, device);
} else if (!device->name || strcmp(device->name->str, path)) {
/*
* When FS is already mounted.
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v2 1/6] btrfs: cleanup to make btrfs_free_stale_device() readable
2017-12-15 3:47 ` [PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable Anand Jain
@ 2017-12-15 11:29 ` Anand Jain
2017-12-15 13:07 ` [PATCH v2 6/6] " Anand Jain
2017-12-16 2:48 ` [PATCH v3 " Anand Jain
2 siblings, 0 replies; 20+ messages in thread
From: Anand Jain @ 2017-12-15 11:29 UTC (permalink / raw)
To: linux-btrfs
Now as the there is path in arg, so instead of reading the path from
cur_device just get it from the caller, and so the purpose of cur_device
is to skip the device, so rename it to skip_dev. Also drop the comment
about different path being used for the same device, since now we will
have cli to clean any device that's not a concern any more.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2: btrfs_device::name is null when we have missing device and
unmounted. So we still need to check for dev->name.
fs/btrfs/volumes.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0bf3233859b6..9f3a59f26e84 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -607,8 +607,7 @@ static void pending_bios_fn(struct btrfs_work *work)
run_scheduled_bios(device);
}
-
-static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
+static void btrfs_free_stale_device(const char *path, struct btrfs_device *skip_dev)
{
struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
struct btrfs_device *dev, *tmp_dev;
@@ -622,20 +621,13 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
&fs_devs->devices, dev_list) {
int not_found;
- if (cur_dev && (cur_dev == dev || !dev->name))
+ if (skip_dev && skip_dev == dev)
+ continue;
+ if (path && !dev->name)
continue;
- /*
- * Todo: This won't be enough. What if the same device
- * comes back (with new uuid and) with its mapper path?
- * But for now, this does help as mostly an admin will
- * either use mapper or non mapper path throughout.
- */
rcu_read_lock();
- if (cur_dev)
- not_found = strcmp(rcu_str_deref(dev->name),
- rcu_str_deref(cur_dev->name));
- else if (path)
+ if (path)
not_found = strcmp(rcu_str_deref(dev->name), path);
else
not_found = 0;
@@ -784,7 +776,7 @@ static noinline int device_list_add(const char *path,
ret = 1;
device->fs_devices = fs_devices;
- btrfs_free_stale_device(device, NULL);
+ btrfs_free_stale_device(path, device);
} else if (!device->name || strcmp(device->name->str, path)) {
/*
* When FS is already mounted.
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v2 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable
2017-12-15 3:47 ` [PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable Anand Jain
2017-12-15 11:29 ` [PATCH v2 1/6] " Anand Jain
@ 2017-12-15 13:07 ` Anand Jain
2017-12-16 2:48 ` [PATCH v3 " Anand Jain
2 siblings, 0 replies; 20+ messages in thread
From: Anand Jain @ 2017-12-15 13:07 UTC (permalink / raw)
To: linux-btrfs
Now as the there is path in arg, so instead of reading the path from
cur_device just get it from the caller, and so the purpose of cur_device
is to skip the device, so rename it to skip_dev. Also drop the comment
about different path being used for the same device, since now we will
have cli to clean any device that's not a concern any more.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
[fix patch number]
v2: btrfs_device::name is null when we have missing device and
unmounted. So we still need to check for dev->name.
fs/btrfs/volumes.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0bf3233859b6..9f3a59f26e84 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -607,8 +607,7 @@ static void pending_bios_fn(struct btrfs_work *work)
run_scheduled_bios(device);
}
-
-static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
+static void btrfs_free_stale_device(const char *path, struct btrfs_device *skip_dev)
{
struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
struct btrfs_device *dev, *tmp_dev;
@@ -622,20 +621,13 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
&fs_devs->devices, dev_list) {
int not_found;
- if (cur_dev && (cur_dev == dev || !dev->name))
+ if (skip_dev && skip_dev == dev)
+ continue;
+ if (path && !dev->name)
continue;
- /*
- * Todo: This won't be enough. What if the same device
- * comes back (with new uuid and) with its mapper path?
- * But for now, this does help as mostly an admin will
- * either use mapper or non mapper path throughout.
- */
rcu_read_lock();
- if (cur_dev)
- not_found = strcmp(rcu_str_deref(dev->name),
- rcu_str_deref(cur_dev->name));
- else if (path)
+ if (path)
not_found = strcmp(rcu_str_deref(dev->name), path);
else
not_found = 0;
@@ -784,7 +776,7 @@ static noinline int device_list_add(const char *path,
ret = 1;
device->fs_devices = fs_devices;
- btrfs_free_stale_device(device, NULL);
+ btrfs_free_stale_device(path, device);
} else if (!device->name || strcmp(device->name->str, path)) {
/*
* When FS is already mounted.
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable
2017-12-15 3:47 ` [PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable Anand Jain
2017-12-15 11:29 ` [PATCH v2 1/6] " Anand Jain
2017-12-15 13:07 ` [PATCH v2 6/6] " Anand Jain
@ 2017-12-16 2:48 ` Anand Jain
2 siblings, 0 replies; 20+ messages in thread
From: Anand Jain @ 2017-12-16 2:48 UTC (permalink / raw)
To: linux-btrfs
Now as the there is path in arg, so instead of reading the path from
cur_device just get it from the caller, and so the purpose of cur_device
is to skip the device, so rename it to skip_dev. Also drop the comment
about different path being used for the same device, since now we will
have cli to clean any device that's not a concern any more.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: add btrfs_free_stale_device() fn description, as suggested by
Nikolay
Fix line with longer than 80 char
v2: btrfs_device::name is null when we have missing device and
unmounted. So we still need to check for dev->name.
fs/btrfs/volumes.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 0bf3233859b6..6d3353ce50d8 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -607,8 +607,17 @@ static void pending_bios_fn(struct btrfs_work *work)
run_scheduled_bios(device);
}
-
-static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
+/*
+ * btrfs_free_stale_device()
+ * Search and remove all stale (devices which are not mounted) devices.
+ * When both inputs are NULL, it will search and release all stale devices.
+ * path: Optional. When provided will it release all unmounted devices
+ * matching this path only.
+ * skip_dev: Optional. Will skip this device when searching for the stale
+ * devices.
+ */
+static void btrfs_free_stale_device(const char *path,
+ struct btrfs_device *skip_dev)
{
struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
struct btrfs_device *dev, *tmp_dev;
@@ -622,20 +631,13 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
&fs_devs->devices, dev_list) {
int not_found;
- if (cur_dev && (cur_dev == dev || !dev->name))
+ if (skip_dev && skip_dev == dev)
+ continue;
+ if (path && !dev->name)
continue;
- /*
- * Todo: This won't be enough. What if the same device
- * comes back (with new uuid and) with its mapper path?
- * But for now, this does help as mostly an admin will
- * either use mapper or non mapper path throughout.
- */
rcu_read_lock();
- if (cur_dev)
- not_found = strcmp(rcu_str_deref(dev->name),
- rcu_str_deref(cur_dev->name));
- else if (path)
+ if (path)
not_found = strcmp(rcu_str_deref(dev->name), path);
else
not_found = 0;
@@ -784,7 +786,7 @@ static noinline int device_list_add(const char *path,
ret = 1;
device->fs_devices = fs_devices;
- btrfs_free_stale_device(device, NULL);
+ btrfs_free_stale_device(path, device);
} else if (!device->name || strcmp(device->name->str, path)) {
/*
* When FS is already mounted.
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable
2018-01-09 14:13 [PATCH RESEND v3 0/6] preparatory work to add device forget Anand Jain
@ 2018-01-09 14:13 ` Anand Jain
2018-01-09 16:24 ` Josef Bacik
0 siblings, 1 reply; 20+ messages in thread
From: Anand Jain @ 2018-01-09 14:13 UTC (permalink / raw)
To: linux-btrfs
From: Anand Jain <Anand.Jain@oracle.com>
Now as the there is path in arg, so instead of reading the path from
cur_device just get it from the caller, and so the purpose of cur_device
is to skip the device, so rename it to skip_dev. Also drop the comment
about different path being used for the same device, since now we will
have cli to clean any device that's not a concern any more.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: add btrfs_free_stale_device() fn description, as suggested by
Nikolay
Fix line with longer than 80 char
v2: btrfs_device::name is null when we have missing device and
unmounted. So we still need to check for dev->name.
fs/btrfs/volumes.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f87d30aa0e18..848824fe98a3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -605,8 +605,17 @@ static void pending_bios_fn(struct btrfs_work *work)
run_scheduled_bios(device);
}
-
-static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
+/*
+ * btrfs_free_stale_device()
+ * Search and remove all stale (devices which are not mounted) devices.
+ * When both inputs are NULL, it will search and release all stale devices.
+ * path: Optional. When provided will it release all unmounted devices
+ * matching this path only.
+ * skip_dev: Optional. Will skip this device when searching for the stale
+ * devices.
+ */
+static void btrfs_free_stale_device(const char *path,
+ struct btrfs_device *skip_dev)
{
struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
struct btrfs_device *dev, *tmp_dev;
@@ -620,20 +629,13 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev, char *path)
&fs_devs->devices, dev_list) {
int not_found;
- if (cur_dev && (cur_dev == dev || !dev->name))
+ if (skip_dev && skip_dev == dev)
+ continue;
+ if (path && !dev->name)
continue;
- /*
- * Todo: This won't be enough. What if the same device
- * comes back (with new uuid and) with its mapper path?
- * But for now, this does help as mostly an admin will
- * either use mapper or non mapper path throughout.
- */
rcu_read_lock();
- if (cur_dev)
- not_found = strcmp(rcu_str_deref(dev->name),
- rcu_str_deref(cur_dev->name));
- else if (path)
+ if (path)
not_found = strcmp(rcu_str_deref(dev->name), path);
else
not_found = 0;
@@ -778,7 +780,7 @@ static noinline int device_list_add(const char *path,
ret = 1;
device->fs_devices = fs_devices;
- btrfs_free_stale_device(device, NULL);
+ btrfs_free_stale_device(path, device);
} else if (!device->name || strcmp(device->name->str, path)) {
/*
* When FS is already mounted.
--
2.7.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable
2018-01-09 14:13 ` [PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable Anand Jain
@ 2018-01-09 16:24 ` Josef Bacik
0 siblings, 0 replies; 20+ messages in thread
From: Josef Bacik @ 2018-01-09 16:24 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs
On Tue, Jan 09, 2018 at 10:13:14PM +0800, Anand Jain wrote:
> From: Anand Jain <Anand.Jain@oracle.com>
>
> Now as the there is path in arg, so instead of reading the path from
> cur_device just get it from the caller, and so the purpose of cur_device
> is to skip the device, so rename it to skip_dev. Also drop the comment
> about different path being used for the same device, since now we will
> have cli to clean any device that's not a concern any more.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
Wait you added char *path, and then changed it to device here? Just drop the
previous patch and do this one. Thanks,
Josef
^ permalink raw reply [flat|nested] 20+ messages in thread