Re: [Qemu-devel] [PATCH v3 21/29] block: Assert serialisation assumptions in pwritev

[Kevin Wolf <kwolf@redhat.com>]

Am 24.01.2014 um 17:09 hat Benoît Canet geschrieben:
> 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 <kwolf@redhat.com>
> > ---
> >  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 ?

Yes. If req->serialising is true here, it's an RMW case and we already
had a mark_request_serialising() + wait_serialising_requests() pair. So
we have already waited for earlier requests and newer requests must be
waiting for us. Therefore, it can't happen that a serialising request
has to wait here.

Kevin