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

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

[Anand Jain]

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 delete all stales
  btrfs: make btrfs_free_stale_device() argument optional
  btrfs: make btrfs_free_stale_device() to search given path
  btrfs: cleanup to make btrfs_free_stale_device() readable

 fs/btrfs/volumes.c | 50 +++++++++++++++-----------------------------------
 1 file changed, 15 insertions(+), 35 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>
---
 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

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

[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

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

[Anand Jain]

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

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

[Anand Jain]

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

[PATCH 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable

[Anand Jain]

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

[PATCH v2 1/6] btrfs: cleanup to make btrfs_free_stale_device() readable

[Anand Jain]

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

[PATCH v2 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable

[Anand Jain]

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

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

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

[Nikolay Borisov]

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

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

[Nikolay Borisov]

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

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 4/6] btrfs: make btrfs_free_stale_device() argument optional

[Anand Jain]

		/*
>> @@ -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

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

[Anand Jain]

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

[PATCH v3 6/6] btrfs: cleanup to make btrfs_free_stale_device() readable

[Anand Jain]

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

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 4/6] btrfs: make btrfs_free_stale_device() argument optional

[Nikolay Borisov]

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