[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 21/29] block: Assert serialisation assumption
From: |
Benoît Canet |
Subject: |
Re: [Qemu-devel] [PATCH v3 21/29] block: Assert serialisation assumptions in pwritev |
Date: |
Fri, 24 Jan 2014 17:09:20 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Le Friday 17 Jan 2014 à 15:15:11 (+0100), Kevin Wolf a écrit :
> If a request calls wait_serialising_requests() and actually has to wait
> in this function (i.e. a coroutine yield), other requests can run and
> previously read data (like the head or tail buffer) could become
> outdated. In this case, we would have to restart from the beginning to
> read in the updated data.
>
> However, we're lucky and don't actually need to do that: A request can
> only wait in the first call of wait_serialising_requests() because we
> mark it as serialising before that call, so any later requests would
> wait. So as we don't wait in practice, we don't have to reload the data.
>
> This is an important assumption that may not be broken or data
> corruption will happen. Document it with some assertions.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> block.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index 859e1aa..53d9bd5 100644
> --- a/block.c
> +++ b/block.c
> @@ -2123,14 +2123,15 @@ static bool
> tracked_request_overlaps(BdrvTrackedRequest *req,
> return true;
> }
>
> -static void coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
> +static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
> {
> BlockDriverState *bs = self->bs;
> BdrvTrackedRequest *req;
> bool retry;
> + bool waited = false;
>
> if (!bs->serialising_in_flight) {
> - return;
> + return false;
> }
>
> do {
> @@ -2156,11 +2157,14 @@ static void coroutine_fn
> wait_serialising_requests(BdrvTrackedRequest *self)
> qemu_co_queue_wait(&req->wait_queue);
> self->waiting_for = NULL;
> retry = true;
> + waited = true;
> break;
> }
> }
> }
> } while (retry);
> +
> + return waited;
> }
>
> /*
> @@ -3011,6 +3015,7 @@ static int coroutine_fn
> bdrv_aligned_pwritev(BlockDriverState *bs,
> QEMUIOVector *qiov, int flags)
> {
> BlockDriver *drv = bs->drv;
> + bool waited;
> int ret;
>
> int64_t sector_num = offset >> BDRV_SECTOR_BITS;
> @@ -3019,7 +3024,8 @@ static int coroutine_fn
> bdrv_aligned_pwritev(BlockDriverState *bs,
> assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
>
> - wait_serialising_requests(req);
> + waited = wait_serialising_requests(req);
> + assert(!waited || !req->serialising);
I we apply De Morgan's law to the expression we have:
assert(!(waited && req->serialising));
Is it really the condition we want ?
Best regards
Benoît
>
> ret = notifier_with_return_list_notify(&bs->before_write_notifiers, req);
>
> @@ -3119,9 +3125,11 @@ static int coroutine_fn
> bdrv_co_do_pwritev(BlockDriverState *bs,
> QEMUIOVector tail_qiov;
> struct iovec tail_iov;
> size_t tail_bytes;
> + bool waited;
>
> mark_request_serialising(&req, align);
> - wait_serialising_requests(&req);
> + waited = wait_serialising_requests(&req);
> + assert(!waited || !use_local_qiov);
>
> tail_buf = qemu_blockalign(bs, align);
> tail_iov = (struct iovec) {
> --
> 1.8.1.4
>
>
- [Qemu-devel] [PATCH v3 17/29] block: Generalise and optimise COR serialisation, (continued)
- [Qemu-devel] [PATCH v3 17/29] block: Generalise and optimise COR serialisation, Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 18/29] block: Make overlap range for serialisation dynamic, Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 19/29] block: Allow wait_serialising_requests() at any point, Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 20/29] block: Align requests in bdrv_co_do_pwritev(), Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 21/29] block: Assert serialisation assumptions in pwritev, Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 22/29] block: Change coroutine wrapper to byte granularity, Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 23/29] block: Make bdrv_pread() a bdrv_prwv_co() wrapper, Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 25/29] iscsi: Set bs->request_alignment, Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 24/29] block: Make bdrv_pwrite() a bdrv_prwv_co() wrapper, Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 26/29] blkdebug: Make required alignment configurable, Kevin Wolf, 2014/01/17
- [Qemu-devel] [PATCH v3 27/29] qemu-io: New command 'sleep', Kevin Wolf, 2014/01/17