[Devel] [PATCH RH9] dm-ploop: port the standby mode feature.
Kui Liu
Kui.Liu at acronis.com
Thu Oct 27 12:03:56 MSK 2022
-----Original Message-----
From: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
Date: Thursday, 27 October 2022 at 4:28 PM
To: Kui Liu <Kui.Liu at acronis.com>, Konstantin Khorenko <khorenko at virtuozzo.com>, Alexey Kuznetsov <kuznet at acronis.com>, "Denis V. Lunev" <den at virtuozzo.com>
Cc: Devel <devel at openvz.org>
Subject: Re: [Devel] [PATCH RH9] dm-ploop: port the standby mode feature.
On 26.10.22 17:42, Kui Liu wrote:
>
>
> -----Original Message-----
> From: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> Date: Wednesday, 26 October 2022 at 9:32 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 26.10.22 16:15, Kui Liu wrote:
> >
> >
> > -----Original Message-----
> > From: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> > Date: Wednesday, 26 October 2022 at 5:14 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.
> >
> > Hi,
> >
> > On 21.10.22 16:28, Kui Liu wrote:
> > >
> > >
> > > -----Original Message-----
> > > From: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> > > Date: Friday, 21 October 2022 at 7:35 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 21.10.22 14:28, Kui Liu wrote:
> > > > > > 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 ?
> > > > > >
> > > > > > [LIU]: Since the static key is a global option, I also think the second approach is better.
> > > > > > In case of nodes with vStorage + ploop + iSCSI setup, we just need to enable the static
> > > > > > key early in system init or iSCSI service init. I don't think it is necessary to implement both.
> > > > >
> > > > > You are right. It will need at least one ploop device created to issue a
> > > > > device mapper command to enable standby. Would a module parameter work?
> > > > >
> > > > > [LIU] Yes, I think it is better to make the static key a module parameter.
> > > >
> > > > This rised a question - how and who will pass the module parameter when
> > > > required ? How we will handle the existin users ? What do you think ?
> > > >
> > > > [LIU]: Like add a conf file ploop.conf in /etc/modprobe.d or lib/modeprobe.d? This conf file can be shipped with the release image, or installed later. Are you referring to VZ7 as existing users? I saw you also made the changes to VZ7, but I'm not sure whether it is necessary to backport these changes to VZ7.
> > >
> > > Yes, VZ7 users - there are issues related to it so trying to fix them.
> > > the modprobe.d would do but do you have a package that can install the
> > > conf tied to the iSCSI target, so it gets installed only if the iSCSI is
> > > used ?
> > >
> > > [LIU]: Yes, we do build the our own SCST package, so we can install the conf from there.
> >
> > After discussion turns out module_param is not an option - package is
> > always installed so it will enable the key always which is not what we want.
> >
> > To solve this we can
> > struct static_key ploop_standby_check = STATIC_KEY_INIT_FALSE;
> > +EXPORT_SYMBOL(ploop_standby_check)
> >
> > and the module can enable it at use or init time.
> >
> > static int scst_vstor_pr_init(struct scst_device *dev, const char
> > *cl_dev_id)
> >
> > looks like the place to enable the static key and set the _en flag on
> > the queue.
> >
> > _en flag is a MUST static key is a SHOULD - where do you think is the
> > best place to implement them in the scst code?
> >
> > [LIU]: This would introduce explicit dependency between SCST and ploop module, which I feel is inappropriate .
> > Can we instead export the static key via a sysfs file, say /sys/kernel/ploop/ploop_standby_check?
>
> We can export it via sysfs or proc but who and how will enable it thru
> there ?
>
> [LIU] Well, I think we can enable it in Pre-/ On- / Post- start of vstorage-target-manager.service.
> The point is once it is exported to userspace, when and who to enable it shouldn't be a big issue.
Since QUEUE_FLAG_STANDBY_EN will make the scst module depend on the
kernel. It would be easier to leave setting in the kernel. And not make
it create userspace dependencies which may cause trouble when upgrading.
The key can be moved in core to avoid depending on another module.
--
Regards,
Alexander Atanasov
--- scst/src/scst_vstor.c.orig 2022-10-26 13:55:26.544317020 +0300
+++ scst/src/scst_vstor.c 2022-10-27 11:15:07.568776117 +0300
@@ -886,7 +886,14 @@ static int scst_vstor_pr_init(struct scs
dev->pr_vstor = pr_vstor;
pr_vstor->pr_dirty = 1;
res = 0;
-
+#ifdef QUEUE_FLAG_STANDBY_EN
+ if (dev->scsi_dev) {
+ extern struct static_key ploop_standby_check;
+ queue_flag_set(QUEUE_FLAG_STANDBY_EN, dev->scsi_dev->request_queue);
+ static_key_slow_inc(&ploop_standby_check);
+ PRINT_INFO("Enabled:ploop standby check");
+ }
+#endif
out:
return res;
}
[LIU]: OK, then move the key in core, I'll modify SCST code to enable it, though not in scst_vstor_pr_init().
More information about the Devel
mailing list