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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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