Re: [PATCH v2 4/4] xen-blkback: support dynamic unbind/bind

Roger Pau Monné
2019-12-11T10:46:00Z

On Tue, Dec 10, 2019 at 11:33:47AM +0000, Paul Durrant wrote:
> By simply re-attaching to shared rings during connect_ring() rather than
> assuming they are freshly allocated (i.e assuming the counters are zero)
> it is possible for vbd instances to be unbound and re-bound from and to
> (respectively) a running guest.
>
> This has been tested by running:
>
> while true;
> do fio --name=randwrite --ioengine=libaio --iodepth=16 \
> --rw=randwrite --bs=4k --direct=1 --size=1G --verify=crc32;
> done
>
> in a PV guest whilst running:
>
> while true;
> do echo vbd-$DOMID-$VBD >unbind;
> echo unbound;
> sleep 5;

Is there anyway to know when the unbind has finished? AFAICT
xen_blkif_disconnect will return EBUSY if there are in flight
requests, and the disconnect won't be completed until those requests
are finished.

> echo vbd-$DOMID-$VBD >bind;
> echo bound;
> sleep 3;
> done
>
> in dom0 from /sys/bus/xen-backend/drivers/vbd to continuously unbind and
> re-bind its system disk image.
>
> This is a highly useful feature for a backend module as it allows it to be
> unloaded and re-loaded (i.e. updated) without requiring domUs to be halted.
> This was also tested by running:
>
> while true;
> do echo vbd-$DOMID-$VBD >unbind;
> echo unbound;
> sleep 5;
> rmmod xen-blkback;
> echo unloaded;
> sleep 1;
> modprobe xen-blkback;
> echo bound;
> cd $(pwd);
> sleep 3;
> done
>
> in dom0 whilst running the same loop as above in the (single) PV guest.
>
> Some (less stressful) testing has also been done using a Windows HVM guest
> with the latest 9.0 PV drivers installed.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
>
> v2:
> - Apply a sanity check to the value of rsp_prod and fail the re-attach
> if it is implausible
> - Set allow_rebind to prevent ring from being closed on unbind
> - Update test workload from dd to fio (with verification)
> ---
> drivers/block/xen-blkback/xenbus.c | 59 +++++++++++++++++++++---------
> 1 file changed, 41 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index e8c5c54e1d26..13d09630b237 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -181,6 +181,8 @@ static int xen_blkif_map(struct xen_blkif_ring *ring, grant_ref_t *gref,
> {
> int err;
> struct xen_blkif *blkif = ring->blkif;
> + struct blkif_common_sring *sring_common;
> + RING_IDX rsp_prod, req_prod;
>
> /* Already connected through? */
> if (ring->irq)
> @@ -191,46 +193,66 @@ static int xen_blkif_map(struct xen_blkif_ring *ring, grant_ref_t *gref,
> if (err < 0)
> return err;
>
> + sring_common = (struct blkif_common_sring *)ring->blk_ring;
> + rsp_prod = READ_ONCE(sring_common->rsp_prod);
> + req_prod = READ_ONCE(sring_common->req_prod);
> +
> switch (blkif->blk_protocol) {
> case BLKIF_PROTOCOL_NATIVE:
> {
> - struct blkif_sring *sring;
> - sring = (struct blkif_sring *)ring->blk_ring;
> - BACK_RING_INIT(&ring->blk_rings.native, sring,
> - XEN_PAGE_SIZE * nr_grefs);
> + struct blkif_sring *sring_native =
> + (struct blkif_sring *)ring->blk_ring;

I think you can constify both sring_native and sring_common (and the
other instances below).

> + unsigned int size = __RING_SIZE(sring_native,
> + XEN_PAGE_SIZE * nr_grefs);
> +
> + BACK_RING_ATTACH(&ring->blk_rings.native, sring_native,
> + rsp_prod, XEN_PAGE_SIZE * nr_grefs);
> + err = (req_prod - rsp_prod > size) ? -EIO : 0;
> break;
> }
> case BLKIF_PROTOCOL_X86_32:
> {
> - struct blkif_x86_32_sring *sring_x86_32;
> - sring_x86_32 = (struct blkif_x86_32_sring *)ring->blk_ring;
> - BACK_RING_INIT(&ring->blk_rings.x86_32, sring_x86_32,
> - XEN_PAGE_SIZE * nr_grefs);
> + struct blkif_x86_32_sring *sring_x86_32 =
> + (struct blkif_x86_32_sring *)ring->blk_ring;
> + unsigned int size = __RING_SIZE(sring_x86_32,
> + XEN_PAGE_SIZE * nr_grefs);
> +
> + BACK_RING_ATTACH(&ring->blk_rings.x86_32, sring_x86_32,
> + rsp_prod, XEN_PAGE_SIZE * nr_grefs);
> + err = (req_prod - rsp_prod > size) ? -EIO : 0;
> break;
> }
> case BLKIF_PROTOCOL_X86_64:
> {
> - struct blkif_x86_64_sring *sring_x86_64;
> - sring_x86_64 = (struct blkif_x86_64_sring *)ring->blk_ring;
> - BACK_RING_INIT(&ring->blk_rings.x86_64, sring_x86_64,
> - XEN_PAGE_SIZE * nr_grefs);
> + struct blkif_x86_64_sring *sring_x86_64 =
> + (struct blkif_x86_64_sring *)ring->blk_ring;
> + unsigned int size = __RING_SIZE(sring_x86_64,
> + XEN_PAGE_SIZE * nr_grefs);
> +
> + BACK_RING_ATTACH(&ring->blk_rings.x86_64, sring_x86_64,
> + rsp_prod, XEN_PAGE_SIZE * nr_grefs);
> + err = (req_prod - rsp_prod > size) ? -EIO : 0;

This is repeated for all ring types, might be worth to pull it out of
the switch...

> break;
> }
> default:
> BUG();
> }
> + if (err < 0)
> + goto fail;

...and placed here instead?

Thanks, Roger.