[Devel] [PATCH v4] Only perform standby check when enabled on the target.

[Konstantin Khorenko]

0. Andrey, please review as well.

Sasha,

1. Please describe the whole feature in the commit message.
    Why it should be done, what problem does it solve,
    in which configuration should "standby mode" feature be enabled.

2. feature interface from the kernel point of view.
    You have a static key which enables checks globally and
    a per-queue flag which says "this particular queue (read - ploop)"
    should have "standby feature" enabled.

3. Who and how (on which stage) is going to enable the global static key.
4. Who and how is going to enable "standby feature" for a particular queue (ploop),
    i mean to set the QUEUE_FLAG_STANDBY_EN queue flag.


On 28.10.2022 16:25, Alexander Atanasov wrote:
> Add static key enabled via module parameter

module parameter? Don't see it in the patch.

 > when running on mixed mode systems

What does it mean - "mixed mode systems"?


> and standby enable flag on the request queue.
> Using the flags perform the check only if required by
> the target device. Avoid unwanted side effects and performance impact.
> 
> Show the state of new flags at /sys/block/*/queue/standby, so we can
> check them when debugging.
> 
> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> ---
>   block/blk-sysfs.c             | 15 +++++++++++++++
>   drivers/block/ploop/dev.c     | 10 +++++++---
>   drivers/block/ploop/io_kaio.c | 11 ++++++++---
>   include/linux/blkdev.h        |  2 ++
>   kernel/ve/ve.c                |  4 ++++
>   5 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 7aebab0ced19..8ec90e32a2d3 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -357,6 +357,15 @@ static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page,
>   	wbt_update_limits(q->rq_wb);
>   	return count;
>   }
> +
> +static ssize_t queue_standby_show(struct request_queue *q, char *page)
> +{
> +	if (!blk_queue_standby_en(q))
> +		return sprintf(page, "not supported\n");
> +	return sprintf(page, "%s\n",
> +		blk_queue_standby(q) ? "on" : "off");

i've checked - all other queue attrs are numeric (except for "scheduler").
Why to use strings "on"/"off" here, not 1/0?

"not supported" may be a string, it's OK, but i'd suggest to write "feature disabled" may be, not to 
confuse someone with "why my kernel does not support this feature?" while the feature is there, but 
just not enabled to be checked.
But this is minor, i'm fine with "not supported" as well.

> +}
> +
>   static struct queue_sysfs_entry queue_requests_entry = {
>   	.attr = {.name = "nr_requests", .mode = S_IRUGO | S_IWUSR },
>   	.show = queue_requests_show,
> @@ -488,6 +497,11 @@ static struct queue_sysfs_entry queue_wb_lat_entry = {
>   	.store = queue_wb_lat_store,
>   };
>   
> +static struct queue_sysfs_entry queue_standby_flags = {

Please name it "queue_standby_entry" to match other neighbor entries naming.

> +	.attr = {.name = "standby", .mode = S_IRUGO },
> +	.show = queue_standby_show,
> +};
> +
>   static struct attribute *default_attrs[] = {
>   	&queue_requests_entry.attr,
>   	&queue_ra_entry.attr,
> @@ -513,6 +527,7 @@ static struct attribute *default_attrs[] = {
>   	&queue_iostats_entry.attr,
>   	&queue_random_entry.attr,
>   	&queue_wb_lat_entry.attr,
> +	&queue_standby_flags,

+	&queue_standby_entry.attr,

>   	NULL,
>   };
>   
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index 06b72542633b..32f4a394e4b8 100644
> --- a/drivers/block/ploop/dev.c
> +++ b/drivers/block/ploop/dev.c
> @@ -76,6 +76,8 @@ static DEFINE_MUTEX(ploop_devices_mutex);
>   static LIST_HEAD(ploop_formats);
>   static DEFINE_MUTEX(ploop_formats_mutex);
>   
> +extern struct static_key ploop_standby_check;
> +
>   int ploop_register_format(struct ploop_delta_ops * ops)
>   {
>   	mutex_lock(&ploop_formats_mutex);
> @@ -985,9 +987,11 @@ static void ploop_make_request(struct request_queue *q, struct bio *bio)
>   	hd_struct_put(part);
>   	part_stat_unlock();
>   
> -	if (blk_queue_standby(plo->queue)) {
> -		BIO_ENDIO(q, bio, -EIO);
> -		return;
> +	if (static_key_false(&ploop_standby_check)) {
> +		if (blk_queue_standby(plo->queue)) {
> +			BIO_ENDIO(q, bio, -EIO);
> +			return;
> +		}
>   	}
>   
>   	if (unlikely(bio->bi_size == 0)) {
> diff --git a/drivers/block/ploop/io_kaio.c b/drivers/block/ploop/io_kaio.c
> index e24952703549..a0c28379eb7d 100644
> --- a/drivers/block/ploop/io_kaio.c
> +++ b/drivers/block/ploop/io_kaio.c
> @@ -23,6 +23,7 @@
>   
>   #define KAIO_MAX_PAGES_PER_REQ 32	  /* 128 KB */
>   
> +extern struct static_key ploop_standby_check;
>   /* This will be used as flag "ploop_kaio_open() succeeded" */
>   static struct extent_map_tree
>   {
> @@ -116,6 +117,9 @@ static void check_standby_mode(long res, struct ploop_device *plo) {
>   	struct request_queue *q = plo->queue;
>   	int prev;
>   
> +	if (!blk_queue_standby_en(q))
> +		return;
> +
>   	/* move to standby if delta lease was stolen or mount is gone */
>   	if (res != -EBUSY && res != -ENOTCONN && res != -EIO) {
>   		return;
> @@ -146,8 +150,8 @@ static void kaio_rw_aio_complete(u64 data, long res)
>   		bio_list_for_each(b, &preq->bl)
>   			printk(" bio=%p: bi_sector=%ld bi_size=%d\n",
>   			       b, b->bi_sector, b->bi_size);
> -
> -		check_standby_mode(res, preq->plo);
> +		if (static_key_false(&ploop_standby_check))
> +			check_standby_mode(res, preq->plo);
>   		PLOOP_REQ_SET_ERROR(preq, res);
>   	}
>   
> @@ -560,7 +564,8 @@ static int kaio_fsync_thread(void * data)
>   				       "on ploop%d)\n",
>   				       err, io->files.inode->i_ino,
>   				       io2level(io), plo->index);
> -				check_standby_mode(err, plo);
> +				if (static_key_false(&ploop_standby_check))
> +					check_standby_mode(err, plo);
>   				PLOOP_REQ_SET_ERROR(preq, -EIO);
>   			} else if (preq->req_rw & REQ_FLUSH) {
>   				BUG_ON(!preq->req_size);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 4e2970ff97b2..fb6daf0eef7a 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -643,6 +643,7 @@ struct request_queue {
>   #define QUEUE_FLAG_SCSI_PASSTHROUGH 30	/* queue supports SCSI commands */
>   
>   #define QUEUE_FLAG_STANDBY     31	/* unable to handle read/write requests */
> +#define QUEUE_FLAG_STANDBY_EN  32	/* enable standby queue flag */
>   
>   #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
>   				 (1 << QUEUE_FLAG_STACKABLE)	|	\
> @@ -739,6 +740,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
>   #define blk_queue_scsi_passthrough(q)	\
>   	test_bit(QUEUE_FLAG_SCSI_PASSTHROUGH, &(q)->queue_flags)
>   #define blk_queue_standby(q)	test_bit(QUEUE_FLAG_STANDBY, &(q)->queue_flags)
> +#define blk_queue_standby_en(q)	test_bit(QUEUE_FLAG_STANDBY_EN, &(q)->queue_flags)
>   
>   extern int blk_set_preempt_only(struct request_queue *q);
>   extern void blk_clear_preempt_only(struct request_queue *q);
> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
> index 68c3e91d60c1..4fa8969302c2 100644
> --- a/kernel/ve/ve.c
> +++ b/kernel/ve/ve.c
> @@ -76,6 +76,10 @@ MODULE_PARM_DESC(ve_allow_ioctl_fitrim,
>   		 "Allow ioctl(FITRIM) from inside VE. Only ext4 is supported now");
>   EXPORT_SYMBOL(ve_allow_ioctl_fitrim);
>   
> +/* to avoid creating inter module dependencies */

Please, extend the comment with the description of this static key nature.
What is it intended for.

> +struct static_key ploop_standby_check = STATIC_KEY_INIT_FALSE;
> +EXPORT_SYMBOL(ploop_standby_check);
> +
>   static DEFINE_PER_CPU(struct kstat_lat_pcpu_snap_struct, ve0_lat_stats);
>   
>   struct ve_struct ve0 = {
> 
> base-commit: 726ab1f1d5f51619964a0626187f8aee970d6920