Re: [PATCH 2/3] block: blk_interposer - sample

[Sergei Shtepa <sergei.shtepa@veeam.com>]

The 12/09/2020 17:36, Mike Snitzer wrote:
> On Wed, Dec 09 2020 at  8:01am -0500,
> Sergei Shtepa <sergei.shtepa@veeam.com> 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@veeam.com>
> > ---
> >  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@redhat.com>
> 
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.