On 18.10.22 15:47, Kui Liu wrote:
Hello,
-----Original Message-----
From: Alexander Atanasov <alexander.atana...@virtuozzo.com>
Date: Tuesday, 18 October 2022 at 7:40 PM
To: Kui Liu <kui....@acronis.com>, Konstantin Khorenko <khore...@virtuozzo.com>,
Alexey Kuznetsov <kuz...@acronis.com>
Cc: Devel <devel@openvz.org>
Subject: Re: [Devel] [PATCH RH9] dm-ploop: port the standby mode feature.
On 18.10.22 13:57, Kui Liu wrote:
> Hello,
>
> Regarding the QUEUE_FLAG_STANDBY_EN flag, it is like what you added.
>
> However I find some problems with the code, see my comments below.
yes, i did a quick and dirty version just to get on something to talk
over. I will fix the inverted logic and the number. See bellow for
comments on the init.
> @@ -1869,6 +1897,12 @@ int ploop_clone_and_map(struct dm_target *ti,
> struct request *rq,
> struct ploop_rq *prq;
> struct pio *pio;
>
> +
> + if (static_branch_unlikely(&ploop_standby_check)) {
> + if (blk_queue_standby(ploop_blk_queue(ploop)))
> + return DM_MAPIO_KILL;
> + }
> +
> if (blk_rq_bytes(rq) && ploop_rq_valid(ploop, rq) < 0)
> return DM_MAPIO_KILL;
>
> diff --git a/drivers/md/dm-ploop-target.c
b/drivers/md/dm-ploop-target.c
> index 4f7dc36eee0c..dda0fe957c1f 100644
> --- a/drivers/md/dm-ploop-target.c
> +++ b/drivers/md/dm-ploop-target.c
> @@ -21,6 +21,7 @@
> #include <linux/uio.h>
> #include <linux/error-injection.h>
> #include "dm-ploop.h"
> +#include "dm-core.h"
>
> #define DM_MSG_PREFIX "ploop"
>
> @@ -32,6 +33,7 @@ MODULE_PARM_DESC(ignore_signature_disk_in_use,
> static struct kmem_cache *prq_cache;
> static struct kmem_cache *pio_cache;
> struct kmem_cache *cow_cache;
> +extern struct static_key ploop_standby_check;
>
> static void ploop_aio_do_completion(struct pio *pio)
> {
> @@ -406,6 +408,14 @@ static int ploop_ctr(struct dm_target *ti,
unsigned
> int argc, char **argv)
> ti->private = ploop;
> ploop->ti = ti;
>
> + if (blk_queue_standby_en(ploop_blk_queue(ploop))) {
> + blk_queue_flag_clear(QUEUE_FLAG_STANDBY,
ploop_blk_queue(ploop));
> + if (!static_key_enabled(&ploop_standby_check)) {
> + static_key_enable(&ploop_standby_check);
> + pr_info("ploop: standby mode check
enabled\n");
> + }
> + }
> +
>
> [LIU]: This may not work, because normally a ploop device is created first before
being attached to SCST, which means 'ploop_ctr' is called before QUEUE_FLAG_STANDBY_EN can be
set. We need to put " static_key_enable(&ploop_standby_check))" somewhere else.
I am not sure i undestand that correctly but dm-ploop maps the ploop
device over existing device. It first opens the files that are on the
existing device then passes the fds to dm-ploop. To do this the target
must be up and running. device mapper setups the table before calling
the ploop ctr. You already checked the queue lifecycle.
Is there any other precodition for the target to set the EN flag?
There is one more alternative to pass the stanby enable flag enable from
userspace .
[LIU]: well, you are right about the dm-ploop setup process. However SCST can
only set the EN flag after the ploop device is assigned to
SCST, before which the table could have been loaded. So if setup sequence is 1)
create ploop device -> 2) load table -> 3) assign ploop
device to SCST, then the outer if condition won't be true, hence not triggering
static_key_enable().
My concern is this should not be the only place where the static key can be
enabled. Now that there is one more alternative in user space
, above code is OK.
Ok - for the user space there are two options:
1. add an argument to enable at creation time
2. command standby_enable /if using the enable it is possible to
implement a disable command too/.
I prefer the second approach but let's hear other opinions too.
And yes we can possibly do both but is there a use case ?
>
> if (kstrtou32(argv[0], 10, &ploop->cluster_log) < 0) {
> ret = -EINVAL;
> ti->error = "could not parse cluster_log";
> diff --git a/drivers/md/dm-ploop.h b/drivers/md/dm-ploop.h
> index 5d953278a976..f7c21b0b1597 100644
> --- a/drivers/md/dm-ploop.h
> +++ b/drivers/md/dm-ploop.h
> @@ -230,6 +230,7 @@ struct ploop {
> struct timer_list enospc_timer;
> bool event_enospc;
> };
> +#define ploop_blk_queue(p) ((p)->ti->table->md->queue)
>
> struct ploop_rq {
> struct request *rq;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 8fcd753c70b0..8928161e5b1d 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -434,6 +434,8 @@ struct request_queue {
> #define QUEUE_FLAG_RQ_ALLOC_TIME 27 /* record
rq->alloc_time_ns */
> #define QUEUE_FLAG_HCTX_ACTIVE 28 /* at least one blk-mq
hctx is active */
> #define QUEUE_FLAG_NOWAIT 29 /* device supports NOWAIT
*/
> +#define QUEUE_FLAG_STANDBY 30 /* unable to handle
read/write
> requests */
> +#define QUEUE_FLAG_STANDBY_EN 30 /* enable standby queue
flag */
>
> [LIU]: Should use a different number for QUEUE_FLAG_STANDBY_EN.
yes, definitely
>
> #define QUEUE_FLAG_MQ_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) |
\
> (1 << QUEUE_FLAG_SAME_COMP) |
\
> @@ -480,6 +482,8 @@ bool blk_queue_flag_test_and_set(unsigned int
flag,
> struct request_queue *q);
> #define blk_queue_fua(q) test_bit(QUEUE_FLAG_FUA,
&(q)->queue_flags)
> #define blk_queue_registered(q)
test_bit(QUEUE_FLAG_REGISTERED,
> &(q)->queue_flags)
> #define blk_queue_nowait(q) test_bit(QUEUE_FLAG_NOWAIT,
&(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 void blk_set_pm_only(struct request_queue *q);
> extern void blk_clear_pm_only(struct request_queue *q);
>
>
>
--
Regards,
Alexander Atanasov
--
Regards,
Alexander Atanasov
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
