[PATCH] btrfs: add an ioctl to force chunk allocation

[PATCH] btrfs: add an ioctl to force chunk allocation

[Josef Bacik]

In testing block group removal it's sometimes handy to be able to create
block groups on demand.  Add an ioctl to allow us to force allocation
from userspace.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ioctl.c           | 30 ++++++++++++++++++++++++++++++
 include/uapi/linux/btrfs.h |  1 +
 2 files changed, 31 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d0743ec1231d..f100def53c29 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -5553,6 +5553,34 @@ static int _btrfs_ioctl_send(struct file *file, void __user *argp, bool compat)
 	return ret;
 }
 
+static long btrfs_ioctl_alloc_chunk(struct file *file, void __user *arg)
+{
+	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
+	struct btrfs_trans_handle *trans;
+	u64 flags;
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (copy_from_user(&flags, arg, sizeof(flags)))
+		return -EFAULT;
+
+	/* We can only specify one type at a time. */
+	if (flags != BTRFS_BLOCK_GROUP_DATA &&
+	    flags != BTRFS_BLOCK_GROUP_METADATA &&
+	    flags != BTRFS_BLOCK_GROUP_SYSTEM)
+		return -EINVAL;
+
+	trans = btrfs_start_transaction(root, 0);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
+	ret = btrfs_chunk_alloc(trans, flags, CHUNK_ALLOC_FORCE);
+	btrfs_end_transaction(trans);
+	return ret < 0 ? ret : 0;
+}
+
 long btrfs_ioctl(struct file *file, unsigned int
 		cmd, unsigned long arg)
 {
@@ -5699,6 +5727,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_get_subvol_rootref(file, argp);
 	case BTRFS_IOC_INO_LOOKUP_USER:
 		return btrfs_ioctl_ino_lookup_user(file, argp);
+	case BTRFS_IOC_ALLOC_CHUNK:
+		return btrfs_ioctl_alloc_chunk(file, argp);
 	}
 
 	return -ENOTTY;
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index c195896d478f..3a6474c34ad0 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -943,5 +943,6 @@ enum btrfs_err_code {
 				struct btrfs_ioctl_get_subvol_rootref_args)
 #define BTRFS_IOC_INO_LOOKUP_USER _IOWR(BTRFS_IOCTL_MAGIC, 62, \
 				struct btrfs_ioctl_ino_lookup_user_args)
+#define BTRFS_IOC_ALLOC_CHUNK _IOR(BTRFS_IOCTL_MAGIC, 63, __u64)
 
 #endif /* _UAPI_LINUX_BTRFS_H */
-- 
2.21.0

Re: [PATCH] btrfs: add an ioctl to force chunk allocation

[Holger Hoffstätte]

On 8/2/19 6:10 PM, Josef Bacik wrote:
> In testing block group removal it's sometimes handy to be able to create
> block groups on demand.  Add an ioctl to allow us to force allocation
> from userspace.

Gave this a try and it works as advertised, though I noticed that the
redundancy level is ignored, e.g. adding a single metadata chunk will
add a new "single" chunk even when the metadata level is dup.
Doing a balance -mconvert dup,soft fixes that right up, but it's IMHO
unexpected. Can you put a cherry on top and create the new chunk according
to its dup level?

thanks :)
Holger

Re: [PATCH] btrfs: add an ioctl to force chunk allocation

[Hans van Kranenburg]

On 8/5/19 12:20 PM, Holger Hoffstätte wrote:
> On 8/2/19 6:10 PM, Josef Bacik wrote:
>> In testing block group removal it's sometimes handy to be able to create
>> block groups on demand.  Add an ioctl to allow us to force allocation
>> from userspace.
> 
> Gave this a try and it works as advertised, though I noticed that the
> redundancy level is ignored, e.g. adding a single metadata chunk will
> add a new "single" chunk even when the metadata level is dup.
> Doing a balance -mconvert dup,soft fixes that right up, but it's IMHO
> unexpected. Can you put a cherry on top and create the new chunk according
> to its dup level?

Looking at the code, you should get -EINVAL when you specify anything
else than single? (because of the != comparisons).

If this gets in, and is available without doing some special debug style
kernel build, then it will (tm) be (ab)used by users in the future for
things we didn't imagine today. So, in that case, it makes sense to be
able to specify any valid combination of flags (type+profile), like
indeed METADATA|DUP or whatever.

Hans

Re: [PATCH] btrfs: add an ioctl to force chunk allocation

[Holger Hoffstätte]

On 8/5/19 12:31 PM, Hans van Kranenburg wrote:
> On 8/5/19 12:20 PM, Holger Hoffstätte wrote:
>> On 8/2/19 6:10 PM, Josef Bacik wrote:
>>> In testing block group removal it's sometimes handy to be able to create
>>> block groups on demand.  Add an ioctl to allow us to force allocation
>>> from userspace.
>>
>> Gave this a try and it works as advertised, though I noticed that the
>> redundancy level is ignored, e.g. adding a single metadata chunk will
>> add a new "single" chunk even when the metadata level is dup.
>> Doing a balance -mconvert dup,soft fixes that right up, but it's IMHO
>> unexpected. Can you put a cherry on top and create the new chunk according
>> to its dup level?
> 
> Looking at the code, you should get -EINVAL when you specify anything
> else than single? (because of the != comparisons).
> 
> If this gets in, and is available without doing some special debug style
> kernel build, then it will (tm) be (ab)used by users in the future for
> things we didn't imagine today. So, in that case, it makes sense to be
> able to specify any valid combination of flags (type+profile), like
> indeed METADATA|DUP or whatever.

..and that's precisely why it (IMHO) should simply do The Right Thing
by default instead of giving people more footguns with unexpected
behaviour. Just observe the existing profile in the kernel ioctl and
act accordingly, no need for more user-supplied flags. Just don't.

Holger

Re: [PATCH] btrfs: add an ioctl to force chunk allocation

[Hans van Kranenburg]

On 8/5/19 12:56 PM, Holger Hoffstätte wrote:
> On 8/5/19 12:31 PM, Hans van Kranenburg wrote:
>> On 8/5/19 12:20 PM, Holger Hoffstätte wrote:
>>> On 8/2/19 6:10 PM, Josef Bacik wrote:
>>>> In testing block group removal it's sometimes handy to be able to
>>>> create
>>>> block groups on demand.  Add an ioctl to allow us to force allocation
>>>> from userspace.
>>>
>>> Gave this a try and it works as advertised, though I noticed that the
>>> redundancy level is ignored, e.g. adding a single metadata chunk will
>>> add a new "single" chunk even when the metadata level is dup.
>>> Doing a balance -mconvert dup,soft fixes that right up, but it's IMHO
>>> unexpected. Can you put a cherry on top and create the new chunk
>>> according
>>> to its dup level?
>>
>> Looking at the code, you should get -EINVAL when you specify anything
>> else than single? (because of the != comparisons).
>>
>> If this gets in, and is available without doing some special debug style
>> kernel build, then it will (tm) be (ab)used by users in the future for
>> things we didn't imagine today. So, in that case, it makes sense to be
>> able to specify any valid combination of flags (type+profile), like
>> indeed METADATA|DUP or whatever.
> 
> ..and that's precisely why it (IMHO) should simply do The Right Thing
> by default instead of giving people more footguns with unexpected
> behaviour. Just observe the existing profile in the kernel ioctl and
> act accordingly, no need for more user-supplied flags. Just don't.

You're right. That's better than my suggestion.

+1

Hans

Re: [PATCH] btrfs: add an ioctl to force chunk allocation

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2868 bytes --]



On 2019/8/3 上午12:10, Josef Bacik wrote:
> In testing block group removal it's sometimes handy to be able to create
> block groups on demand.  Add an ioctl to allow us to force allocation
> from userspace.

Not sure if we should add another ioctl just for debug purpose.

Although I see the usefulness in such debug feature, can we move it to
something like sysfs so we can hide it more easily?

> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/ioctl.c           | 30 ++++++++++++++++++++++++++++++
>  include/uapi/linux/btrfs.h |  1 +
>  2 files changed, 31 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index d0743ec1231d..f100def53c29 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -5553,6 +5553,34 @@ static int _btrfs_ioctl_send(struct file *file, void __user *argp, bool compat)
>  	return ret;
>  }
>  
> +static long btrfs_ioctl_alloc_chunk(struct file *file, void __user *arg)
> +{
> +	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
> +	struct btrfs_trans_handle *trans;
> +	u64 flags;
> +	int ret;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (copy_from_user(&flags, arg, sizeof(flags)))
> +		return -EFAULT;
> +
> +	/* We can only specify one type at a time. */
> +	if (flags != BTRFS_BLOCK_GROUP_DATA &&
> +	    flags != BTRFS_BLOCK_GROUP_METADATA &&
> +	    flags != BTRFS_BLOCK_GROUP_SYSTEM)
> +		return -EINVAL;

It looks like MIXED bg get less and less love.

> +
> +	trans = btrfs_start_transaction(root, 0);
> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
> +
> +	ret = btrfs_chunk_alloc(trans, flags, CHUNK_ALLOC_FORCE);

And the flags lacks the profile bits, thus default to SINGLE.
Is it designed or you'd better use btrfs_force_chunk_alloc()?

Thanks,
Qu

> +	btrfs_end_transaction(trans);
> +	return ret < 0 ? ret : 0;
> +}
> +
>  long btrfs_ioctl(struct file *file, unsigned int
>  		cmd, unsigned long arg)
>  {
> @@ -5699,6 +5727,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>  		return btrfs_ioctl_get_subvol_rootref(file, argp);
>  	case BTRFS_IOC_INO_LOOKUP_USER:
>  		return btrfs_ioctl_ino_lookup_user(file, argp);
> +	case BTRFS_IOC_ALLOC_CHUNK:
> +		return btrfs_ioctl_alloc_chunk(file, argp);
>  	}
>  
>  	return -ENOTTY;
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index c195896d478f..3a6474c34ad0 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -943,5 +943,6 @@ enum btrfs_err_code {
>  				struct btrfs_ioctl_get_subvol_rootref_args)
>  #define BTRFS_IOC_INO_LOOKUP_USER _IOWR(BTRFS_IOCTL_MAGIC, 62, \
>  				struct btrfs_ioctl_ino_lookup_user_args)
> +#define BTRFS_IOC_ALLOC_CHUNK _IOR(BTRFS_IOCTL_MAGIC, 63, __u64)
>  
>  #endif /* _UAPI_LINUX_BTRFS_H */
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: add an ioctl to force chunk allocation

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 4091 bytes --]

   Just to throw a can of turquoise paint into the bike-shed works,
this could be handy with a list of devids to say *where* to create the
new block group.

   This allows for some truly horrible things to happen if abused, but
could also allow for some kind of poor-mans directed-balance: Create a
new block group on the devices you want, balance away one block group
on device(s) you don't want -- data should end up going to the new
empty block group in preference to another new one being automatically
allocated.

   (Alternatively, ignore this suggestion, and I'll just wait for a
proper "move this BG to these devices" ioctl...)

   Hugo.

On Mon, Aug 05, 2019 at 08:24:23PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/8/3 上午12:10, Josef Bacik wrote:
> > In testing block group removal it's sometimes handy to be able to create
> > block groups on demand.  Add an ioctl to allow us to force allocation
> > from userspace.
> 
> Not sure if we should add another ioctl just for debug purpose.
> 
> Although I see the usefulness in such debug feature, can we move it to
> something like sysfs so we can hide it more easily?
> 
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/ioctl.c           | 30 ++++++++++++++++++++++++++++++
> >  include/uapi/linux/btrfs.h |  1 +
> >  2 files changed, 31 insertions(+)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index d0743ec1231d..f100def53c29 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -5553,6 +5553,34 @@ static int _btrfs_ioctl_send(struct file *file, void __user *argp, bool compat)
> >  	return ret;
> >  }
> >  
> > +static long btrfs_ioctl_alloc_chunk(struct file *file, void __user *arg)
> > +{
> > +	struct btrfs_root *root = BTRFS_I(file_inode(file))->root;
> > +	struct btrfs_trans_handle *trans;
> > +	u64 flags;
> > +	int ret;
> > +
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return -EPERM;
> > +
> > +	if (copy_from_user(&flags, arg, sizeof(flags)))
> > +		return -EFAULT;
> > +
> > +	/* We can only specify one type at a time. */
> > +	if (flags != BTRFS_BLOCK_GROUP_DATA &&
> > +	    flags != BTRFS_BLOCK_GROUP_METADATA &&
> > +	    flags != BTRFS_BLOCK_GROUP_SYSTEM)
> > +		return -EINVAL;
> 
> It looks like MIXED bg get less and less love.
> 
> > +
> > +	trans = btrfs_start_transaction(root, 0);
> > +	if (IS_ERR(trans))
> > +		return PTR_ERR(trans);
> > +
> > +	ret = btrfs_chunk_alloc(trans, flags, CHUNK_ALLOC_FORCE);
> 
> And the flags lacks the profile bits, thus default to SINGLE.
> Is it designed or you'd better use btrfs_force_chunk_alloc()?
> 
> Thanks,
> Qu
> 
> > +	btrfs_end_transaction(trans);
> > +	return ret < 0 ? ret : 0;
> > +}
> > +
> >  long btrfs_ioctl(struct file *file, unsigned int
> >  		cmd, unsigned long arg)
> >  {
> > @@ -5699,6 +5727,8 @@ long btrfs_ioctl(struct file *file, unsigned int
> >  		return btrfs_ioctl_get_subvol_rootref(file, argp);
> >  	case BTRFS_IOC_INO_LOOKUP_USER:
> >  		return btrfs_ioctl_ino_lookup_user(file, argp);
> > +	case BTRFS_IOC_ALLOC_CHUNK:
> > +		return btrfs_ioctl_alloc_chunk(file, argp);
> >  	}
> >  
> >  	return -ENOTTY;
> > diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> > index c195896d478f..3a6474c34ad0 100644
> > --- a/include/uapi/linux/btrfs.h
> > +++ b/include/uapi/linux/btrfs.h
> > @@ -943,5 +943,6 @@ enum btrfs_err_code {
> >  				struct btrfs_ioctl_get_subvol_rootref_args)
> >  #define BTRFS_IOC_INO_LOOKUP_USER _IOWR(BTRFS_IOCTL_MAGIC, 62, \
> >  				struct btrfs_ioctl_ino_lookup_user_args)
> > +#define BTRFS_IOC_ALLOC_CHUNK _IOR(BTRFS_IOCTL_MAGIC, 63, __u64)
> >  
> >  #endif /* _UAPI_LINUX_BTRFS_H */
> > 
> 




-- 
Hugo Mills             | We don't just borrow words; on occasion, English has
hugo@... carfax.org.uk | pursued other languages down alleyways to beat them
http://carfax.org.uk/  | unconscious and rifle their pockets for new
PGP: E2AB1DE4          | vocabulary.                           James D. Nicoll

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]