[Devel] [PATCH RH9] dm-ploop: port the standby mode feature.
Alexander Atanasov
alexander.atanasov at virtuozzo.com
Tue Oct 18 16:12:42 MSK 2022
On 18.10.22 15:47, Kui Liu wrote:
> Hello,
>
> -----Original Message-----
> From: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> Date: Tuesday, 18 October 2022 at 7:40 PM
> To: Kui Liu <Kui.Liu at acronis.com>, Konstantin Khorenko <khorenko at virtuozzo.com>, Alexey Kuznetsov <kuznet at acronis.com>
> Cc: Devel <devel at 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
More information about the Devel
mailing list