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

[Mike Snitzer]

On Thu, Dec 10 2020 at 10:54am -0500,
Sergei Shtepa <sergei.shtepa@xxxxxxxxx> wrote:

> 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.

This practice of sprinkling code in samples/ subdir and getting a
minimalist hook into the kernel (with no actual in-kernel consumer) is
_not_ how we do things.

I'm not well-versed on the samples/ subdir nor when it came into
existence; but it should die in a fire!

Mike