[PATCH v5 0/6] preparatory work to add device forget

[PATCH v5 0/6] preparatory work to add device forget

[Anand Jain]

v4->v5:
 Fix fn name btrfs_free_stale_device() to btrfs_free_stale_devices()
 in the comments and commit title. No code change.
 Add received reviewed-by.

v3->v4:
 Mainly fix as per comments from Josef.
 @3/6: rename btrfs_free_stale_device() to btrfs_free_stale_devices()
 @4/6: reorg logic, init not_found = 0; drop else part
 @5/6: added new in v4. Renames arg cur_dev to skip_dev
 @6/6: v3:5/6 is merged to v4:6/6
 checkpath error fixes.

v2->v3:
 @ 6/6:
 add btrfs_free_stale_device() fn description, suggested by Nikolay
 Fix line with longer than 80 char
 
v1->v2:
 @ 6/6:
 btrfs_device::name is null when we have missing device and
 unmounted. So we still need to check for dev->name.

We can reuse the function btrfs_free_stale_device() to add feature
to forget a scanned device or all stale devices. So this patch set
proposes following changes to it.


Anand Jain (6):
  btrfs: cleanup btrfs_free_stale_device() usage
  btrfs: no need to check for btrfs_fs_devices::seeding
  btrfs: make btrfs_free_stale_device() to iterate all stales
  btrfs: make btrfs_free_stale_devices() argument optional
  btrfs: rename btrfs_free_stale_devices() arg to skip_dev
  btrfs: make btrfs_free_stale_devices() to match the path

 fs/btrfs/volumes.c | 59 +++++++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

-- 
2.7.0

[PATCH 1/6] btrfs: cleanup btrfs_free_stale_device() usage

[Anand Jain]

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>
Reviewed-by: Josef Bacik <jbacik@fb.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 d393808071d5..25b91776d036 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -782,6 +782,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.
@@ -840,13 +841,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

[PATCH 2/6] btrfs: no need to check for btrfs_fs_devices::seeding

[Anand Jain]

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>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/volumes.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 25b91776d036..3f481da9cae7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -619,8 +619,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

[PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales

[Anand Jain]

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

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. Also rename
btrfs_free_stale_device() to btrfs_free_stale_devices().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/volumes.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3f481da9cae7..cd234a5dc763 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -606,21 +606,22 @@ 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_devices(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;
@@ -634,14 +635,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);
@@ -652,7 +651,6 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
 				list_del(&dev->dev_list);
 				free_device(dev);
 			}
-			break;
 		}
 	}
 }
@@ -780,7 +778,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_devices(device);
 	} else if (!device->name || strcmp(device->name->str, path)) {
 		/*
 		 * When FS is already mounted.
-- 
2.7.0

[PATCH 4/6] btrfs: make btrfs_free_stale_devices() argument optional

[Anand Jain]

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

This updates btrfs_free_stale_devices() helper function to delete all
unmouted devices, when arg is NULL.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/volumes.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index cd234a5dc763..bba98d043402 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -611,9 +611,6 @@ static void btrfs_free_stale_devices(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)
@@ -621,11 +618,9 @@ static void btrfs_free_stale_devices(struct btrfs_device *cur_dev)
 
 		list_for_each_entry_safe(dev, tmp_dev,
 					 &fs_devs->devices, dev_list) {
-			int not_found;
+			int not_found = 0;
 
-			if (dev == cur_dev)
-				continue;
-			if (!dev->name)
+			if (cur_dev && (cur_dev == dev || !dev->name))
 				continue;
 
 			/*
@@ -635,8 +630,9 @@ static void btrfs_free_stale_devices(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));
 			rcu_read_unlock();
 			if (not_found)
 				continue;
-- 
2.7.0

[PATCH 5/6] btrfs: rename btrfs_free_stale_devices() arg to skip_dev

[Anand Jain]

No functional changes.
Rename btrfs_free_stale_devices() arg to skip_dev, so that it
reflects what that arg for.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/volumes.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bba98d043402..a3edd4d92c57 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -606,7 +606,7 @@ static void pending_bios_fn(struct btrfs_work *work)
 }
 
 
-static void btrfs_free_stale_devices(struct btrfs_device *cur_dev)
+static void btrfs_free_stale_devices(struct btrfs_device *skip_dev)
 {
 	struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
 	struct btrfs_device *dev, *tmp_dev;
@@ -620,7 +620,7 @@ static void btrfs_free_stale_devices(struct btrfs_device *cur_dev)
 					 &fs_devs->devices, dev_list) {
 			int not_found = 0;
 
-			if (cur_dev && (cur_dev == dev || !dev->name))
+			if (skip_dev && (skip_dev == dev || !dev->name))
 				continue;
 
 			/*
@@ -630,9 +630,9 @@ static void btrfs_free_stale_devices(struct btrfs_device *cur_dev)
 			 * either use mapper or non mapper path throughout.
 			 */
 			rcu_read_lock();
-			if (cur_dev)
+			if (skip_dev)
 				not_found = strcmp(rcu_str_deref(dev->name),
-						   rcu_str_deref(cur_dev->name));
+						   rcu_str_deref(skip_dev->name));
 			rcu_read_unlock();
 			if (not_found)
 				continue;
-- 
2.7.0

[PATCH 6/6] btrfs: make btrfs_free_stale_devices() to match the path

[Anand Jain]

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

The btrfs_free_stale_devices() is updated to match for the given
device path and delete it. (It searches for only unmounted list of
devices.) 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>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---
 fs/btrfs/volumes.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a3edd4d92c57..68674da7f5fc 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_devices(struct btrfs_device *skip_dev)
+/*
+ * btrfs_free_stale_devices()
+ *  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_devices(const char *path,
+				     struct btrfs_device *skip_dev)
 {
 	struct btrfs_fs_devices *fs_devs, *tmp_fs_devs;
 	struct btrfs_device *dev, *tmp_dev;
@@ -620,19 +629,15 @@ static void btrfs_free_stale_devices(struct btrfs_device *skip_dev)
 					 &fs_devs->devices, dev_list) {
 			int not_found = 0;
 
-			if (skip_dev && (skip_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 (skip_dev)
+			if (path)
 				not_found = strcmp(rcu_str_deref(dev->name),
-						   rcu_str_deref(skip_dev->name));
+						   path);
 			rcu_read_unlock();
 			if (not_found)
 				continue;
@@ -774,7 +779,7 @@ static noinline int device_list_add(const char *path,
 
 		ret = 1;
 		device->fs_devices = fs_devices;
-		btrfs_free_stale_devices(device);
+		btrfs_free_stale_devices(path, device);
 	} else if (!device->name || strcmp(device->name->str, path)) {
 		/*
 		 * When FS is already mounted.
-- 
2.7.0

Re: [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales

[Josef Bacik]

On Wed, Jan 10, 2018 at 01:15:20PM +0800, Anand Jain wrote:
> From: Anand Jain <Anand.Jain@oracle.com>
> 
> 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. Also rename
> btrfs_free_stale_device() to btrfs_free_stale_devices().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: Josef Bacik <jbacik@fb.com>

Thanks,

Josef

[PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales

[Anand Jain]

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

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. Also rename
btrfs_free_stale_device() to btrfs_free_stale_devices().

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3f481da9cae7..cd234a5dc763 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -606,21 +606,22 @@ 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_devices(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;
@@ -634,14 +635,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);
@@ -652,7 +651,6 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
 				list_del(&dev->dev_list);
 				free_device(dev);
 			}
-			break;
 		}
 	}
 }
@@ -780,7 +778,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_devices(device);
 	} else if (!device->name || strcmp(device->name->str, path)) {
 		/*
 		 * When FS is already mounted.
-- 
2.7.0

Re: [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales

[Josef Bacik]

On Tue, Jan 09, 2018 at 10:13:11PM +0800, Anand Jain wrote:
> From: Anand Jain <Anand.Jain@oracle.com>
> 
> 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>

If we're going to do this then please change the function name to
btrfs_free_stale_devices() so it's clear what its intentions are.  Thanks,

Josef

[PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales

[Anand Jain]

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

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 ab6ccad08ef7..7b253da6c0a4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -608,19 +608,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;
@@ -634,14 +635,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);
@@ -652,7 +651,6 @@ static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
 				list_del(&dev->dev_list);
 				free_device(dev);
 			}
-			break;
 		}
 	}
 }
-- 
2.7.0

Re: [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales

[Nikolay Borisov]

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
>>
> 

Re: [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales

[Anand Jain]

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
> 

Re: [PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales

[Nikolay Borisov]

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;
>  		}
>  	}
>  }
> 

[PATCH 3/6] btrfs: make btrfs_free_stale_device() to iterate all stales

[Anand Jain]

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