Am 04.07.2018 um 18:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 04.07.2018 18:08, Kevin Wolf wrote:
> > Am 04.07.2018 um 16:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 03.07.2018 21:07, Vladimir Sementsov-Ogievskiy wrote:
> > > > Serialized writes should be used in copy-on-write of backup(sync=none)
> > > > for image fleecing scheme.
> > > >
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> > > > ---
> > > > include/block/block.h | 5 ++++-
> > > > block/io.c | 4 ++++
> > > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/block/block.h b/include/block/block.h
> > > > index e5c7759a0c..107113aad5 100644
> > > > --- a/include/block/block.h
> > > > +++ b/include/block/block.h
> > > > @@ -58,8 +58,11 @@ typedef enum {
> > > > * content. */
> > > > BDRV_REQ_WRITE_UNCHANGED = 0x40,
> > > > + /* Force request serializing. Only for writes. */
> > > > + BDRV_REQ_SERIALISING = 0x80,
> > > > +
> > > > /* Mask of valid flags */
> > > > - BDRV_REQ_MASK = 0x7f,
> > > > + BDRV_REQ_MASK = 0xff,
> > > > } BdrvRequestFlags;
> > > > typedef struct BlockSizes {
> > > > diff --git a/block/io.c b/block/io.c
> > > > index 1a2272fad3..d5ba078514 100644
> > > > --- a/block/io.c
> > > > +++ b/block/io.c
> > > > @@ -1572,6 +1572,10 @@ static int coroutine_fn
> > > > bdrv_aligned_pwritev(BdrvChild *child,
> > > > max_transfer = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_transfer,
> > > > INT_MAX),
> > > > align);
> > > > + if (flags & BDRV_REQ_SERIALISING) {
> > > > + mark_request_serialising(req, bdrv_get_cluster_size(bs));
> > > > + }
> > > > +
> > > > waited = wait_serialising_requests(req);
> > > > assert(!waited || !req->serialising);
> > > Kevin, about this assertion, introduced in 28de2dcd88de "block: Assert
> > > serialisation assumptions in pwritev"? Will not it fail with fleecing
> > > scheme? I'm afraid it will, when we will wait for client read with our
> > > request, marked serializing a moment ago...
> > Hm, looks like it yes.
> >
> > > Can we just switch it to assert(!waited || !req->partial);, setting
> > > req->partial in bdrv_co_pwritev for parts of unaligned requests? And allow
> > > new flag only for aligned requests?
> > >
> > > Other ideas?
> > The commit message of 28de2dcd88de tells you what we need to do (and
> > that just changing the assertion is wrong):
> >
> > 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.
> >
> > So we may need to return -EAGAIN here, check that in the caller and
> > repeat the write request from the very start.
>
> But in case of aligned request, there no previously read data, and we can
> safely continue. And actually it's our case (backup writes are aligned).
Hm, right. I don't particularly like req->partial because it's easy to
forget to set it to false when you do something that would need to be
repeated, but I don't have a better idea.
Kevin
