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

[PATCH][v2] 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>
---
v1->v2:
- I noticed last week when backporting this that btrfs_chunk_alloc doesn't
  figure out the rest of the flags needed for the type.  Use
  btrfs_force_chunk_alloc instead so that we get the raid settings for the alloc
  type we're using.

 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..891bf198d46a 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_force_chunk_alloc(trans, flags);
+	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][v2] btrfs: add an ioctl to force chunk allocation

[Holger Hoffstätte]

On 8/5/19 3:19 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.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> v1->v2:
> - I noticed last week when backporting this that btrfs_chunk_alloc doesn't
>    figure out the rest of the flags needed for the type.  Use
>    btrfs_force_chunk_alloc instead so that we get the raid settings for the alloc
>    type we're using.

Tested-by: Holger Hoffstätte <holger@applied-asynchrony.com>

Now works as intended - very nice, thanks!

-h

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

[David Sterba]

On Mon, Aug 05, 2019 at 04:10:39PM +0200, Holger Hoffstätte wrote:
> On 8/5/19 3:19 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.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> > v1->v2:
> > - I noticed last week when backporting this that btrfs_chunk_alloc doesn't
> >    figure out the rest of the flags needed for the type.  Use
> >    btrfs_force_chunk_alloc instead so that we get the raid settings for the alloc
> >    type we're using.
> 
> Tested-by: Holger Hoffstätte <holger@applied-asynchrony.com>
> 
> Now works as intended - very nice, thanks!

Tell me, are you interested in this ioctl because it was the missing bit
for testing or because the chunk allocator is so bad that you need a
command to make work?

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

[Holger Hoffstätte]

On 8/5/19 5:41 PM, David Sterba wrote:
> On Mon, Aug 05, 2019 at 04:10:39PM +0200, Holger Hoffstätte wrote:
>> On 8/5/19 3:19 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.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>> v1->v2:
>>> - I noticed last week when backporting this that btrfs_chunk_alloc doesn't
>>>     figure out the rest of the flags needed for the type.  Use
>>>     btrfs_force_chunk_alloc instead so that we get the raid settings for the alloc
>>>     type we're using.
>>
>> Tested-by: Holger Hoffstätte <holger@applied-asynchrony.com>
>>
>> Now works as intended - very nice, thanks!
> 
> Tell me, are you interested in this ioctl because it was the missing bit
> for testing or because the chunk allocator is so bad that you need a
> command to make work?

Mostly curiosity. "To make [it] work" would be a bit strong - btrfs works
fine by now. But yes, slightly over-allocating both metadata and data
(both by ~1-2 chunks) helps esp. when there is a mix of small and large
files, and to reduce fragmentation of large files. That should not be
surprising.

-h

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

[Qu Wenruo]

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



On 2019/8/5 下午9:19, 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.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> v1->v2:
> - I noticed last week when backporting this that btrfs_chunk_alloc doesn't
>   figure out the rest of the flags needed for the type.  Use
>   btrfs_force_chunk_alloc instead so that we get the raid settings for the alloc
>   type we're using.

Still not sure if it's worthy to add a debug only ioctl.

If sysfs is not good enough, maybe modify btrfs_force_chunk_alloc() to
do extra chunk allocation depending on a internal return value
overridable function?

At least for the later case, we can trigger empty chunk allocation with
BPF while without messing any interface.

Thanks,
Qu
> 
>  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..891bf198d46a 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_force_chunk_alloc(trans, flags);
> +	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][v2] btrfs: add an ioctl to force chunk allocation

[Josef Bacik]

On Mon, Aug 05, 2019 at 10:14:15PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/8/5 下午9:19, 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.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> > v1->v2:
> > - I noticed last week when backporting this that btrfs_chunk_alloc doesn't
> >   figure out the rest of the flags needed for the type.  Use
> >   btrfs_force_chunk_alloc instead so that we get the raid settings for the alloc
> >   type we're using.
> 
> Still not sure if it's worthy to add a debug only ioctl.
> 
> If sysfs is not good enough, maybe modify btrfs_force_chunk_alloc() to
> do extra chunk allocation depending on a internal return value
> overridable function?
> 
> At least for the later case, we can trigger empty chunk allocation with
> BPF while without messing any interface.

I hadn't thought of the sysfs route, I actually like that a lot better.  Once I
figure out why our debugfs stuff is broken I'll add this in there, that'll save
us a btrfs-progs command as well.  Thanks,

Josef

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

[Nikolay Borisov]

On 5.08.19 г. 16:19 ч., 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.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---

I'm against exposing internal interfaces of btrfs as a public ABI. This
patch is simple enough so that you can carry it in your private
debugging branches.

<snip>

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

[David Sterba]

On Mon, Aug 05, 2019 at 09:19:42AM -0400, 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.

For debugging and testing purposes that's possible to add, but certainly
not as a high-level command of 'btrfs filesystem'.

> +#define BTRFS_IOC_ALLOC_CHUNK _IOR(BTRFS_IOCTL_MAGIC, 63, __u64)

From interface POV, this is wrong on several levels.

* it addresses a single narrow usecase

* the parameters are non-extensible so when you're going to need one
  more u64 next week or month, it'll cost another ioctl, but this one
  has to stay forever

* ioctl for debugging is not always the best interface (sysfs was
  suggested)

It would help if you explain the 'sometimes handy' in more detail
otherwise I'm going to be suspecting the usecase is not to help
debugging but to paper over inefficient chunk allocator behaviour.
I hope I'm wrong on that one but user interest under the patch shows
otherwise.

Regarding the points above, the sysfs is IMO more suitable:

* single file representing a command to the filesystem is fine for a
  narrow usecase, ie.
  'echo metadata > /sys/fs/btrfs/UUID/debug/force_chunk_alloc'
  is acceptable for testing

* string-based commands are extensible, as long as we pass eg. key=value