Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Dec 2020 09:36:06 -0500
From:   Mike Snitzer <snitzer@redhat.com>
To:     Sergei Shtepa <sergei.shtepa@veeam.com>
Cc:     axboe@kernel.dk, johannes.thumshirn@wdc.com, koct9i@gmail.com,
        ming.lei@redhat.com, hare@suse.de, josef@toxicpanda.com,
        steve@sk2.org, linux-block@vger.kernel.org,
        linux-kernel@vger.kernel.org, pavel.tide@veeam.com
Subject: Re: [PATCH 2/3] block: blk_interposer - sample
Message-ID: <20201209143606.GA494@redhat.com>
References: <1607518911-30692-1-git-send-email-sergei.shtepa@veeam.com>
 <1607518911-30692-3-git-send-email-sergei.shtepa@veeam.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1607518911-30692-3-git-send-email-sergei.shtepa@veeam.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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>