[PATCH 1/2 v3 RESEND] Btrfs: device_list_add() should not update list when mounted

[PATCH 1/2 v3 RESEND] Btrfs: device_list_add() should not update list when mounted

[Anand Jain]

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

device_list_add() is called when user runs btrfs dev scan, which would add
any btrfs device into the btrfs_fs_devices list.

Now think of a mounted btrfs. And a new device which contains the a SB
from the mounted btrfs devices.

In this situation when user runs btrfs dev scan, the current code would
just replace existing device with the new device.

Which is to note that old device is neither closed nor gracefully
removed from the btrfs.

The FS is still operational with the old bdev however the device name
is the btrfs_device is new which is provided by the btrfs dev scan.

reproducer:

devmgt[1] detach /dev/sdc

replace the missing disk /dev/sdc

btrfs rep start -f 1 /dev/sde /btrfs
Label: none  uuid: 5dc0aaf4-4683-4050-b2d6-5ebe5f5cd120
        Total devices 2 FS bytes used 32.00KiB
        devid    1 size 958.94MiB used 115.88MiB path /dev/sde
        devid    2 size 958.94MiB used 103.88MiB path /dev/sdd

make /dev/sdc to reappear

devmgt attach host2

btrfs dev scan

btrfs fi show -m
Label: none  uuid: 5dc0aaf4-4683-4050-b2d6-5ebe5f5cd120^M
        Total devices 2 FS bytes used 32.00KiB^M
        devid    1 size 958.94MiB used 115.88MiB path /dev/sdc <- Wrong.
        devid    2 size 958.94MiB used 103.88MiB path /dev/sdd

since /dev/sdc has been replaced with /dev/sde, the /dev/sdc shouldn't be
part of the btrfs-fsid when it reappears. If user want it to be part of it
then sys admin should be using btrfs device add instead.

[1] github.com/anajain/devmgt.git

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2->v3: simpler commit and comment message
v1->v2: remove '---' in commit messages which conflict with git am

 fs/btrfs/volumes.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bb2cd66..56822f0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -472,6 +472,7 @@ static noinline int device_list_add(const char *path,
 		device = __find_device(&fs_devices->devices, devid,
 				       disk_super->dev_item.uuid);
 	}
+
 	if (!device) {
 		if (fs_devices->opened)
 			return -EBUSY;
@@ -497,6 +498,32 @@ static noinline int device_list_add(const char *path,
 
 		device->fs_devices = fs_devices;
 	} else if (!device->name || strcmp(device->name->str, path)) {
+		/*
+		 * When FS is already mounted.
+		 * 1. If you are here and if the device->name is NULL that means
+		 *    this device was missing at time of FS mount.
+		 * 2. If you are here and if the device->name is different from 'path'
+		 *    that means either
+		 *      a. The same device disappeared and reappeared with different
+		 *         name. or
+		 *      b. The missing-disk-which-was-replaced, has reappeared now.
+		 *
+		 *  We must allow 1 and 2a above. But 2b would be a spurious and
+		 *  unintentional.
+		 *  Further in case of 1 and 2a above, the disk at 'path' would have
+		 *  missed some transaction when it was away and in case of 2a
+		 *  the stale bdev has to be updated as well.
+		 *  2b must not be allowed at all time.
+		 */
+
+		/*
+		 * As of now don't allow update to btrfs_fs_device through the
+		 * btrfs dev scan cli, after FS has been mounted.
+		 */
+
+		if (fs_devices->opened)
+			return -EBUSY;
+
 		name = rcu_string_strdup(path, GFP_NOFS);
 		if (!name)
 			return -ENOMEM;
-- 
1.8.5.3

[PATCH 2/2] btrfs: check generation as replace duplicates devid+uuid

[Anand Jain]

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

When FS in unmounted we need to check generation number as well
since devid+uuid combination could match with the missing replaced
disk when it reappears, and without this patch it might pair with
the replaced disk again.

 device_list_add() function is called in the following threads,
	mount device option
	mount argument
	ioctl BTRFS_IOC_SCAN_DEV (btrfs dev scan)
	ioctl BTRFS_IOC_DEVICES_READY (btrfs dev ready <dev>)
 they have been unit tested to work fine with this patch.

 If the user knows what he is doing and really want to pair with
 replaced disk (which is not a standard operation), then he should
 first clear the kernel btrfs device list in the memory by doing
 the module unload/load and followed with the mount -o device option.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 56822f0..bb1b4bd 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -523,6 +523,16 @@ static noinline int device_list_add(const char *path,
 
 		if (fs_devices->opened)
 			return -EBUSY;
+		else {
+			/*
+			 * That is if the FS is _not_ mounted and if you are here, that
+			 * means there is more than one disk with same uuid and devid.
+			 * We keep the one with larger generation number or the last-in
+			 * if generation are equal.
+			 */
+			if (found_transid < device->generation)
+				return -EINVAL;
+		}
 
 		name = rcu_string_strdup(path, GFP_NOFS);
 		if (!name)
@@ -535,6 +545,15 @@ static noinline int device_list_add(const char *path,
 		}
 	}
 
+	/*
+	 * Unmount does not free the btrfs_device struct but would zero
+	 * generation along with most of the other members. So just update
+	 * it back. We need it to pick the disk with largest generation
+	 * (as above).
+	 */
+	if (!fs_devices->opened)
+		device->generation = found_transid;
+
 	if (found_transid > fs_devices->latest_trans) {
 		fs_devices->latest_devid = devid;
 		fs_devices->latest_trans = found_transid;
-- 
1.8.5.3

Re: [PATCH 1/2 v3 RESEND] Btrfs: device_list_add() should not update list when mounted

[Wang Shilong]

Hi Anand,

Sorry for delay reply, more comments below:

On 06/13/2014 12:26 PM, Anand Jain wrote:
> From: Anand Jain <anand.jain@oracle.com>
>
> device_list_add() is called when user runs btrfs dev scan, which would add
> any btrfs device into the btrfs_fs_devices list.
>
> Now think of a mounted btrfs. And a new device which contains the a SB
> from the mounted btrfs devices.
>
> In this situation when user runs btrfs dev scan, the current code would
> just replace existing device with the new device.
>
> Which is to note that old device is neither closed nor gracefully
> removed from the btrfs.
>
> The FS is still operational with the old bdev however the device name
> is the btrfs_device is new which is provided by the btrfs dev scan.
>
> reproducer:
>
> devmgt[1] detach /dev/sdc
>
> replace the missing disk /dev/sdc
>
> btrfs rep start -f 1 /dev/sde /btrfs
> Label: none  uuid: 5dc0aaf4-4683-4050-b2d6-5ebe5f5cd120
>          Total devices 2 FS bytes used 32.00KiB
>          devid    1 size 958.94MiB used 115.88MiB path /dev/sde
>          devid    2 size 958.94MiB used 103.88MiB path /dev/sdd
>
> make /dev/sdc to reappear
>
> devmgt attach host2
>
> btrfs dev scan
>
> btrfs fi show -m
> Label: none  uuid: 5dc0aaf4-4683-4050-b2d6-5ebe5f5cd120^M
>          Total devices 2 FS bytes used 32.00KiB^M
>          devid    1 size 958.94MiB used 115.88MiB path /dev/sdc <- Wrong.
>          devid    2 size 958.94MiB used 103.88MiB path /dev/sdd
>
> since /dev/sdc has been replaced with /dev/sde, the /dev/sdc shouldn't be
> part of the btrfs-fsid when it reappears. If user want it to be part of it
> then sys admin should be using btrfs device add instead.
>
> [1] github.com/anajain/devmgt.git
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2->v3: simpler commit and comment message
> v1->v2: remove '---' in commit messages which conflict with git am
>
>   fs/btrfs/volumes.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bb2cd66..56822f0 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -472,6 +472,7 @@ static noinline int device_list_add(const char *path,
>   		device = __find_device(&fs_devices->devices, devid,
>   				       disk_super->dev_item.uuid);
>   	}
> +
You add an extra line here.
>   	if (!device) {
>   		if (fs_devices->opened)
>   			return -EBUSY;
> @@ -497,6 +498,32 @@ static noinline int device_list_add(const char *path,
>   
>   		device->fs_devices = fs_devices;
>   	} else if (!device->name || strcmp(device->name->str, path)) {
> +		/*
> +		 * When FS is already mounted.
> +		 * 1. If you are here and if the device->name is NULL that means
> +		 *    this device was missing at time of FS mount.
> +		 * 2. If you are here and if the device->name is different from 'path'
> +		 *    that means either
> +		 *      a. The same device disappeared and reappeared with different
> +		 *         name. or
> +		 *      b. The missing-disk-which-was-replaced, has reappeared now.
> +		 *
> +		 *  We must allow 1 and 2a above. But 2b would be a spurious and
> +		 *  unintentional.
> +		 *  Further in case of 1 and 2a above, the disk at 'path' would have
> +		 *  missed some transaction when it was away and in case of 2a
> +		 *  the stale bdev has to be updated as well.
> +		 *  2b must not be allowed at all time.
> +		 */
> +
> +		/*
> +		 * As of now don't allow update to btrfs_fs_device through the
> +		 * btrfs dev scan cli, after FS has been mounted.
> +		 */
> +
> +		if (fs_devices->opened)
> +			return -EBUSY;
> +
I agree we don't allow to update device list when mounted, i tested this 
patch, it worked but
it will output the following message:

Scanning for Btrfs filesystems in '/dev/sdc'
ERROR: unable to scan the device '/dev/sdc' - Device or resource busy

I think this is a little confusing for common users. Maybe don't return 
error but output
some log message into kernel buffer, better?

Thanks,
Wang
>   		name = rcu_string_strdup(path, GFP_NOFS);
>   		if (!name)
>   			return -ENOMEM;

Re: [PATCH 2/2] btrfs: check generation as replace duplicates devid+uuid

[Wang Shilong]

Hi Anand,

On 06/13/2014 12:26 PM, Anand Jain wrote:
>   
<..snip..>
> @@ -523,6 +523,16 @@ static noinline int device_list_add(const char *path,
>   
>   		if (fs_devices->opened)
>   			return -EBUSY;
> +		else {
> +			/*
> +			 * That is if the FS is _not_ mounted and if you are here, that
> +			 * means there is more than one disk with same uuid and devid.
> +			 * We keep the one with larger generation number or the last-in
> +			 * if generation are equal.
> +			 */
> +			if (found_transid < device->generation)
> +				return -EINVAL;
> +		}
I tried this patch it outputed the following message if it encounter two 
device with the same uuid and device id:

Scanning for Btrfs filesystems
ERROR: device scan failed '/dev/sdc' - Invalid argument

Same comment as your first patch here.
>   
>   		name = rcu_string_strdup(path, GFP_NOFS);
>   		if (!name)
> @@ -535,6 +545,15 @@ static noinline int device_list_add(const char *path,
>   		}
>   	}
>   
> +	/*
> +	 * Unmount does not free the btrfs_device struct but would zero
> +	 * generation along with most of the other members. So just update
> +	 * it back. We need it to pick the disk with largest generation
> +	 * (as above).
> +	 */
> +	if (!fs_devices->opened)
> +		device->generation = found_transid;
> +
>   	if (found_transid > fs_devices->latest_trans) {
>   		fs_devices->latest_devid = devid;
>   		fs_devices->latest_trans = found_transid;

Re: [PATCH 1/2 v3 RESEND] Btrfs: device_list_add() should not update list when mounted

[Anand Jain]

Thanks for the commenting Wang.
inline below.


On 01/07/2014 15:16, Wang Shilong wrote:
> Hi Anand,
>
> Sorry for delay reply, more comments below:
>
> On 06/13/2014 12:26 PM, Anand Jain wrote:
>> From: Anand Jain <anand.jain@oracle.com>
>>
>> device_list_add() is called when user runs btrfs dev scan, which would
>> add
>> any btrfs device into the btrfs_fs_devices list.
>>
>> Now think of a mounted btrfs. And a new device which contains the a SB
>> from the mounted btrfs devices.
>>
>> In this situation when user runs btrfs dev scan, the current code would
>> just replace existing device with the new device.
>>
>> Which is to note that old device is neither closed nor gracefully
>> removed from the btrfs.
>>
>> The FS is still operational with the old bdev however the device name
>> is the btrfs_device is new which is provided by the btrfs dev scan.
>>
>> reproducer:
>>
>> devmgt[1] detach /dev/sdc
>>
>> replace the missing disk /dev/sdc
>>
>> btrfs rep start -f 1 /dev/sde /btrfs
>> Label: none  uuid: 5dc0aaf4-4683-4050-b2d6-5ebe5f5cd120
>>          Total devices 2 FS bytes used 32.00KiB
>>          devid    1 size 958.94MiB used 115.88MiB path /dev/sde
>>          devid    2 size 958.94MiB used 103.88MiB path /dev/sdd
>>
>> make /dev/sdc to reappear
>>
>> devmgt attach host2
>>
>> btrfs dev scan
>>
>> btrfs fi show -m
>> Label: none  uuid: 5dc0aaf4-4683-4050-b2d6-5ebe5f5cd120^M
>>          Total devices 2 FS bytes used 32.00KiB^M
>>          devid    1 size 958.94MiB used 115.88MiB path /dev/sdc <- Wrong.
>>          devid    2 size 958.94MiB used 103.88MiB path /dev/sdd
>>
>> since /dev/sdc has been replaced with /dev/sde, the /dev/sdc shouldn't be
>> part of the btrfs-fsid when it reappears. If user want it to be part
>> of it
>> then sys admin should be using btrfs device add instead.
>>
>> [1] github.com/anajain/devmgt.git
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2->v3: simpler commit and comment message
>> v1->v2: remove '---' in commit messages which conflict with git am
>>
>>   fs/btrfs/volumes.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index bb2cd66..56822f0 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -472,6 +472,7 @@ static noinline int device_list_add(const char *path,
>>           device = __find_device(&fs_devices->devices, devid,
>>                          disk_super->dev_item.uuid);
>>       }
>> +
> You add an extra line here.
>>       if (!device) {
>>           if (fs_devices->opened)
>>               return -EBUSY;
>> @@ -497,6 +498,32 @@ static noinline int device_list_add(const char
>> *path,
>>           device->fs_devices = fs_devices;
>>       } else if (!device->name || strcmp(device->name->str, path)) {
>> +        /*
>> +         * When FS is already mounted.
>> +         * 1. If you are here and if the device->name is NULL that means
>> +         *    this device was missing at time of FS mount.
>> +         * 2. If you are here and if the device->name is different
>> from 'path'
>> +         *    that means either
>> +         *      a. The same device disappeared and reappeared with
>> different
>> +         *         name. or
>> +         *      b. The missing-disk-which-was-replaced, has
>> reappeared now.
>> +         *
>> +         *  We must allow 1 and 2a above. But 2b would be a spurious and
>> +         *  unintentional.
>> +         *  Further in case of 1 and 2a above, the disk at 'path'
>> would have
>> +         *  missed some transaction when it was away and in case of 2a
>> +         *  the stale bdev has to be updated as well.
>> +         *  2b must not be allowed at all time.
>> +         */
>> +
>> +        /*
>> +         * As of now don't allow update to btrfs_fs_device through the
>> +         * btrfs dev scan cli, after FS has been mounted.
>> +         */
>> +
>> +        if (fs_devices->opened)
>> +            return -EBUSY;
>> +
> I agree we don't allow to update device list when mounted, i tested this
> patch, it worked but

Thanks for testing this patch set.

> it will output the following message:
>
> Scanning for Btrfs filesystems in '/dev/sdc'
> ERROR: unable to scan the device '/dev/sdc' - Device or resource busy
>
> I think this is a little confusing for common users. Maybe don't return
> error but output
> some log message into kernel buffer, better?


  the Other choices are:

  display the error only when specific device is used
  by the user like 'btrfs dev scan /dev/sdc' And don't print busy /
  invalid error when a system wide scan is used like 'btrfs dev scan'.
  To achieve this we have to tweak btrfs-progs.

  or put the error under the verbose option in the btrfs-progs.




> Thanks,
> Wang
>>           name = rcu_string_strdup(path, GFP_NOFS);
>>           if (!name)
>>               return -ENOMEM;
>
> --
> 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 1/2 v3 RESEND] Btrfs: device_list_add() should not update list when mounted

[Wang Shilong]

On 07/02/2014 12:31 AM, Anand Jain wrote:
>
>
> Thanks for the commenting Wang.
> inline below.
>
>
> On 01/07/2014 15:16, Wang Shilong wrote:
>> Hi Anand,
>>
>> Sorry for delay reply, more comments below:
>>
>> On 06/13/2014 12:26 PM, Anand Jain wrote:
>>> From: Anand Jain <anand.jain@oracle.com>
>>>
[...]
>> I agree we don't allow to update device list when mounted, i tested this
>> patch, it worked but
>
> Thanks for testing this patch set.
>
>> it will output the following message:
>>
>> Scanning for Btrfs filesystems in '/dev/sdc'
>> ERROR: unable to scan the device '/dev/sdc' - Device or resource busy
>>
>> I think this is a little confusing for common users. Maybe don't return
>> error but output
>> some log message into kernel buffer, better?
>
>
>  the Other choices are:
>
>  display the error only when specific device is used
>  by the user like 'btrfs dev scan /dev/sdc' And don't print busy /
>  invalid error when a system wide scan is used like 'btrfs dev scan'.
>  To achieve this we have to tweak btrfs-progs.

Maybe return EEXIST value here is better.

Also for your second patch, if we replace one disk with existed disk,
printk that info is good.:-)

Regard,
Wang
>
>  or put the error under the verbose option in the btrfs-progs.
>
>
>
>
>> Thanks,
>> Wang
>>>           name = rcu_string_strdup(path, GFP_NOFS);
>>>           if (!name)
>>>               return -ENOMEM;
>>
>> -- 
>> 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
> .
>