[PATCH] Btrfs-progs: do not add stale device into fs_devices

[PATCH] Btrfs-progs: do not add stale device into fs_devices

[Liu Bo]

If one of btrfs's devices was pulled out and we've replaced it with a
new one, then they have the same uuid.

If that device gets reconnected, 'btrfs filesystem show' will show the
stale one instead of the new one, but on kernel side btrfs has a fix
to not include the stale one, this could confuse users as people may
monitor btrfs by running that cli.

This does the similar thing to what kernel side has done.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 volumes.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/volumes.c b/volumes.c
index 2f3943d..c7b7a41 100644
--- a/volumes.c
+++ b/volumes.c
@@ -138,7 +138,20 @@ static int device_list_add(const char *path,
 		list_add(&device->dev_list, &fs_devices->devices);
 		device->fs_devices = fs_devices;
 	} else if (!device->name || strcmp(device->name, path)) {
-		char *name = strdup(path);
+		char *name;
+
+		/*
+		 * The existing device has newer generation, so this
+		 * one could be a stale one, don't add it.
+		 */
+		if (found_transid < device->generation) {
+			warning("adding device %s gen %llu but found a existing device %s gen %llu\n",
+				path, found_transid, device->name,
+				device->generation, found_transid);
+			return -EEXIST;
+		}
+
+		name = strdup(path);
                 if (!name)
                         return -ENOMEM;
                 kfree(device->name);
-- 
2.9.4

Re: [PATCH] Btrfs-progs: do not add stale device into fs_devices

[Anand Jain]

On 10/11/2017 08:28 AM, Liu Bo wrote:
> If one of btrfs's devices was pulled out and we've replaced it with a
> new one, then they have the same uuid.
> 
> If that device gets reconnected, 'btrfs filesystem show' will show the
> stale one instead of the new one, but on kernel side btrfs has a fix
> to not include the stale one, this could confuse users as people may
> monitor btrfs by running that cli.
> 
> This does the similar thing to what kernel side has done.

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

Thanks, Anand


> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>   volumes.c | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/volumes.c b/volumes.c
> index 2f3943d..c7b7a41 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -138,7 +138,20 @@ static int device_list_add(const char *path,
>   		list_add(&device->dev_list, &fs_devices->devices);
>   		device->fs_devices = fs_devices;
>   	} else if (!device->name || strcmp(device->name, path)) {
> -		char *name = strdup(path);
> +		char *name;
> +
> +		/*
> +		 * The existing device has newer generation, so this
> +		 * one could be a stale one, don't add it.
> +		 */
> +		if (found_transid < device->generation) {
> +			warning("adding device %s gen %llu but found a existing device %s gen %llu\n",
> +				path, found_transid, device->name,
> +				device->generation, found_transid);
> +			return -EEXIST;
> +		}
> +
> +		name = strdup(path);
>                   if (!name)
>                           return -ENOMEM;
>                   kfree(device->name);
> 

Re: [PATCH] Btrfs-progs: do not add stale device into fs_devices

[Nikolay Borisov]

On 11.10.2017 03:28, Liu Bo wrote:
> If one of btrfs's devices was pulled out and we've replaced it with a
> new one, then they have the same uuid.
> 
> If that device gets reconnected, 'btrfs filesystem show' will show the
> stale one instead of the new one, but on kernel side btrfs has a fix
> to not include the stale one, this could confuse users as people may
> monitor btrfs by running that cli.
> 
> This does the similar thing to what kernel side has done.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  volumes.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/volumes.c b/volumes.c
> index 2f3943d..c7b7a41 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -138,7 +138,20 @@ static int device_list_add(const char *path,
>  		list_add(&device->dev_list, &fs_devices->devices);
>  		device->fs_devices = fs_devices;
>  	} else if (!device->name || strcmp(device->name, path)) {
> -		char *name = strdup(path);
> +		char *name;
> +
> +		/*
> +		 * The existing device has newer generation, so this
> +		 * one could be a stale one, don't add it.
> +		 */
> +		if (found_transid < device->generation) {
> +			warning("adding device %s gen %llu but found a existing device %s gen %llu\n",
> +				path, found_transid, device->name,
> +				device->generation, found_transid);

You pass in 5 parameters but have only 4 formatting strings. I don't see
the same happening on other warning() invocations? Perhaps the last
found_transid is not necessary?

> +			return -EEXIST;
> +		}
> +
> +		name = strdup(path);
>                  if (!name)
>                          return -ENOMEM;
>                  kfree(device->name);
> 

Re: [PATCH] Btrfs-progs: do not add stale device into fs_devices

[Anand Jain]

On 10/11/2017 05:33 PM, Nikolay Borisov wrote:
> 
> 
> On 11.10.2017 03:28, Liu Bo wrote:
>> If one of btrfs's devices was pulled out and we've replaced it with a
>> new one, then they have the same uuid.
>>
>> If that device gets reconnected, 'btrfs filesystem show' will show the
>> stale one instead of the new one, but on kernel side btrfs has a fix
>> to not include the stale one, this could confuse users as people may
>> monitor btrfs by running that cli.
>>
>> This does the similar thing to what kernel side has done.
>>
>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>> ---
>>   volumes.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/volumes.c b/volumes.c
>> index 2f3943d..c7b7a41 100644
>> --- a/volumes.c
>> +++ b/volumes.c
>> @@ -138,7 +138,20 @@ static int device_list_add(const char *path,
>>   		list_add(&device->dev_list, &fs_devices->devices);
>>   		device->fs_devices = fs_devices;
>>   	} else if (!device->name || strcmp(device->name, path)) {
>> -		char *name = strdup(path);
>> +		char *name;
>> +
>> +		/*
>> +		 * The existing device has newer generation, so this
>> +		 * one could be a stale one, don't add it.
>> +		 */
>> +		if (found_transid < device->generation) {
>> +			warning("adding device %s gen %llu but found a existing device %s gen %llu\n",
>> +				path, found_transid, device->name,
>> +				device->generation, found_transid);
> 
> You pass in 5 parameters but have only 4 formatting strings. I don't see
> the same happening on other warning() invocations? Perhaps the last
> found_transid is not necessary?

  I missed that. Sorry.

Thanks, Anand


>> +			return -EEXIST;
>> +		}
>> +
>> +		name = strdup(path);
>>                   if (!name)
>>                           return -ENOMEM;
>>                   kfree(device->name);
>>
> --
> 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] Btrfs-progs: do not add stale device into fs_devices

[Liu Bo]

On Wed, Oct 11, 2017 at 12:33:15PM +0300, Nikolay Borisov wrote:
> 
> 
> On 11.10.2017 03:28, Liu Bo wrote:
> > If one of btrfs's devices was pulled out and we've replaced it with a
> > new one, then they have the same uuid.
> > 
> > If that device gets reconnected, 'btrfs filesystem show' will show the
> > stale one instead of the new one, but on kernel side btrfs has a fix
> > to not include the stale one, this could confuse users as people may
> > monitor btrfs by running that cli.
> > 
> > This does the similar thing to what kernel side has done.
> > 
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  volumes.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/volumes.c b/volumes.c
> > index 2f3943d..c7b7a41 100644
> > --- a/volumes.c
> > +++ b/volumes.c
> > @@ -138,7 +138,20 @@ static int device_list_add(const char *path,
> >  		list_add(&device->dev_list, &fs_devices->devices);
> >  		device->fs_devices = fs_devices;
> >  	} else if (!device->name || strcmp(device->name, path)) {
> > -		char *name = strdup(path);
> > +		char *name;
> > +
> > +		/*
> > +		 * The existing device has newer generation, so this
> > +		 * one could be a stale one, don't add it.
> > +		 */
> > +		if (found_transid < device->generation) {
> > +			warning("adding device %s gen %llu but found a existing device %s gen %llu\n",
> > +				path, found_transid, device->name,
> > +				device->generation, found_transid);
> 
> You pass in 5 parameters but have only 4 formatting strings. I don't see
> the same happening on other warning() invocations? Perhaps the last
> found_transid is not necessary?
> 

Oh, thanks for the comments.

I messed it up again, what I was testing is warning("blabla",
device->name, device->generation, path, found_transid), but later I
updated the message but got the parameters wrong.

Will do a v2.

Thanks,

-liubo
> > +			return -EEXIST;
> > +		}
> > +
> > +		name = strdup(path);
> >                  if (!name)
> >                          return -ENOMEM;
> >                  kfree(device->name);
> > 

[PATCH v2] Btrfs-progs: do not add stale device into fs_devices

[Liu Bo]

If one of btrfs's devices was pulled out and we've replaced it with a
new one, then they have the same uuid.

If that device gets reconnected, 'btrfs filesystem show' will show the
stale one instead of the new one, but on kernel side btrfs has a fix
to not include the stale one, this could confuse users as people may
monitor btrfs by running that cli.

This does the similar thing to what kernel side has done.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
Reviewed-by: Anand Jain <anand.jain@oracle.com>
---
v2: remove a wrong parameter.

 volumes.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/volumes.c b/volumes.c
index 2f3943d..036c58d 100644
--- a/volumes.c
+++ b/volumes.c
@@ -138,7 +138,20 @@ static int device_list_add(const char *path,
 		list_add(&device->dev_list, &fs_devices->devices);
 		device->fs_devices = fs_devices;
 	} else if (!device->name || strcmp(device->name, path)) {
-		char *name = strdup(path);
+		char *name;
+
+		/*
+		 * The existing device has newer generation, so this
+		 * one could be a stale one, don't add it.
+		 */
+		if (found_transid < device->generation) {
+			warning("adding device %s gen %llu but found an existing device %s gen %llu\n",
+				path, found_transid, device->name,
+				device->generation);
+			return -EEXIST;
+		}
+
+		name = strdup(path);
                 if (!name)
                         return -ENOMEM;
                 kfree(device->name);
-- 
2.9.4

Re: [PATCH v2] Btrfs-progs: do not add stale device into fs_devices

[David Sterba]

On Wed, Oct 11, 2017 at 11:57:16AM -0600, Liu Bo wrote:
> If one of btrfs's devices was pulled out and we've replaced it with a
> new one, then they have the same uuid.
> 
> If that device gets reconnected, 'btrfs filesystem show' will show the
> stale one instead of the new one, but on kernel side btrfs has a fix
> to not include the stale one, this could confuse users as people may
> monitor btrfs by running that cli.
> 
> This does the similar thing to what kernel side has done.
> 
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> Reviewed-by: Anand Jain <anand.jain@oracle.com>

Applied with the following diff, thanks.

--- a/volumes.c
+++ b/volumes.c
@@ -141,11 +141,12 @@ static int device_list_add(const char *path,
                char *name;

                /*
-                * The existing device has newer generation, so this
-                * one could be a stale one, don't add it.
+                * The existing device has newer generation, so this one could
+                * be a stale one, don't add it.
                 */
                if (found_transid < device->generation) {
-                       warning("adding device %s gen %llu but found an existing device %s gen %llu\n",
+                       warning(
+       "adding device %s gen %llu but found an existing device %s gen %llu",
                                path, found_transid, device->name,
                                device->generation);
                        return -EEXIST;