The 12/09/2020 17:36, Mike Snitzer wrote:
> On Wed, Dec 09 2020 at 8:01am -0500,
> Sergei Shtepa <sergei.shtepa@xxxxxxxxx> wrote:
>
> > This sample demonstrates how to use blk_interposer.
> > It show how to properly connect the interposer module to kernel,
> > and perform the simplest monitoring of the number of bio.
> >
> > Signed-off-by: Sergei Shtepa <sergei.shtepa@xxxxxxxxx>
> > ---
> > samples/blk_interposer/Makefile | 2 +
> > samples/blk_interposer/blk-interposer.c | 276 ++++++++++++++++++++++++
> > 2 files changed, 278 insertions(+)
> > create mode 100644 samples/blk_interposer/Makefile
> > create mode 100644 samples/blk_interposer/blk-interposer.c
> >
> > diff --git a/samples/blk_interposer/Makefile b/samples/blk_interposer/Makefile
> > new file mode 100644
> > index 000000000000..b11aefde2b1c
> > --- /dev/null
> > +++ b/samples/blk_interposer/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_SAMPLE_BLK_INTERPOSER) += blk-interposer.o
> > diff --git a/samples/blk_interposer/blk-interposer.c b/samples/blk_interposer/blk-interposer.c
> > new file mode 100644
> > index 000000000000..92b4c1fcf8f7
> > --- /dev/null
> > +++ b/samples/blk_interposer/blk-interposer.c
> > @@ -0,0 +1,276 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Block layer interposer allow to interpose bio requests from kernel module.
> > + * This allows you to monitor requests, modify requests, add new request,
> > + * or even redirect requests to another devices.
> > + *
> > + * This sample demonstrates how to use blk_interposer.
> > + * It show how to properly connect the interposer module to kernel,
> > + * and perform the simplest monitoring of the number of bio.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/errno.h>
> > +#include <linux/blkdev.h>
> > +#include <linux/genhd.h>
> > +#include <linux/blk-mq.h>
> > +
> > +int device_major = 8;
> > +int device_minor;
> > +int fmode = FMODE_READ | FMODE_WRITE;
> > +
> > +/*
> > + * Each interposer must have a common part in the form of the blk_interposer structure,
> > + * as well as its own unique data.
> > + */
> > +struct my_interposer {
> > + /*
> > + * Common part of block device interposer.
> > + */
> > + struct blk_interposer interposer;
> > + /*
> > + * Specific part for our interposer data.
> > + */
> > + atomic_t counter;
> > +};
> > +
> > +struct my_interposer my_ip;
> > +
> > +/**
> > + * blk_interposer_attach - Attach interposer to disk
> > + * @disk: target disk
> > + * @interposer: block device interposer
> > + */
> > +static int blk_interposer_attach(struct gendisk *disk, struct blk_interposer *interposer)
> > +{
> > + int ret = 0;
> > +
> > + /*
> > + * Stop disks queue processing.
> > + */
> > + blk_mq_freeze_queue(disk->queue);
> > + blk_mq_quiesce_queue(disk->queue);
> > +
> > + /*
> > + * Check if the interposer is already busy.
> > + * The interposer will only connect if it is not busy.
> > + */
> > + if (blk_has_interposer(disk)) {
> > + pr_info("The interposer is already busy.\n");
> > + ret = -EBUSY;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Attach the interposer.
> > + */
> > + disk->interposer = interposer;
> > + /*
> > + * And while the queue is stopped, we can do something specific for our module.
> > + */
> > + pr_info("Block device interposer attached successfully.\n");
> > +
> > +out:
> > + /*
> > + * Resume disks queue processing
> > + */
> > + blk_mq_unquiesce_queue(disk->queue);
> > + blk_mq_unfreeze_queue(disk->queue);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * blk_interposer_detach - Detach interposer from disk
> > + * @disk: target disk
> > + * @interposer: block device interposer
> > + */
> > +static int blk_interposer_detach(struct gendisk *disk, struct blk_interposer *interposer)
> > +{
> > + int ret = 0;
> > +
> > + if (WARN_ON(!disk))
> > + return -EINVAL;
> > +
> > + /*
> > + * Stop disks queue processing.
> > + */
> > + blk_mq_freeze_queue(disk->queue);
> > + blk_mq_quiesce_queue(disk->queue);
> > +
> > + /*
> > + * Check if the interposer is still available.
> > + */
> > + if (!disk->interposer) {
> > + pr_info("The interposer is not available.\n");
> > + return -ENOENT;
> > + goto out;
> > + }
> > + /*
> > + * Check if it is really our interposer.
> > + */
> > + if (disk->interposer->ip_submit_bio != interposer->ip_submit_bio) {
> > + pr_info("The interposer found is not ours.\n");
> > + return -EPERM;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Detach interposer.
> > + */
> > + disk->interposer = NULL;
> > + /*
> > + * And while the queue is stopped, we can do something specific for our module.
> > + */
> > + pr_info("Block device interposer detached successfully.\n");
> > +
> > +out:
> > + /*
> > + * Resume disks queue processing.
> > + */
> > + blk_mq_unquiesce_queue(disk->queue);
> > + blk_mq_unfreeze_queue(disk->queue);
> > +
> > + return ret;
> > +}
>
> This attach and detach code needs to be elevated out of samples so that
> any future consumer of blk_interposer doesn't reinvent it. It is far
> too fundamental.
>
> The way you've proposed this be merged is very much unacceptable.
>
> Nacked-by: Mike Snitzer <snitzer@xxxxxxxxxx>
>
Yes, but on the other hand, while the queue is suspended, the module can perform
some other actions specific to it.
And since the functions blk_mq_freeze_queue(), blk_mq_quiesce_queue(),
blk_mq_unquiesce_queue() and blk_mq_unfreeze_queue() are public,
the module creator can implement its module connection functionality regardless of
whether we make the functions blk_interposer_attach() and blk_interposer_detach()
in the kernel or not.
I'll think about it and try to come up with a better solution.
--
Sergei Shtepa
Veeam Software developer.
