[PATCH 1/2] Btrfs: do not resize a seeding device

[PATCH 1/2] Btrfs: do not resize a seeding device

[Liu Bo]

Seeding devices are not supposed to change any more.

Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index f056469..ec2245d 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1303,6 +1303,13 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
 		ret = -EINVAL;
 		goto out_free;
 	}
+	if (device->fs_devices && device->fs_devices->seeding) {
+		printk(KERN_INFO "btrfs: resizer unable to apply on "
+		       "seeding device %s\n", device->name);
+		ret = -EACCES;
+		goto out_free;
+	}
+
 	if (!strcmp(sizestr, "max"))
 		new_size = device->bdev->bd_inode->i_size;
 	else {
-- 
1.6.5.2

[PATCH 2/2] Btrfs: resize all devices when we dont assign a specific device id

[Liu Bo]

This patch fixes two bugs:

When we do not assigne a device id for the resizer,
- it will only take one device to resize, which is supposed to apply on
  all available devices.
- it will take 'id 1' device as default, and this will cause a bug as we
  may have removed the 'id 1' device from the filesystem.

After this patch, we can find all available devices by searching the
chunk tree and resize them:

$ mkfs.btrfs /dev/sdb7
$ mount /dev/sdb7 /mnt/btrfs/
$ btrfs dev add /dev/sdb8 /mnt/btrfs/

$ btrfs fi resize -100m /mnt/btrfs/
then we can get from dmesg:
btrfs: new size for /dev/sdb7 is 980844544
btrfs: new size for /dev/sdb8 is 980844544

$ btrfs fi resize max /mnt/btrfs
then we can get from dmesg:
btrfs: new size for /dev/sdb7 is 1085702144
btrfs: new size for /dev/sdb8 is 1085702144

$ btrfs fi resize 1:-100m /mnt/btrfs
then we can get from dmesg:
btrfs: resizing devid 1
btrfs: new size for /dev/sdb7 is 980844544

$ btrfs fi resize 1:-100m /mnt/btrfs
then we can get from dmesg:
btrfs: resizing devid 2
btrfs: new size for /dev/sdb8 is 980844544

Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c |  101 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index ec2245d..d9a4fa8 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1250,12 +1250,51 @@ out_ra:
 	return ret;
 }
 
+static struct btrfs_device *get_avail_device(struct btrfs_root *root, u64 devid)
+{
+	struct btrfs_key key;
+	struct btrfs_path *path;
+	struct btrfs_dev_item *dev_item;
+	struct btrfs_device *device = NULL;
+	int ret;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return ERR_PTR(-ENOMEM);
+
+	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
+	key.offset = devid;
+	key.type = BTRFS_DEV_ITEM_KEY;
+
+	ret = btrfs_search_slot(NULL, root->fs_info->chunk_root, &key,
+				path, 0, 0);
+	if (ret < 0) {
+		device = ERR_PTR(ret);
+		goto out;
+	}
+	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
+	if (key.objectid != BTRFS_DEV_ITEMS_OBJECTID ||
+	    key.type != BTRFS_DEV_ITEM_KEY) {
+		device = NULL;
+		goto out;
+	}
+	dev_item = btrfs_item_ptr(path->nodes[0], path->slots[0],
+				  struct btrfs_dev_item);
+	devid = btrfs_device_id(path->nodes[0], dev_item);
+
+	device = btrfs_find_device(root, devid, NULL, NULL);
+out:
+	btrfs_free_path(path);
+	return device;
+}
+
 static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
 					void __user *arg)
 {
-	u64 new_size;
+	u64 new_size = 0;
 	u64 old_size;
-	u64 devid = 1;
+	u64 orig_new_size = 0;
+	u64 devid = (-1ULL);
 	struct btrfs_ioctl_vol_args *vol_args;
 	struct btrfs_trans_handle *trans;
 	struct btrfs_device *device = NULL;
@@ -1263,6 +1302,8 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
 	char *devstr = NULL;
 	int ret = 0;
 	int mod = 0;
+	int scan_all = 1;
+	int use_max = 0;
 
 	if (root->fs_info->sb->s_flags & MS_RDONLY)
 		return -EROFS;
@@ -1295,8 +1336,31 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
 		devid = simple_strtoull(devstr, &end, 10);
 		printk(KERN_INFO "btrfs: resizing devid %llu\n",
 		       (unsigned long long)devid);
+		scan_all = 0;
 	}
-	device = btrfs_find_device(root, devid, NULL, NULL);
+
+	if (!strcmp(sizestr, "max")) {
+		use_max = 1;
+	} else {
+		if (sizestr[0] == '-') {
+			mod = -1;
+			sizestr++;
+		} else if (sizestr[0] == '+') {
+			mod = 1;
+			sizestr++;
+		}
+		orig_new_size = memparse(sizestr, NULL);
+		if (orig_new_size == 0) {
+			ret = -EINVAL;
+			goto out_free;
+		}
+	}
+
+	if (devid < (-1ULL))
+		device = btrfs_find_device(root, devid, NULL, NULL);
+	else
+		device = get_avail_device(root, 0);
+again:
 	if (!device) {
 		printk(KERN_INFO "btrfs: resizer unable to find device %llu\n",
 		       (unsigned long long)devid);
@@ -1310,22 +1374,10 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
 		goto out_free;
 	}
 
-	if (!strcmp(sizestr, "max"))
+	if (use_max)
 		new_size = device->bdev->bd_inode->i_size;
-	else {
-		if (sizestr[0] == '-') {
-			mod = -1;
-			sizestr++;
-		} else if (sizestr[0] == '+') {
-			mod = 1;
-			sizestr++;
-		}
-		new_size = memparse(sizestr, NULL);
-		if (new_size == 0) {
-			ret = -EINVAL;
-			goto out_free;
-		}
-	}
+	else
+		new_size = orig_new_size;
 
 	old_size = device->total_bytes;
 
@@ -1365,7 +1417,20 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
 	} else if (new_size < old_size) {
 		ret = btrfs_shrink_device(device, new_size);
 	}
+	if (ret)
+		goto out_free;
 
+	if (scan_all) {
+		/* next available device */
+		device = get_avail_device(root, device->devid + 1);
+		if (!device)
+			goto out_free;
+		if (IS_ERR(device)) {
+			ret = PTR_ERR(device);
+			goto out_free;
+		}
+		goto again;
+	}
 out_free:
 	kfree(vol_args);
 out:
-- 
1.6.5.2

Re: [PATCH 1/2] Btrfs: do not resize a seeding device

[David Sterba]

On Thu, May 17, 2012 at 08:08:08PM +0800, Liu Bo wrote:
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1303,6 +1303,13 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>  		ret = -EINVAL;
>  		goto out_free;
>  	}
> +	if (device->fs_devices && device->fs_devices->seeding) {
> +		printk(KERN_INFO "btrfs: resizer unable to apply on "
> +		       "seeding device %s\n", device->name);
> +		ret = -EACCES;

I think EINVAL would be more appropriate. EACCESS is about permissions
which do not make much sense in context of resizing devices, besides
that CAP_SYS_ADMIN is required anyway (and checked a few lines above).


david

Re: [PATCH 1/2] Btrfs: do not resize a seeding device

[Liu Bo]

On 05/18/2012 09:01 PM, David Sterba wrote:

> On Thu, May 17, 2012 at 08:08:08PM +0800, Liu Bo wrote:
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1303,6 +1303,13 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>  		ret = -EINVAL;
>>  		goto out_free;
>>  	}
>> +	if (device->fs_devices && device->fs_devices->seeding) {
>> +		printk(KERN_INFO "btrfs: resizer unable to apply on "
>> +		       "seeding device %s\n", device->name);
>> +		ret = -EACCES;
> 
> I think EINVAL would be more appropriate. EACCESS is about permissions
> which do not make much sense in context of resizing devices, besides
> that CAP_SYS_ADMIN is required anyway (and checked a few lines above).
> 


That's true, I'll follow your advice.

CAP_SYS_ADMIN has already been there. :)

And thanks for reviewing this.

thanks,
liubo

> 
> david
> --
> 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 2/2] Btrfs: resize all devices when we dont assign a specific device id

[Goffredo Baroncelli]

Hi

On 05/17/2012 02:08 PM, Liu Bo wrote:
> This patch fixes two bugs:
> 
> When we do not assigne a device id for the resizer,
> - it will only take one device to resize, which is supposed to apply on
>   all available devices.
> - it will take 'id 1' device as default, and this will cause a bug as we
>   may have removed the 'id 1' device from the filesystem.
> 
> After this patch, we can find all available devices by searching the
> chunk tree and resize them:


I am not sure that this is a sane default for all resizing.

If the user want to resize to MAX, I agree that it is a sane default,
but when the user want to shrink or enlarge of a fixed quantity, the
user should specific the dev id. Because the shrinking and or the
enlarging should be evaluated case by case.

My suggestion is to change the code at kernel level so in case of
multi-volume file-system the user *has* to specify the device to shrink
and/or enlarge.
Should be the user space btrfs tool to handle the check and the growing
(i.e: if the new size is max, automatically grow all the device up to
max; otherwise the user should specific the device to shrink and/or
enlarge).

BR
Goffredo


> 
> $ mkfs.btrfs /dev/sdb7
> $ mount /dev/sdb7 /mnt/btrfs/
> $ btrfs dev add /dev/sdb8 /mnt/btrfs/
> 
> $ btrfs fi resize -100m /mnt/btrfs/
> then we can get from dmesg:
> btrfs: new size for /dev/sdb7 is 980844544
> btrfs: new size for /dev/sdb8 is 980844544
> 
> $ btrfs fi resize max /mnt/btrfs
> then we can get from dmesg:
> btrfs: new size for /dev/sdb7 is 1085702144
> btrfs: new size for /dev/sdb8 is 1085702144
> 
> $ btrfs fi resize 1:-100m /mnt/btrfs
> then we can get from dmesg:
> btrfs: resizing devid 1
> btrfs: new size for /dev/sdb7 is 980844544
> 
> $ btrfs fi resize 1:-100m /mnt/btrfs
> then we can get from dmesg:
> btrfs: resizing devid 2
> btrfs: new size for /dev/sdb8 is 980844544
> 
> Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
> ---
>  fs/btrfs/ioctl.c |  101 ++++++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 83 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index ec2245d..d9a4fa8 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1250,12 +1250,51 @@ out_ra:
>  	return ret;
>  }
>  
> +static struct btrfs_device *get_avail_device(struct btrfs_root *root, u64 devid)
> +{
> +	struct btrfs_key key;
> +	struct btrfs_path *path;
> +	struct btrfs_dev_item *dev_item;
> +	struct btrfs_device *device = NULL;
> +	int ret;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return ERR_PTR(-ENOMEM);
> +
> +	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
> +	key.offset = devid;
> +	key.type = BTRFS_DEV_ITEM_KEY;
> +
> +	ret = btrfs_search_slot(NULL, root->fs_info->chunk_root, &key,
> +				path, 0, 0);
> +	if (ret < 0) {
> +		device = ERR_PTR(ret);
> +		goto out;
> +	}
> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> +	if (key.objectid != BTRFS_DEV_ITEMS_OBJECTID ||
> +	    key.type != BTRFS_DEV_ITEM_KEY) {
> +		device = NULL;
> +		goto out;
> +	}
> +	dev_item = btrfs_item_ptr(path->nodes[0], path->slots[0],
> +				  struct btrfs_dev_item);
> +	devid = btrfs_device_id(path->nodes[0], dev_item);
> +
> +	device = btrfs_find_device(root, devid, NULL, NULL);
> +out:
> +	btrfs_free_path(path);
> +	return device;
> +}
> +
>  static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>  					void __user *arg)
>  {
> -	u64 new_size;
> +	u64 new_size = 0;
>  	u64 old_size;
> -	u64 devid = 1;
> +	u64 orig_new_size = 0;
> +	u64 devid = (-1ULL);
>  	struct btrfs_ioctl_vol_args *vol_args;
>  	struct btrfs_trans_handle *trans;
>  	struct btrfs_device *device = NULL;
> @@ -1263,6 +1302,8 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>  	char *devstr = NULL;
>  	int ret = 0;
>  	int mod = 0;
> +	int scan_all = 1;
> +	int use_max = 0;
>  
>  	if (root->fs_info->sb->s_flags & MS_RDONLY)
>  		return -EROFS;
> @@ -1295,8 +1336,31 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>  		devid = simple_strtoull(devstr, &end, 10);
>  		printk(KERN_INFO "btrfs: resizing devid %llu\n",
>  		       (unsigned long long)devid);
> +		scan_all = 0;
>  	}
> -	device = btrfs_find_device(root, devid, NULL, NULL);
> +
> +	if (!strcmp(sizestr, "max")) {
> +		use_max = 1;
> +	} else {
> +		if (sizestr[0] == '-') {
> +			mod = -1;
> +			sizestr++;
> +		} else if (sizestr[0] == '+') {
> +			mod = 1;
> +			sizestr++;
> +		}
> +		orig_new_size = memparse(sizestr, NULL);
> +		if (orig_new_size == 0) {
> +			ret = -EINVAL;
> +			goto out_free;
> +		}
> +	}
> +
> +	if (devid < (-1ULL))
> +		device = btrfs_find_device(root, devid, NULL, NULL);
> +	else
> +		device = get_avail_device(root, 0);
> +again:
>  	if (!device) {
>  		printk(KERN_INFO "btrfs: resizer unable to find device %llu\n",
>  		       (unsigned long long)devid);
> @@ -1310,22 +1374,10 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>  		goto out_free;
>  	}
>  
> -	if (!strcmp(sizestr, "max"))
> +	if (use_max)
>  		new_size = device->bdev->bd_inode->i_size;
> -	else {
> -		if (sizestr[0] == '-') {
> -			mod = -1;
> -			sizestr++;
> -		} else if (sizestr[0] == '+') {
> -			mod = 1;
> -			sizestr++;
> -		}
> -		new_size = memparse(sizestr, NULL);
> -		if (new_size == 0) {
> -			ret = -EINVAL;
> -			goto out_free;
> -		}
> -	}
> +	else
> +		new_size = orig_new_size;
>  
>  	old_size = device->total_bytes;
>  
> @@ -1365,7 +1417,20 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>  	} else if (new_size < old_size) {
>  		ret = btrfs_shrink_device(device, new_size);
>  	}
> +	if (ret)
> +		goto out_free;
>  
> +	if (scan_all) {
> +		/* next available device */
> +		device = get_avail_device(root, device->devid + 1);
> +		if (!device)
> +			goto out_free;
> +		if (IS_ERR(device)) {
> +			ret = PTR_ERR(device);
> +			goto out_free;
> +		}
> +		goto again;
> +	}
>  out_free:
>  	kfree(vol_args);
>  out:

Re: [PATCH 2/2] Btrfs: resize all devices when we dont assign a specific device id

[Liu Bo]

On 05/23/2012 01:21 PM, Goffredo Baroncelli wrote:

> Hi
> 
> On 05/17/2012 02:08 PM, Liu Bo wrote:
>> This patch fixes two bugs:
>>
>> When we do not assigne a device id for the resizer,
>> - it will only take one device to resize, which is supposed to apply on
>>   all available devices.
>> - it will take 'id 1' device as default, and this will cause a bug as we
>>   may have removed the 'id 1' device from the filesystem.
>>
>> After this patch, we can find all available devices by searching the
>> chunk tree and resize them:
> 
> 
> I am not sure that this is a sane default for all resizing.
> 
> If the user want to resize to MAX, I agree that it is a sane default,
> but when the user want to shrink or enlarge of a fixed quantity, the
> user should specific the dev id. Because the shrinking and or the
> enlarging should be evaluated case by case.
> 
> My suggestion is to change the code at kernel level so in case of
> multi-volume file-system the user *has* to specify the device to shrink
> and/or enlarge.
> Should be the user space btrfs tool to handle the check and the growing
> (i.e: if the new size is max, automatically grow all the device up to
> max; otherwise the user should specific the device to shrink and/or
> enlarge).
> 


Hi,

It is quite easy to do what you expect, but IMO that will confuse users a lot...

With this patch, you can still assign a specific dev id to resize what you want :)

thanks,
liubo

> BR
> Goffredo
> 
> 
>> $ mkfs.btrfs /dev/sdb7
>> $ mount /dev/sdb7 /mnt/btrfs/
>> $ btrfs dev add /dev/sdb8 /mnt/btrfs/
>>
>> $ btrfs fi resize -100m /mnt/btrfs/
>> then we can get from dmesg:
>> btrfs: new size for /dev/sdb7 is 980844544
>> btrfs: new size for /dev/sdb8 is 980844544
>>
>> $ btrfs fi resize max /mnt/btrfs
>> then we can get from dmesg:
>> btrfs: new size for /dev/sdb7 is 1085702144
>> btrfs: new size for /dev/sdb8 is 1085702144
>>
>> $ btrfs fi resize 1:-100m /mnt/btrfs
>> then we can get from dmesg:
>> btrfs: resizing devid 1
>> btrfs: new size for /dev/sdb7 is 980844544
>>
>> $ btrfs fi resize 1:-100m /mnt/btrfs
>> then we can get from dmesg:
>> btrfs: resizing devid 2
>> btrfs: new size for /dev/sdb8 is 980844544
>>
>> Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
>> ---
>>  fs/btrfs/ioctl.c |  101 ++++++++++++++++++++++++++++++++++++++++++++----------
>>  1 files changed, 83 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index ec2245d..d9a4fa8 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1250,12 +1250,51 @@ out_ra:
>>  	return ret;
>>  }
>>  
>> +static struct btrfs_device *get_avail_device(struct btrfs_root *root, u64 devid)
>> +{
>> +	struct btrfs_key key;
>> +	struct btrfs_path *path;
>> +	struct btrfs_dev_item *dev_item;
>> +	struct btrfs_device *device = NULL;
>> +	int ret;
>> +
>> +	path = btrfs_alloc_path();
>> +	if (!path)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
>> +	key.offset = devid;
>> +	key.type = BTRFS_DEV_ITEM_KEY;
>> +
>> +	ret = btrfs_search_slot(NULL, root->fs_info->chunk_root, &key,
>> +				path, 0, 0);
>> +	if (ret < 0) {
>> +		device = ERR_PTR(ret);
>> +		goto out;
>> +	}
>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>> +	if (key.objectid != BTRFS_DEV_ITEMS_OBJECTID ||
>> +	    key.type != BTRFS_DEV_ITEM_KEY) {
>> +		device = NULL;
>> +		goto out;
>> +	}
>> +	dev_item = btrfs_item_ptr(path->nodes[0], path->slots[0],
>> +				  struct btrfs_dev_item);
>> +	devid = btrfs_device_id(path->nodes[0], dev_item);
>> +
>> +	device = btrfs_find_device(root, devid, NULL, NULL);
>> +out:
>> +	btrfs_free_path(path);
>> +	return device;
>> +}
>> +
>>  static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>  					void __user *arg)
>>  {
>> -	u64 new_size;
>> +	u64 new_size = 0;
>>  	u64 old_size;
>> -	u64 devid = 1;
>> +	u64 orig_new_size = 0;
>> +	u64 devid = (-1ULL);
>>  	struct btrfs_ioctl_vol_args *vol_args;
>>  	struct btrfs_trans_handle *trans;
>>  	struct btrfs_device *device = NULL;
>> @@ -1263,6 +1302,8 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>  	char *devstr = NULL;
>>  	int ret = 0;
>>  	int mod = 0;
>> +	int scan_all = 1;
>> +	int use_max = 0;
>>  
>>  	if (root->fs_info->sb->s_flags & MS_RDONLY)
>>  		return -EROFS;
>> @@ -1295,8 +1336,31 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>  		devid = simple_strtoull(devstr, &end, 10);
>>  		printk(KERN_INFO "btrfs: resizing devid %llu\n",
>>  		       (unsigned long long)devid);
>> +		scan_all = 0;
>>  	}
>> -	device = btrfs_find_device(root, devid, NULL, NULL);
>> +
>> +	if (!strcmp(sizestr, "max")) {
>> +		use_max = 1;
>> +	} else {
>> +		if (sizestr[0] == '-') {
>> +			mod = -1;
>> +			sizestr++;
>> +		} else if (sizestr[0] == '+') {
>> +			mod = 1;
>> +			sizestr++;
>> +		}
>> +		orig_new_size = memparse(sizestr, NULL);
>> +		if (orig_new_size == 0) {
>> +			ret = -EINVAL;
>> +			goto out_free;
>> +		}
>> +	}
>> +
>> +	if (devid < (-1ULL))
>> +		device = btrfs_find_device(root, devid, NULL, NULL);
>> +	else
>> +		device = get_avail_device(root, 0);
>> +again:
>>  	if (!device) {
>>  		printk(KERN_INFO "btrfs: resizer unable to find device %llu\n",
>>  		       (unsigned long long)devid);
>> @@ -1310,22 +1374,10 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>  		goto out_free;
>>  	}
>>  
>> -	if (!strcmp(sizestr, "max"))
>> +	if (use_max)
>>  		new_size = device->bdev->bd_inode->i_size;
>> -	else {
>> -		if (sizestr[0] == '-') {
>> -			mod = -1;
>> -			sizestr++;
>> -		} else if (sizestr[0] == '+') {
>> -			mod = 1;
>> -			sizestr++;
>> -		}
>> -		new_size = memparse(sizestr, NULL);
>> -		if (new_size == 0) {
>> -			ret = -EINVAL;
>> -			goto out_free;
>> -		}
>> -	}
>> +	else
>> +		new_size = orig_new_size;
>>  
>>  	old_size = device->total_bytes;
>>  
>> @@ -1365,7 +1417,20 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>  	} else if (new_size < old_size) {
>>  		ret = btrfs_shrink_device(device, new_size);
>>  	}
>> +	if (ret)
>> +		goto out_free;
>>  
>> +	if (scan_all) {
>> +		/* next available device */
>> +		device = get_avail_device(root, device->devid + 1);
>> +		if (!device)
>> +			goto out_free;
>> +		if (IS_ERR(device)) {
>> +			ret = PTR_ERR(device);
>> +			goto out_free;
>> +		}
>> +		goto again;
>> +	}
>>  out_free:
>>  	kfree(vol_args);
>>  out:
> 
> --
> 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 2/2] Btrfs: resize all devices when we dont assign a specific device id

[Goffredo Baroncelli]

On 05/24/2012 04:15 AM, Liu Bo wrote:
> On 05/23/2012 01:21 PM, Goffredo Baroncelli wrote:
> 
>> Hi
>>
>> On 05/17/2012 02:08 PM, Liu Bo wrote:
>>> This patch fixes two bugs:
>>>
>>> When we do not assigne a device id for the resizer,
>>> - it will only take one device to resize, which is supposed to apply on
>>>   all available devices.
>>> - it will take 'id 1' device as default, and this will cause a bug as we
>>>   may have removed the 'id 1' device from the filesystem.
>>>
>>> After this patch, we can find all available devices by searching the
>>> chunk tree and resize them:
>>
>>
>> I am not sure that this is a sane default for all resizing.
>>
>> If the user want to resize to MAX, I agree that it is a sane default,
>> but when the user want to shrink or enlarge of a fixed quantity, the
>> user should specific the dev id. Because the shrinking and or the
>> enlarging should be evaluated case by case.
>>
>> My suggestion is to change the code at kernel level so in case of
>> multi-volume file-system the user *has* to specify the device to shrink
>> and/or enlarge.
>> Should be the user space btrfs tool to handle the check and the growing
>> (i.e: if the new size is max, automatically grow all the device up to
>> max; otherwise the user should specific the device to shrink and/or
>> enlarge).
>>
> 
> 
> Hi,
> 
> It is quite easy to do what you expect, but IMO that will confuse users a lot...

What happens if one device (of a pool) is too small to hosts a
filesystem enlargement ? How from kernel space the error is returned ?

In kernel space is more complex to handle an error. If there is a single
device there is not problem: the error is linked to the single device.
But in the multiple devices case, which one fails ?

Think also to other error like a dive missing.

Returning a generic error and force the user to see log is a too simple
approach.

> 
> With this patch, you can still assign a specific dev id to resize what you want :)

Of course everything that could be done in user-space could be done in
kernel space.
I think that as general rule we should put all the code in user space
except if there are performance problem and/or missing information. In
this case the userspace could have all the information (see
scrub_fs_info() and scrub_device_info() on how acquire the filesystem
information ) and there is no performance problem.


> 
> thanks,
> liubo
> 
>> BR
>> Goffredo
>>
>>
>>> $ mkfs.btrfs /dev/sdb7
>>> $ mount /dev/sdb7 /mnt/btrfs/
>>> $ btrfs dev add /dev/sdb8 /mnt/btrfs/
>>>
>>> $ btrfs fi resize -100m /mnt/btrfs/
>>> then we can get from dmesg:
>>> btrfs: new size for /dev/sdb7 is 980844544
>>> btrfs: new size for /dev/sdb8 is 980844544
>>>
>>> $ btrfs fi resize max /mnt/btrfs
>>> then we can get from dmesg:
>>> btrfs: new size for /dev/sdb7 is 1085702144
>>> btrfs: new size for /dev/sdb8 is 1085702144
>>>
>>> $ btrfs fi resize 1:-100m /mnt/btrfs
>>> then we can get from dmesg:
>>> btrfs: resizing devid 1
>>> btrfs: new size for /dev/sdb7 is 980844544
>>>
>>> $ btrfs fi resize 1:-100m /mnt/btrfs
>>> then we can get from dmesg:
>>> btrfs: resizing devid 2
>>> btrfs: new size for /dev/sdb8 is 980844544
>>>
>>> Signed-off-by: Liu Bo <liubo2009@cn.fujitsu.com>
>>> ---
>>>  fs/btrfs/ioctl.c |  101 ++++++++++++++++++++++++++++++++++++++++++++----------
>>>  1 files changed, 83 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index ec2245d..d9a4fa8 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -1250,12 +1250,51 @@ out_ra:
>>>  	return ret;
>>>  }
>>>  
>>> +static struct btrfs_device *get_avail_device(struct btrfs_root *root, u64 devid)
>>> +{
>>> +	struct btrfs_key key;
>>> +	struct btrfs_path *path;
>>> +	struct btrfs_dev_item *dev_item;
>>> +	struct btrfs_device *device = NULL;
>>> +	int ret;
>>> +
>>> +	path = btrfs_alloc_path();
>>> +	if (!path)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
>>> +	key.offset = devid;
>>> +	key.type = BTRFS_DEV_ITEM_KEY;
>>> +
>>> +	ret = btrfs_search_slot(NULL, root->fs_info->chunk_root, &key,
>>> +				path, 0, 0);
>>> +	if (ret < 0) {
>>> +		device = ERR_PTR(ret);
>>> +		goto out;
>>> +	}
>>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>> +	if (key.objectid != BTRFS_DEV_ITEMS_OBJECTID ||
>>> +	    key.type != BTRFS_DEV_ITEM_KEY) {
>>> +		device = NULL;
>>> +		goto out;
>>> +	}
>>> +	dev_item = btrfs_item_ptr(path->nodes[0], path->slots[0],
>>> +				  struct btrfs_dev_item);
>>> +	devid = btrfs_device_id(path->nodes[0], dev_item);
>>> +
>>> +	device = btrfs_find_device(root, devid, NULL, NULL);
>>> +out:
>>> +	btrfs_free_path(path);
>>> +	return device;
>>> +}
>>> +
>>>  static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>>  					void __user *arg)
>>>  {
>>> -	u64 new_size;
>>> +	u64 new_size = 0;
>>>  	u64 old_size;
>>> -	u64 devid = 1;
>>> +	u64 orig_new_size = 0;
>>> +	u64 devid = (-1ULL);
>>>  	struct btrfs_ioctl_vol_args *vol_args;
>>>  	struct btrfs_trans_handle *trans;
>>>  	struct btrfs_device *device = NULL;
>>> @@ -1263,6 +1302,8 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>>  	char *devstr = NULL;
>>>  	int ret = 0;
>>>  	int mod = 0;
>>> +	int scan_all = 1;
>>> +	int use_max = 0;
>>>  
>>>  	if (root->fs_info->sb->s_flags & MS_RDONLY)
>>>  		return -EROFS;
>>> @@ -1295,8 +1336,31 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>>  		devid = simple_strtoull(devstr, &end, 10);
>>>  		printk(KERN_INFO "btrfs: resizing devid %llu\n",
>>>  		       (unsigned long long)devid);
>>> +		scan_all = 0;
>>>  	}
>>> -	device = btrfs_find_device(root, devid, NULL, NULL);
>>> +
>>> +	if (!strcmp(sizestr, "max")) {
>>> +		use_max = 1;
>>> +	} else {
>>> +		if (sizestr[0] == '-') {
>>> +			mod = -1;
>>> +			sizestr++;
>>> +		} else if (sizestr[0] == '+') {
>>> +			mod = 1;
>>> +			sizestr++;
>>> +		}
>>> +		orig_new_size = memparse(sizestr, NULL);
>>> +		if (orig_new_size == 0) {
>>> +			ret = -EINVAL;
>>> +			goto out_free;
>>> +		}
>>> +	}
>>> +
>>> +	if (devid < (-1ULL))
>>> +		device = btrfs_find_device(root, devid, NULL, NULL);
>>> +	else
>>> +		device = get_avail_device(root, 0);
>>> +again:
>>>  	if (!device) {
>>>  		printk(KERN_INFO "btrfs: resizer unable to find device %llu\n",
>>>  		       (unsigned long long)devid);
>>> @@ -1310,22 +1374,10 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>>  		goto out_free;
>>>  	}
>>>  
>>> -	if (!strcmp(sizestr, "max"))
>>> +	if (use_max)
>>>  		new_size = device->bdev->bd_inode->i_size;
>>> -	else {
>>> -		if (sizestr[0] == '-') {
>>> -			mod = -1;
>>> -			sizestr++;
>>> -		} else if (sizestr[0] == '+') {
>>> -			mod = 1;
>>> -			sizestr++;
>>> -		}
>>> -		new_size = memparse(sizestr, NULL);
>>> -		if (new_size == 0) {
>>> -			ret = -EINVAL;
>>> -			goto out_free;
>>> -		}
>>> -	}
>>> +	else
>>> +		new_size = orig_new_size;
>>>  
>>>  	old_size = device->total_bytes;
>>>  
>>> @@ -1365,7 +1417,20 @@ static noinline int btrfs_ioctl_resize(struct btrfs_root *root,
>>>  	} else if (new_size < old_size) {
>>>  		ret = btrfs_shrink_device(device, new_size);
>>>  	}
>>> +	if (ret)
>>> +		goto out_free;
>>>  
>>> +	if (scan_all) {
>>> +		/* next available device */
>>> +		device = get_avail_device(root, device->devid + 1);
>>> +		if (!device)
>>> +			goto out_free;
>>> +		if (IS_ERR(device)) {
>>> +			ret = PTR_ERR(device);
>>> +			goto out_free;
>>> +		}
>>> +		goto again;
>>> +	}
>>>  out_free:
>>>  	kfree(vol_args);
>>>  out:
>>
>> --
>> 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
>>
> 
> 
> .
>