Hi, all
How about we introduce another block queue flag, say QUEUE_FLAG_STANDBY_EN, to
indicate whether
the device support the standby mode. the flag bit shall be set by SCST when a
blockio device is created, if
the blockio device is ploop device, then it will turn on standby mode check in
ploop, if the blockio
device is not ploop device, it should have no impact at all.
As for ploop not being used by scst, the flag is always 0, so we can skip
standby mode check in ploop by checking
the flag at beginning of 'check_standby_mode()'.
How do you think of above implementation?
Regards,
Liu Kui
-----Original Message-----
From: Konstantin Khorenko <khore...@virtuozzo.com>
Date: Tuesday, 18 October 2022 at 1:57 AM
To: Kui Liu <kui....@acronis.com>, Alexey Kuznetsov <kuz...@acronis.com>
Cc: Devel <devel@openvz.org>, Alexander Mikhalitsyn
<alexander.mikhalit...@virtuozzo.com>, Alexander Atanasov
<alexander.atana...@virtuozzo.com>
Subject: Re: [Devel] [PATCH RH9] dm-ploop: port the standby mode feature.
Hi Liu, Alexey,
to summarize a bit:
In case we have ploops without iSCSI targets on top (but may be with
vStorage underneath), in case we
get these errors EBUSY/ENOTCONN/EIO we get stuck io on ploop (which reason
by the way is quite
difficult to detect) - and this is not we want to get.
There can be setups with a Node + vStorage + ploops with iSCSI ontop AND
ploops without iSCSI on top
on the same node,
thus i have to suggest a per-ploop flag, otherwise we will not distingiush
those ploop setups.
And we also do not want to have a performance degradation if our setup is
not
"vStorage + ploops with iSCSI ontop", so i suggest to put that per-ploop
checks under a static key and
enable this key, for example, on the first iSCSI over ploop creation.
Please suggest the implementation and what would be the trigger for static
key switch and how to mark
ploop devices with that flag,
from my point of view it might be either some kernel code trigger like scst
module load (for static
key switch) and iSCSI target creation in scst module (for marking ploop
devices), or may be we can
introduce a new ploop device construction userspace command line option
which will say "hey, this
ploop is gonna be used for iSCSI, please put the appropriate flag for the
device".
Thank you.
--
Best regards,
Konstantin Khorenko,
Virtuozzo Linux Kernel Team
On 14.10.2022 11:42, Alexander Atanasov wrote:
> Hello,
>
> On 14.10.22 8:57, Kui Liu wrote:
>>
>>
>> -----Original Message-----
>> From: Alexander Atanasov <alexander.atana...@virtuozzo.com>
>> Date: Thursday, 13 October 2022 at 11:26 PM
>> To: Kui Liu <kui....@acronis.com>, Konstantin Khorenko
<khore...@virtuozzo.com>, Alexey Kuznetsov <kuz...@acronis.com>
>> Cc: Devel <devel@openvz.org>, Alexander Mikhalitsyn
<alexander.mikhalit...@virtuozzo.com>
>> Subject: Re: [Devel] [PATCH RH9] dm-ploop: port the standby mode feature.
>>
>> On 13.10.22 16:33, Kui Liu wrote:
>> > Hello,
>> >
>> > First of all, please bear in mind that this patch is a port of a
patchset currently present in VZ 7 kernel.
>> > Original patches were added just to address a problem that would
only happen in our particular use case,
>> > where we need to use ploop devices as the back store for iSCSI
target, while ploop devices are backed
>> > by files stored in vstorage filesystem.
>>
>>
>> Ok, i don't quite get this yet. Since it is specific to vstorage
can we
>> check the queue on init and see if it is backed by that iSCSI
target and
>> set a static key or flag to enable the checks only if that is the
case?
>> check can be done via
queue->disk->major/minor/diskname/fops/pick-one
>> from a quick look.
>>
>> [LIU]: Do you mean that you want a 'check standby mode enable' flag so
that the check
>> is done only when the enable flag is true, and the flag should be set by
iSCSI target when the
>> ploop device is attached iSCSI target?
>> Well, it can be done, but I'm not sure why that's necessary? Current
code is kind copied
>> directly from original implementation, where there wasn't such 'enable'
flag, the question would
>> be why it wasn't implemented back then?
>
> Yes, I think it is better to be explicit and not blindly assume that it
> wouldn't affect anything. Old ploop code might not needed it. But
> dm-ploop is new code and it might need that flag - point is that it is
> better be safe and we know that we run on exactly that target.
>
>>
>> [snip]
>>
>> > >> + /* move to standby if delta lease was stolen or
mount is gone */
>> > >> + if (res != -EBUSY && res != -ENOTCONN && res !=
-EIO) {
>> > >> + return;
>> > >> + }
>> > >> +
>> > >> + spin_lock_irq(&q->queue_lock);
>> > >> + prev =
blk_queue_flag_test_and_set(QUEUE_FLAG_STANDBY, q);
>> > >> + spin_unlock_irq(&q->queue_lock);
>> > >> +
>> > >> + if (!prev)
>> > >> + pr_info("ploop: switch into standby mode\n");
>> > >> +}
>> >
>> >
>> > What guarantees that we got EBUSY, EIO or ENOTCONN for the
reasons
>> > listed in the description - "This mode shows that a delta
lease was
>> > stolen and it is impossible to handle any requests." - what
if we got
>> > such an error for other reason? If the problem comes from
nfs why not
>> > suspend device mapper from there ?
>> >
>> > [LIU]: I would assume vstorage filesystem guarantees that only
EBUSY, EIO, or ENOTCONN
>> > will be returned when the described event happens, and it
doesn't matter whether we could
>> > get these errors for other reasons. And what matters is, in our
use case, once the 3 errors are
>> > returned, the ploop device need to be flagged as 'standby mode'
which needs to be passed to
>> > iSCSI target driver. As for what happens when used with other
filesystems, we actually don't care,
>> > however you can see that the changes don't affect ploop's
normal behaviour either. >
>> > Also do this errors actually reach dm-ploop ? From my
recent tests i
>> > observed that if you have and EIO the filesystem gets it
and remounts RO
>> > - it might not reach ploop code at all. Even if you
suspend/resume the
>> > queue it would not help with the RO fs.
>> >
>> > [LIU]: When used with vstorage filesystem, these errors do
reach dm-ploop. Again, we really
>> > don’t care about other filesystems as long as it doesn't break
anything.
>>
>> If there is no check for the specific target and some of the errors
>> comes from other device - ploop will say it is into standby but no
>> device will check the flags - so it will not be true. My point is
that
>> it is hard to say it won't break anything.
>>
>> [LIU]: Currently only iSCSI target driver is aware of this flag, and of
course we have code in iSCSI target driver
>> to deal with this flag. However for any other ploop users who are not
aware of the standby flag, the flag is just
>> transparent, and it doesn't matter whether the flag is set or clear
anyway, then how does it break anything?
>>
>> Or is your concern that there may be users that would test the entire
flag without masking out uninterested bits?
>> I don’t believe such use exists in the kernel code.
>
> My concern is more about the debug message in case some other driver
> return one of the errors.
>
>>
>>
>>
>> [snip]
>> There is ploop->ti->table->md->queue, why not use it but cache
queue ptr
>> here ? Is it guaranteed that queue won't change and leave ploop
with
>> dangling ptr?
>>
>> [LIU]: Because of convenience, and yes, it is guaranteed that the queue
won't change
>> during ploop's lifespan. Looking at dm-mapper's code, apparently the
'md', hence 'md->queue',
>> outlives 'ploop', and md->queue can't be changed while ploop is still
alive.
>> In case of table reload, a new ploop instance will allocated and
initialized, the old one will be destroyed.
>
>
> Ok, i double checked and you are right about this.
>
>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
