[RFC] block/mirror: fix file-system went to read-only after block-mirror

[RFC] block/mirror: fix file-system went to read-only after block-mirror

[Jinhua Cao]

1) Configure the VM disk as prdm.
...
<disk type='block' device='lun'>
  <driver name='qemu' type='raw' cache='none'/>
  <source dev='/dev/disk/by-id/scsi-368886030000000ca50c1cd1563996917' index='1'/>
  <backingStore/>
  <target dev='sdb' bus='scsi'/>
  <alias name='scsi0-0-0-1'/>
  <address type='drive' controller='0' bus='0' target='0' unit='1'/>
</disk>
...
Mount the disk in guest and keep the disk writing data continuously during block-mirror,
the file-system went to read-only after block-mirror.

2) This commit 6cdbceb12cf[mirror: Add filter-node-name to blockdev-mirror] introduces
mirror_top_bs which does not set default function for mirror_top_bs->drv->bdrv_co_ioctl.

3) The function bdrv_co_ioctl in block/io.c will be called during block-mirror, in this
function, the judgment is as follow:
---
    if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
        co.ret = -ENOTSUP;
        goto out;
    }
---
The mirror_top_bs does not set drv->bdrv_aio_ioctl or drv->bdrv_co_ioctl which result this
return -ENOTSUP. So the file-system went to read-only after block-mirror.

4) This patch set a default function for mirror_top_bs->drv->bdrv_aio_ioctl, fix this problem.
---
 block/mirror.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 019f6deaa5..63b788ec39 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1480,6 +1480,12 @@ static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
     return bdrv_co_flush(bs->backing->bs);
 }
 
+static int coroutine_fn bdrv_mirror_top_ioctl(BlockDriverState *bs,
+    unsigned long int req, void *buf)
+{
+    return 0;
+}
+
 static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int bytes, BdrvRequestFlags flags)
 {
@@ -1555,6 +1561,7 @@ static BlockDriver bdrv_mirror_top = {
     .bdrv_co_pwrite_zeroes      = bdrv_mirror_top_pwrite_zeroes,
     .bdrv_co_pdiscard           = bdrv_mirror_top_pdiscard,
     .bdrv_co_flush              = bdrv_mirror_top_flush,
+    .bdrv_co_ioctl              = bdrv_mirror_top_ioctl,
     .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
     .bdrv_child_perm            = bdrv_mirror_top_child_perm,
 
-- 
2.27.0

Re: [RFC] block/mirror: fix file-system went to read-only after block-mirror

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210624120635.54573-1-caojinhua1@huawei.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210624120635.54573-1-caojinhua1@huawei.com
Subject: [RFC] block/mirror: fix file-system went to read-only after block-mirror

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210624120635.54573-1-caojinhua1@huawei.com -> patchew/20210624120635.54573-1-caojinhua1@huawei.com
Switched to a new branch 'test'
6e2ba54 block/mirror: fix file-system went to read-only after block-mirror

=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 19 lines checked

Commit 6e2ba547f042 (block/mirror: fix file-system went to read-only after block-mirror) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210624120635.54573-1-caojinhua1@huawei.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC] block/mirror: fix file-system went to read-only after block-mirror

[Vladimir Sementsov-Ogievskiy]

24.06.2021 15:06, Jinhua Cao wrote:
> 1) Configure the VM disk as prdm.
> ...
> <disk type='block' device='lun'>
>    <driver name='qemu' type='raw' cache='none'/>
>    <source dev='/dev/disk/by-id/scsi-368886030000000ca50c1cd1563996917' index='1'/>
>    <backingStore/>
>    <target dev='sdb' bus='scsi'/>
>    <alias name='scsi0-0-0-1'/>
>    <address type='drive' controller='0' bus='0' target='0' unit='1'/>
> </disk>
> ...
> Mount the disk in guest and keep the disk writing data continuously during block-mirror,
> the file-system went to read-only after block-mirror.
> 
> 2) This commit 6cdbceb12cf[mirror: Add filter-node-name to blockdev-mirror] introduces
> mirror_top_bs which does not set default function for mirror_top_bs->drv->bdrv_co_ioctl.
> 
> 3) The function bdrv_co_ioctl in block/io.c will be called during block-mirror, in this
> function, the judgment is as follow:
> ---
>      if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
>          co.ret = -ENOTSUP;
>          goto out;
>      }
> ---
> The mirror_top_bs does not set drv->bdrv_aio_ioctl or drv->bdrv_co_ioctl which result this
> return -ENOTSUP. So the file-system went to read-only after block-mirror.
> 
> 4) This patch set a default function for mirror_top_bs->drv->bdrv_aio_ioctl, fix this problem.
> ---
>   block/mirror.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 019f6deaa5..63b788ec39 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1480,6 +1480,12 @@ static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
>       return bdrv_co_flush(bs->backing->bs);
>   }
>   
> +static int coroutine_fn bdrv_mirror_top_ioctl(BlockDriverState *bs,
> +    unsigned long int req, void *buf)
> +{
> +    return 0;
> +}

I'm not very familiar with .bdrv_co_ioctl kitchen in Qemu, but intuitively this seems wrong to me: you do nothing and report success in this handler.

So, probably more correct would be at least call bdrv_co_ioctl() on bs->backing->bs, which is filtered child of mirror_top.

This raise another question: what exactly this ioctl does? If it changes the device like write operation, we should somehow handle it to update dirty bitmap, otherwise mirror will not work correctly. In this way, it seems correctly to not implement .bdrv_co_ioctl in mirror_top, and keep it returning ENOTSUP, as we really can't support it..

> +
>   static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
>       int64_t offset, int bytes, BdrvRequestFlags flags)
>   {
> @@ -1555,6 +1561,7 @@ static BlockDriver bdrv_mirror_top = {
>       .bdrv_co_pwrite_zeroes      = bdrv_mirror_top_pwrite_zeroes,
>       .bdrv_co_pdiscard           = bdrv_mirror_top_pdiscard,
>       .bdrv_co_flush              = bdrv_mirror_top_flush,
> +    .bdrv_co_ioctl              = bdrv_mirror_top_ioctl,
>       .bdrv_refresh_filename      = bdrv_mirror_top_refresh_filename,
>       .bdrv_child_perm            = bdrv_mirror_top_child_perm,
>   
> 


-- 
Best regards,
Vladimir