Message-ID: <CAG2S0o__jLV0gh=O_14e5LTQmVTPDAHZ-o=FAMv5YuGgjEDaZw@mail.gmail.com>
Date:   Thu, 21 Apr 2022 15:39:20 +0200
From:   Andrea Tomassetti <andrea.tomassetti@...o.com>
To:     CAG2S0o-yjcc=HGVhZ-YfukT10+US45TemykFwETdgPRbJHLyqw@...l.gmail.com
Cc:     Coly Li <colyli@...e.de>,
        Kent Overstreet <kent.overstreet@...il.com>,
        Zhang Zhen <zhangzhen.email@...il.com>,
        linux-bcache@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] bcache: Use bcache without formatting existing device

ping...  any comments will be really appreciated
And with *any* I really mean *any*.
If it's not of any interest, I will just stop sending pings :)

Because it is my first attempt to send a patch, any feedback will be
really helpful.

Thank you very much,
Andrea

On Mon, Mar 28, 2022 at 1:37 PM Andrea Tomassetti
<andrea.tomassetti@...o.com> wrote:
>
> v4: Simplify and make more consistent `dc->sb_disk != NULL` check
>
> v3: fix build warning reported by kernel test robot
>
> >> drivers/md/bcache/control.c:18:6: warning: no
>    previous prototype for function 'bch_service_ioctl_ctrl'
>    [-Wmissing-prototypes]
>    long bch_service_ioctl_ctrl(struct file *filp, unsigned int cmd,
>         ^
>    drivers/md/bcache/control.c:18:1: note: declare 'static' if the
>    function is not intended to be used outside of this translation unit
>    long bch_service_ioctl_ctrl(struct file *filp, unsigned int cmd,
>    ^
>    static
>    1 warning generated.
>
> v2: Fixed small typos
>
> Introducing a bcache control device (/dev/bcache_ctrl)
> that allows communicating to the driver from user space
> via IOCTL. The only IOCTL commands currently implemented,
> receives a struct cache_sb and uses it to register the
> backing device.
>
> Signed-off-by: Andrea Tomassetti <andrea.tomassetti@...o.com>
> ---
> Hi all,
> At Devo we started to think of using bcache in our production systems
> to boost performance. But, at the very beginning, we were faced with
> one annoying issue, at least for our use-case: bcache needs the backing
> devices to be formatted before being able to use them. What's really
> needed is just to wipe any FS signature out of them. This is definitely
> not an option we will consider in our production systems because we would
> need to move hundreds of terabytes of data.
>
> To circumvent the "formatting" problem, in the past weeks I worked on some
> modifications to the actual bcache module. In particular, I added a bcache
> control device (exported to /dev/bcache_ctrl) that allows communicating to
> the driver from userspace via IOCTL. One of the IOCTL commands that I
> implemented receives a struct cache_sb and uses it to register the backing
> device. The modifications are really small and retro compatible. To then
> re-create the same configuration after every boot (because the backing
> devices now do not present the bcache super block anymore) I created an
> udev rule that invokes a python script that will re-create the same
> scenario based on a yaml configuration file.
>
>  drivers/md/bcache/Makefile      |   2 +-
>  drivers/md/bcache/control.c     | 117 ++++++++++++++++++++++++++++++++
>  drivers/md/bcache/control.h     |  12 ++++
>  drivers/md/bcache/ioctl_codes.h |  19 ++++++
>  drivers/md/bcache/super.c       |  50 +++++++++++---
>  5 files changed, 189 insertions(+), 11 deletions(-)
>  create mode 100644 drivers/md/bcache/control.c
>  create mode 100644 drivers/md/bcache/control.h
>  create mode 100644 drivers/md/bcache/ioctl_codes.h
>
> diff --git a/drivers/md/bcache/Makefile b/drivers/md/bcache/Makefile
> index 5b87e59676b8..46ed41baed7a 100644
> --- a/drivers/md/bcache/Makefile
> +++ b/drivers/md/bcache/Makefile
> @@ -4,4 +4,4 @@ obj-$(CONFIG_BCACHE)    += bcache.o
>
>  bcache-y               := alloc.o bset.o btree.o closure.o debug.o extents.o\
>         io.o journal.o movinggc.o request.o stats.o super.o sysfs.o trace.o\
> -       util.o writeback.o features.o
> +       util.o writeback.o features.o control.o
> diff --git a/drivers/md/bcache/control.c b/drivers/md/bcache/control.c
> new file mode 100644
> index 000000000000..69b5e554d093
> --- /dev/null
> +++ b/drivers/md/bcache/control.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/cdev.h>
> +#include <linux/fs.h>
> +#include <linux/vmalloc.h>
> +
> +#include "control.h"
> +
> +struct bch_ctrl_device {
> +       struct cdev cdev;
> +       struct class *class;
> +       dev_t dev;
> +};
> +
> +static struct bch_ctrl_device _control_device;
> +
> +/* this handles IOCTL for /dev/bcache_ctrl */
> +/*********************************************/
> +static long bch_service_ioctl_ctrl(struct file *filp, unsigned int cmd,
> +               unsigned long arg)
> +{
> +       int retval = 0;
> +
> +       if (_IOC_TYPE(cmd) != BCH_IOCTL_MAGIC)
> +               return -EINVAL;
> +
> +       if (!capable(CAP_SYS_ADMIN)) {
> +               /* Must be root to issue ioctls */
> +               return -EPERM;
> +       }
> +
> +       switch (cmd) {
> +       case BCH_IOCTL_REGISTER_DEVICE: {
> +               struct bch_register_device *cmd_info;
> +
> +               cmd_info = vmalloc(sizeof(struct bch_register_device));
> +               if (!cmd_info)
> +                       return -ENOMEM;
> +
> +               if (copy_from_user(cmd_info, (void __user *)arg,
> +                               sizeof(struct bch_register_device))) {
> +                       pr_err("Cannot copy cmd info from user space\n");
> +                       vfree(cmd_info);
> +                       return -EINVAL;
> +               }
> +
> +               retval = register_bcache_ioctl(cmd_info);
> +
> +               vfree(cmd_info);
> +               return retval;
> +       }
> +
> +       default:
> +               return -EINVAL;
> +       }
> +}
> +
> +static const struct file_operations _ctrl_dev_fops = {
> +       .owner = THIS_MODULE,
> +       .unlocked_ioctl = bch_service_ioctl_ctrl
> +};
> +
> +int __init bch_ctrl_device_init(void)
> +{
> +       struct bch_ctrl_device *ctrl = &_control_device;
> +       struct device *device;
> +       int result = 0;
> +
> +       result = alloc_chrdev_region(&ctrl->dev, 0, 1, "bcache");
> +       if (result) {
> +               pr_err("Cannot allocate control chrdev number.\n");
> +               goto error_alloc_chrdev_region;
> +       }
> +
> +       cdev_init(&ctrl->cdev, &_ctrl_dev_fops);
> +
> +       result = cdev_add(&ctrl->cdev, ctrl->dev, 1);
> +       if (result) {
> +               pr_err("Cannot add control chrdev.\n");
> +               goto error_cdev_add;
> +       }
> +
> +       ctrl->class = class_create(THIS_MODULE, "bcache");
> +       if (IS_ERR(ctrl->class)) {
> +               pr_err("Cannot create control chrdev class.\n");
> +               result = PTR_ERR(ctrl->class);
> +               goto error_class_create;
> +       }
> +
> +       device = device_create(ctrl->class, NULL, ctrl->dev, NULL,
> +                       "bcache_ctrl");
> +       if (IS_ERR(device)) {
> +               pr_err("Cannot create control chrdev.\n");
> +               result = PTR_ERR(device);
> +               goto error_device_create;
> +       }
> +
> +       return result;
> +
> +error_device_create:
> +       class_destroy(ctrl->class);
> +error_class_create:
> +       cdev_del(&ctrl->cdev);
> +error_cdev_add:
> +       unregister_chrdev_region(ctrl->dev, 1);
> +error_alloc_chrdev_region:
> +       return result;
> +}
> +
> +void bch_ctrl_device_deinit(void)
> +{
> +       struct bch_ctrl_device *ctrl = &_control_device;
> +
> +       device_destroy(ctrl->class, ctrl->dev);
> +       class_destroy(ctrl->class);
> +       cdev_del(&ctrl->cdev);
> +       unregister_chrdev_region(ctrl->dev, 1);
> +}
> diff --git a/drivers/md/bcache/control.h b/drivers/md/bcache/control.h
> new file mode 100644
> index 000000000000..3e4273db02a3
> --- /dev/null
> +++ b/drivers/md/bcache/control.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __BCACHE_CONTROL_H__
> +#define __BCACHE_CONTROL_H__
> +
> +#include "ioctl_codes.h"
> +
> +int __init bch_ctrl_device_init(void);
> +void bch_ctrl_device_deinit(void);
> +
> +ssize_t register_bcache_ioctl(struct bch_register_device *brd);
> +
> +#endif
> diff --git a/drivers/md/bcache/ioctl_codes.h b/drivers/md/bcache/ioctl_codes.h
> new file mode 100644
> index 000000000000..b004d60c29ff
> --- /dev/null
> +++ b/drivers/md/bcache/ioctl_codes.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __BCACHE_IOCTL_CODES_H__
> +#define __BCACHE_IOCTL_CODES_H__
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +struct bch_register_device {
> +       const char *dev_name;
> +       size_t size;
> +       struct cache_sb *sb;
> +};
> +
> +#define BCH_IOCTL_MAGIC (0xBC)
> +
> +/* Register a new backing device */
> +#define BCH_IOCTL_REGISTER_DEVICE      _IOWR(BCH_IOCTL_MAGIC, 1, struct bch_register_device)
> +
> +#endif
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 140f35dc0c45..339a11d00fef 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -14,6 +14,7 @@
>  #include "request.h"
>  #include "writeback.h"
>  #include "features.h"
> +#include "control.h"
>
>  #include <linux/blkdev.h>
>  #include <linux/pagemap.h>
> @@ -340,6 +341,9 @@ void bch_write_bdev_super(struct cached_dev *dc, struct closure *parent)
>         struct closure *cl = &dc->sb_write;
>         struct bio *bio = &dc->sb_bio;
>
> +       if (!dc->sb_disk)
> +               return;
> +
>         down(&dc->sb_write_mutex);
>         closure_init(cl, parent);
>
> @@ -2403,14 +2407,14 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
>
>  /* Global interfaces/init */
>
> -static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> +static ssize_t register_bcache_sysfs(struct kobject *k, struct kobj_attribute *attr,
>                                const char *buffer, size_t size);
>  static ssize_t bch_pending_bdevs_cleanup(struct kobject *k,
>                                          struct kobj_attribute *attr,
>                                          const char *buffer, size_t size);
>
> -kobj_attribute_write(register,         register_bcache);
> -kobj_attribute_write(register_quiet,   register_bcache);
> +kobj_attribute_write(register,         register_bcache_sysfs);
> +kobj_attribute_write(register_quiet,   register_bcache_sysfs);
>  kobj_attribute_write(pendings_cleanup, bch_pending_bdevs_cleanup);
>
>  static bool bch_is_open_backing(dev_t dev)
> @@ -2465,7 +2469,8 @@ static void register_bdev_worker(struct work_struct *work)
>         dc = kzalloc(sizeof(*dc), GFP_KERNEL);
>         if (!dc) {
>                 fail = true;
> -               put_page(virt_to_page(args->sb_disk));
> +               if (args->sb_disk)
> +                       put_page(virt_to_page(args->sb_disk));
>                 blkdev_put(args->bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
>                 goto out;
>         }
> @@ -2495,7 +2500,8 @@ static void register_cache_worker(struct work_struct *work)
>         ca = kzalloc(sizeof(*ca), GFP_KERNEL);
>         if (!ca) {
>                 fail = true;
> -               put_page(virt_to_page(args->sb_disk));
> +               if (args->sb_disk)
> +                       put_page(virt_to_page(args->sb_disk));
>                 blkdev_put(args->bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
>                 goto out;
>         }
> @@ -2525,7 +2531,7 @@ static void register_device_async(struct async_reg_args *args)
>         queue_delayed_work(system_wq, &args->reg_work, 10);
>  }
>
> -static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
> +static ssize_t register_bcache_common(void *k, struct kobj_attribute *attr,
>                                const char *buffer, size_t size)
>  {
>         const char *err;
> @@ -2587,9 +2593,14 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
>         if (set_blocksize(bdev, 4096))
>                 goto out_blkdev_put;
>
> -       err = read_super(sb, bdev, &sb_disk);
> -       if (err)
> -               goto out_blkdev_put;
> +       if (!k) {
> +               err = read_super(sb, bdev, &sb_disk);
> +               if (err)
> +                       goto out_blkdev_put;
> +       } else {
> +               sb_disk =  NULL;
> +               memcpy(sb, (struct cache_sb *)k, sizeof(struct cache_sb));
> +       }
>
>         err = "failed to register device";
>
> @@ -2651,7 +2662,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
>         return size;
>
>  out_put_sb_page:
> -       put_page(virt_to_page(sb_disk));
> +       if (!k)
> +               put_page(virt_to_page(sb_disk));
>  out_blkdev_put:
>         blkdev_put(bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
>  out_free_sb:
> @@ -2666,6 +2678,18 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
>         return ret;
>  }
>
> +static ssize_t register_bcache_sysfs(struct kobject *k, struct kobj_attribute *attr,
> +                              const char *buffer, size_t size)
> +{
> +       return register_bcache_common(NULL, attr, buffer, size);
> +}
> +
> +ssize_t register_bcache_ioctl(struct bch_register_device *brd)
> +{
> +       return register_bcache_common((void *)brd->sb, NULL, brd->dev_name, brd->size);
> +}
> +
> +
>
>  struct pdev {
>         struct list_head list;
> @@ -2819,6 +2843,7 @@ static void bcache_exit(void)
>  {
>         bch_debug_exit();
>         bch_request_exit();
> +       bch_ctrl_device_deinit();
>         if (bcache_kobj)
>                 kobject_put(bcache_kobj);
>         if (bcache_wq)
> @@ -2918,6 +2943,11 @@ static int __init bcache_init(void)
>         bch_debug_init();
>         closure_debug_init();
>
> +       if (bch_ctrl_device_init()) {
> +               pr_err("Cannot initialize control device\n");
> +               goto err;
> +       }
> +
>         bcache_is_reboot = false;
>
>         return 0;
> --
> 2.25.1
>

-- 







The contents of this email are confidential. If the reader of this 
message is not the intended recipient, you are hereby notified that any 
dissemination, distribution or copying of this communication is strictly 
prohibited. If you have received this communication in error, please notify 
us immediately by replying to this message and deleting it from your 
computer. Thank you. Devo, Inc; arco@...o.com <mailto:arco@...o.com>; 
255 
Main St Suite 702, Cambridge MA USA 02142 
Calle Estébanez Calderón 3-5, 
5th Floor Madrid, Spain 28020

Powered by blists - more mailing lists