[PATCH 1/4] btrfs: preparatory to make btrfs_rm_dev_replace_srcdev() seed aware

[PATCH 1/4] btrfs: preparatory to make btrfs_rm_dev_replace_srcdev() seed aware

[Anand Jain]

There is no logical change in this patch, just a preparatory patch,
so that changes can be easily reasoned.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5f634b6..5fd0132 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1960,19 +1960,23 @@ error_undo:
 void btrfs_rm_dev_replace_srcdev(struct btrfs_fs_info *fs_info,
 				 struct btrfs_device *srcdev)
 {
+	struct btrfs_fs_devices *fs_devices;
+
 	WARN_ON(!mutex_is_locked(&fs_info->fs_devices->device_list_mutex));
 
+	fs_devices = fs_info->fs_devices;
+
 	list_del_rcu(&srcdev->dev_list);
 	list_del_rcu(&srcdev->dev_alloc_list);
-	fs_info->fs_devices->num_devices--;
+	fs_devices->num_devices--;
 	if (srcdev->missing) {
-		fs_info->fs_devices->missing_devices--;
-		fs_info->fs_devices->rw_devices++;
+		fs_devices->missing_devices--;
+		fs_devices->rw_devices++;
 	}
 	if (srcdev->can_discard)
-		fs_info->fs_devices->num_can_discard--;
+		fs_devices->num_can_discard--;
 	if (srcdev->bdev) {
-		fs_info->fs_devices->open_devices--;
+		fs_devices->open_devices--;
 
 		/*
 		 * zero out the old super if it is not writable
-- 
2.0.0.153.g79dcccc

[PATCH 2/4] btrfs: replace seed device followed by unmount causes kernel WARNING

[Anand Jain]

reproducer:
mount /dev/sdb /btrfs
btrfs dev add /dev/sdc /btrfs
btrfs rep start -B /dev/sdb /dev/sdd /btrfs
umount /btrfs

WARNING: CPU: 0 PID: 12661 at fs/btrfs/volumes.c:891 __btrfs_close_devices+0x1b0/0x200 [btrfs]()
::

__btrfs_close_devices()
::
        WARN_ON(fs_devices->open_devices);

After the seed device has been replaced the new target device
is no more a seed device. So we need to update the device
numbers in the fs_devices as pointed by the fs_info.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5fd0132..f098ae7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1964,7 +1964,13 @@ void btrfs_rm_dev_replace_srcdev(struct btrfs_fs_info *fs_info,
 
 	WARN_ON(!mutex_is_locked(&fs_info->fs_devices->device_list_mutex));
 
-	fs_devices = fs_info->fs_devices;
+	/*
+	 * in case of fs with no seed, srcdev->fs_devices will point
+	 * to fs_devices of fs_info. However when the dev being replaced is
+	 * a seed dev it will point to the seed's local fs_devices. In short
+	 * srcdev will have its correct fs_devices in both the cases.
+	 */
+	fs_devices = srcdev->fs_devices;
 
 	list_del_rcu(&srcdev->dev_list);
 	list_del_rcu(&srcdev->dev_alloc_list);
-- 
2.0.0.153.g79dcccc

[PATCH 3/4] btrfs: fix rw_devices miss match after seed replace

[Anand Jain]

reproducer:
    reproducer:
    mount /dev/sdb /btrfs
    btrfs dev add /dev/sdc /btrfs
    btrfs rep start -B /dev/sdb /dev/sdd /btrfs
    umount /btrfs

WARNING: CPU: 0 PID: 3882 at fs/btrfs/volumes.c:892 __btrfs_close_devices+0x1c8/0x200 [btrfs]()

which is

        WARN_ON(fs_devices->rw_devices);

   The problem here is that we did not add one to the rw_devices when
   we replace the seed device with a writable device.

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

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index eea26e1..fb0a7fa 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -562,6 +562,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	if (fs_info->fs_devices->latest_bdev == src_device->bdev)
 		fs_info->fs_devices->latest_bdev = tgt_device->bdev;
 	list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
+	if (src_device->fs_devices->seeding)
+		fs_info->fs_devices->rw_devices++;
 
 	/* replace the sysfs entry */
 	btrfs_kobj_rm_device(fs_info, src_device);
-- 
2.0.0.153.g79dcccc

[PATCH 4/4] btrfs: update sprout seed pointer when seed fs is relinquished

[Anand Jain]

We are not updating sprout fs seed pointer when all seed device
is replaced. This patch will check if all seed device has been
replaced and then update the sprout pointer accordingly.

Same reproducer as in the previous patch would apply here.
And notice that btrfs_close_device will check if seed fs is
present and spits out the error with out this patch.

int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
{
::
                seed_devices = fs_devices->seed;
::
        while (seed_devices) {
                fs_devices = seed_devices;
                seed_devices = fs_devices->seed;
                __btrfs_close_devices(fs_devices);
                free_fs_devices(fs_devices);
        }

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 f098ae7..bfdc11f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1992,6 +1992,25 @@ void btrfs_rm_dev_replace_srcdev(struct btrfs_fs_info *fs_info,
 			btrfs_scratch_superblock(srcdev);
 	}
 
+	/* unless fs_devices is seed fs, num_devices shouldn't go
+	 * zero
+	 */
+	BUG_ON(!fs_devices->num_devices && !fs_devices->seeding);
+
+	/* if this is no devs we rather delete the fs_devices */
+	if (!fs_devices->num_devices) {
+		struct btrfs_fs_devices *tmp_fs_devices;
+
+		tmp_fs_devices = fs_info->fs_devices;
+		while (tmp_fs_devices) {
+			if (tmp_fs_devices->seed == fs_devices) {
+				tmp_fs_devices->seed = fs_devices->seed;
+				break;
+			}
+			tmp_fs_devices = tmp_fs_devices->seed;
+		}
+		fs_devices->seed = NULL;
+	}
 	call_rcu(&srcdev->rcu, free_device);
 }
 
-- 
2.0.0.153.g79dcccc

Re: [PATCH 4/4] btrfs: update sprout seed pointer when seed fs is relinquished

[Miao Xie]

On Mon, 11 Aug 2014 17:42:56 +0800, Anand Jain wrote:
> We are not updating sprout fs seed pointer when all seed device
> is replaced. This patch will check if all seed device has been
> replaced and then update the sprout pointer accordingly.
> 
> Same reproducer as in the previous patch would apply here.
> And notice that btrfs_close_device will check if seed fs is
> present and spits out the error with out this patch.
> 
> int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
> {
> ::
>                 seed_devices = fs_devices->seed;
> ::
>         while (seed_devices) {
>                 fs_devices = seed_devices;
>                 seed_devices = fs_devices->seed;
>                 __btrfs_close_devices(fs_devices);
>                 free_fs_devices(fs_devices);
>         }
> 
> 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 f098ae7..bfdc11f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1992,6 +1992,25 @@ void btrfs_rm_dev_replace_srcdev(struct btrfs_fs_info *fs_info,
>  			btrfs_scratch_superblock(srcdev);
>  	}
>  
> +	/* unless fs_devices is seed fs, num_devices shouldn't go
> +	 * zero
> +	 */

According to coding style, the preferred style for multi-line comments is(except files in net
subsystem):

/*
 * <comment>
 */

> +	BUG_ON(!fs_devices->num_devices && !fs_devices->seeding);

Use ASSERT?

> +	/* if this is no devs we rather delete the fs_devices */
> +	if (!fs_devices->num_devices) {
> +		struct btrfs_fs_devices *tmp_fs_devices;
> +
> +		tmp_fs_devices = fs_info->fs_devices;
> +		while (tmp_fs_devices) {
> +			if (tmp_fs_devices->seed == fs_devices) {
> +				tmp_fs_devices->seed = fs_devices->seed;
> +				break;
> +			}
> +			tmp_fs_devices = tmp_fs_devices->seed;
> +		}
> +		fs_devices->seed = NULL;

Why not free fs_devices like btrfs_rm_device?

The other is OK.

Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>

> +	}
>  	call_rcu(&srcdev->rcu, free_device);
>  }
>  
> 

Re: [PATCH 2/4] btrfs: replace seed device followed by unmount causes kernel WARNING

[Miao Xie]

On Mon, 11 Aug 2014 17:42:54 +0800, Anand Jain wrote:
> reproducer:
> mount /dev/sdb /btrfs
> btrfs dev add /dev/sdc /btrfs
> btrfs rep start -B /dev/sdb /dev/sdd /btrfs
> umount /btrfs
> 
> WARNING: CPU: 0 PID: 12661 at fs/btrfs/volumes.c:891 __btrfs_close_devices+0x1b0/0x200 [btrfs]()
> ::
> 
> __btrfs_close_devices()
> ::
>         WARN_ON(fs_devices->open_devices);
> 
> After the seed device has been replaced the new target device
> is no more a seed device. So we need to update the device
> numbers in the fs_devices as pointed by the fs_info.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5fd0132..f098ae7 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1964,7 +1964,13 @@ void btrfs_rm_dev_replace_srcdev(struct btrfs_fs_info *fs_info,
>  
>  	WARN_ON(!mutex_is_locked(&fs_info->fs_devices->device_list_mutex));
>  
> -	fs_devices = fs_info->fs_devices;
> +	/*
> +	 * in case of fs with no seed, srcdev->fs_devices will point
> +	 * to fs_devices of fs_info. However when the dev being replaced is
> +	 * a seed dev it will point to the seed's local fs_devices. In short
> +	 * srcdev will have its correct fs_devices in both the cases.
> +	 */
> +	fs_devices = srcdev->fs_devices;

Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>

>  
>  	list_del_rcu(&srcdev->dev_list);
>  	list_del_rcu(&srcdev->dev_alloc_list);
> 

Re: [PATCH 3/4] btrfs: fix rw_devices miss match after seed replace

[Miao Xie]

On Mon, 11 Aug 2014 17:42:55 +0800, Anand Jain wrote:
> reproducer:
>     reproducer:
>     mount /dev/sdb /btrfs
>     btrfs dev add /dev/sdc /btrfs
>     btrfs rep start -B /dev/sdb /dev/sdd /btrfs
>     umount /btrfs
> 
> WARNING: CPU: 0 PID: 3882 at fs/btrfs/volumes.c:892 __btrfs_close_devices+0x1c8/0x200 [btrfs]()
> 
> which is
> 
>         WARN_ON(fs_devices->rw_devices);
> 
>    The problem here is that we did not add one to the rw_devices when
>    we replace the seed device with a writable device.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/dev-replace.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index eea26e1..fb0a7fa 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -562,6 +562,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  	if (fs_info->fs_devices->latest_bdev == src_device->bdev)
>  		fs_info->fs_devices->latest_bdev = tgt_device->bdev;
>  	list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
> +	if (src_device->fs_devices->seeding)
> +		fs_info->fs_devices->rw_devices++;

As I said in the previous version of this patch, we might increase ->rw_devices twice if
the source device is a missing device in the seed filesystem. Once is here, the other is
in btrfs_rm_dev_replace_srcdev().

Thanks
Miao

>  
>  	/* replace the sysfs entry */
>  	btrfs_kobj_rm_device(fs_info, src_device);
> 

Re: [PATCH 1/4] btrfs: preparatory to make btrfs_rm_dev_replace_srcdev() seed aware

[Miao Xie]

On Mon, 11 Aug 2014 17:42:53 +0800, Anand Jain wrote:
> There is no logical change in this patch, just a preparatory patch,
> so that changes can be easily reasoned.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5f634b6..5fd0132 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1960,19 +1960,23 @@ error_undo:
>  void btrfs_rm_dev_replace_srcdev(struct btrfs_fs_info *fs_info,
>  				 struct btrfs_device *srcdev)
>  {
> +	struct btrfs_fs_devices *fs_devices;
> +
>  	WARN_ON(!mutex_is_locked(&fs_info->fs_devices->device_list_mutex));
>  
> +	fs_devices = fs_info->fs_devices;
> +
>  	list_del_rcu(&srcdev->dev_list);
>  	list_del_rcu(&srcdev->dev_alloc_list);
> -	fs_info->fs_devices->num_devices--;
> +	fs_devices->num_devices--;
>  	if (srcdev->missing) {
> -		fs_info->fs_devices->missing_devices--;
> -		fs_info->fs_devices->rw_devices++;
> +		fs_devices->missing_devices--;
> +		fs_devices->rw_devices++;
>  	}
>  	if (srcdev->can_discard)
> -		fs_info->fs_devices->num_can_discard--;
> +		fs_devices->num_can_discard--;
>  	if (srcdev->bdev) {
> -		fs_info->fs_devices->open_devices--;
> +		fs_devices->open_devices--;
>  
>  		/*
>  		 * zero out the old super if it is not writable
> 

Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>

Re: [PATCH 4/4] btrfs: update sprout seed pointer when seed fs is relinquished

[Anand Jain]

On 12/08/2014 15:24, Miao Xie wrote:
> On Mon, 11 Aug 2014 17:42:56 +0800, Anand Jain wrote:
>> We are not updating sprout fs seed pointer when all seed device
>> is replaced. This patch will check if all seed device has been
>> replaced and then update the sprout pointer accordingly.
>>
>> Same reproducer as in the previous patch would apply here.
>> And notice that btrfs_close_device will check if seed fs is
>> present and spits out the error with out this patch.
>>
>> int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
>> {
>> ::
>>                  seed_devices = fs_devices->seed;
>> ::
>>          while (seed_devices) {
>>                  fs_devices = seed_devices;
>>                  seed_devices = fs_devices->seed;
>>                  __btrfs_close_devices(fs_devices);
>>                  free_fs_devices(fs_devices);
>>          }
>>
>> 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 f098ae7..bfdc11f 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1992,6 +1992,25 @@ void btrfs_rm_dev_replace_srcdev(struct btrfs_fs_info *fs_info,
>>   			btrfs_scratch_superblock(srcdev);
>>   	}
>>
>> +	/* unless fs_devices is seed fs, num_devices shouldn't go
>> +	 * zero
>> +	 */
>
> According to coding style, the preferred style for multi-line comments is(except files in net
> subsystem):
>
> /*
>   * <comment>
>   */
>
>> +	BUG_ON(!fs_devices->num_devices && !fs_devices->seeding);
>
> Use ASSERT?
>
>> +	/* if this is no devs we rather delete the fs_devices */
>> +	if (!fs_devices->num_devices) {
>> +		struct btrfs_fs_devices *tmp_fs_devices;
>> +
>> +		tmp_fs_devices = fs_info->fs_devices;
>> +		while (tmp_fs_devices) {
>> +			if (tmp_fs_devices->seed == fs_devices) {
>> +				tmp_fs_devices->seed = fs_devices->seed;
>> +				break;
>> +			}
>> +			tmp_fs_devices = tmp_fs_devices->seed;
>> +		}
>> +		fs_devices->seed = NULL;
>
> Why not free fs_devices like btrfs_rm_device?

  Thanks for the review. ! Yes memory leak is another bug
  I have the patch for it will send it soon. the bug trying
  to address here is fs_devices stale seed pointer.

Anand

> The other is OK.
>
> Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>
>
>> +	}
>>   	call_rcu(&srcdev->rcu, free_device);
>>   }
>>
>>
>
> --
> 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/4] btrfs: fix rw_devices miss match after seed replace

[Anand Jain]

On 12/08/2014 15:29, Miao Xie wrote:
> On Mon, 11 Aug 2014 17:42:55 +0800, Anand Jain wrote:
>> reproducer:
>>      reproducer:
>>      mount /dev/sdb /btrfs
>>      btrfs dev add /dev/sdc /btrfs
>>      btrfs rep start -B /dev/sdb /dev/sdd /btrfs
>>      umount /btrfs
>>
>> WARNING: CPU: 0 PID: 3882 at fs/btrfs/volumes.c:892 __btrfs_close_devices+0x1c8/0x200 [btrfs]()
>>
>> which is
>>
>>          WARN_ON(fs_devices->rw_devices);
>>
>>     The problem here is that we did not add one to the rw_devices when
>>     we replace the seed device with a writable device.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/dev-replace.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
>> index eea26e1..fb0a7fa 100644
>> --- a/fs/btrfs/dev-replace.c
>> +++ b/fs/btrfs/dev-replace.c
>> @@ -562,6 +562,8 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>>   	if (fs_info->fs_devices->latest_bdev == src_device->bdev)
>>   		fs_info->fs_devices->latest_bdev = tgt_device->bdev;
>>   	list_add(&tgt_device->dev_alloc_list, &fs_info->fs_devices->alloc_list);
>> +	if (src_device->fs_devices->seeding)
>> +		fs_info->fs_devices->rw_devices++;
>
> As I said in the previous version of this patch, we might increase ->rw_devices twice if
> the source device is a missing device in the seed filesystem. Once is here, the other is
> in btrfs_rm_dev_replace_srcdev().

   Yes that should be fixed. I think a separate patch will do since
   btrfs_rm_dev_replace_srcdev() is invariably incrementing the
   ->rw_devices without checking if the fs is a seed fs.

Anand

> Thanks
> Miao
>
>>
>>   	/* replace the sysfs entry */
>>   	btrfs_kobj_rm_device(fs_info, src_device);
>>
>
> --
> 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
>