| Message ID | 20180705074638.770905-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix image fleecing | expand |
On 07/05/2018 02:46 AM, Vladimir Sementsov-Ogievskiy wrote: > Before commit 9ded4a01149 "backup: Use copy offloading", > BDRV_REQ_NO_SERIALISING was used for only one case: read in > copy-on-write operation during backup. Also, the flag was handled only > on read path (in bdrv_co_preadv and bdrv_aligned_preadv). > > After 9ded4a01149, flag is used for not waiting serializing operations > on backup target (in same case of copy-on-write operation). This > behavior change is unsubstantiated and potentially dangerous, let's > drop it. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/block.h | 13 +++++++++++++ > block/io.c | 7 ++++++- > 2 files changed, 19 insertions(+), 1 deletion(-) > Commenting only on the grammar: > diff --git a/include/block/block.h b/include/block/block.h > index e5c7759a0c..a06a4d27de 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -50,6 +50,19 @@ typedef enum { > * opened with BDRV_O_UNMAP. > */ > BDRV_REQ_MAY_UNMAP = 0x4, > + > + /* The BDRV_REQ_NO_SERIALISING means that we don't want to > + * wait_serialising_requests(), when reading. Either: /* BDRV_REQ_NO_SERALISING means that... or /* The BDRV_REQ_NO_SERIALISING flag means that... s/want to/want/ > + * > + * This flag is used for backup copy on write operation, when we need to > + * read old data before write (write notifier triggered). It is ok, due to > + * we already waited for serializing requests in initiative write (see > + * bdrv_aligned_pwritev), and it is necessary for the case when initiative > + * write is serializing itself (we'll dead lock waiting it). It is okay since we already waited for other serializing requests in the initiating write (see bdrv_aligned_pwritev), and it is necessary since the initiating write is already serializing (without the flag, the read would deadlock waiting for the write to complete). > + * > + * The described case is the only usage for the flag for now, so, it is > + * supported only for read operation and restricted for write. This last sentence is rather wordy; I'm fine with just: The flag is only valid during read operations. > + */ > BDRV_REQ_NO_SERIALISING = 0x8, > BDRV_REQ_FUA = 0x10, > BDRV_REQ_WRITE_COMPRESSED = 0x20, We're inconsistent on which flags we document; it might be nice to have a comment for each of them. But not necessarily this patch's problem.
On 07/06/2018 04:32 PM, Eric Blake wrote: > On 07/05/2018 02:46 AM, Vladimir Sementsov-Ogievskiy wrote: >> Before commit 9ded4a01149 "backup: Use copy offloading", >> BDRV_REQ_NO_SERIALISING was used for only one case: read in >> copy-on-write operation during backup. Also, the flag was handled only >> on read path (in bdrv_co_preadv and bdrv_aligned_preadv). >> >> After 9ded4a01149, flag is used for not waiting serializing operations >> on backup target (in same case of copy-on-write operation). This >> behavior change is unsubstantiated and potentially dangerous, let's >> drop it. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/block/block.h | 13 +++++++++++++ >> block/io.c | 7 ++++++- >> 2 files changed, 19 insertions(+), 1 deletion(-) >> > > Commenting only on the grammar: > >> diff --git a/include/block/block.h b/include/block/block.h >> index e5c7759a0c..a06a4d27de 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -50,6 +50,19 @@ typedef enum { >> * opened with BDRV_O_UNMAP. >> */ >> BDRV_REQ_MAY_UNMAP = 0x4, >> + >> + /* The BDRV_REQ_NO_SERIALISING means that we don't want to >> + * wait_serialising_requests(), when reading. > > Either: > > /* BDRV_REQ_NO_SERALISING means that... > > or > > /* The BDRV_REQ_NO_SERIALISING flag means that... > > s/want to/want/ Or, after reading patch 3/4, BDRV_REQ_NO_SERAILISING bypasses request serialisation during reads. (where the counterpart starts: BDRV_REQ_SERIALISING forces request serialisation during writes. ) > >> + * >> + * This flag is used for backup copy on write operation, when we >> need to >> + * read old data before write (write notifier triggered). It is >> ok, due to >> + * we already waited for serializing requests in initiative write >> (see >> + * bdrv_aligned_pwritev), and it is necessary for the case when >> initiative >> + * write is serializing itself (we'll dead lock waiting it). > > It is okay since we already waited for other serializing requests in the > initiating write (see bdrv_aligned_pwritev), and it is necessary since > the initiating write is already serializing (without the flag, the read > would deadlock waiting for the write to complete). > >> + * >> + * The described case is the only usage for the flag for now, so, >> it is >> + * supported only for read operation and restricted for write. > > This last sentence is rather wordy; I'm fine with just: > > The flag is only valid during read operations. Or even just drop this last paragraph, since the first sentence of the comment already stated it was only for reads. > >> + */ >> BDRV_REQ_NO_SERIALISING = 0x8, >> BDRV_REQ_FUA = 0x10, >> BDRV_REQ_WRITE_COMPRESSED = 0x20, > > We're inconsistent on which flags we document; it might be nice to have > a comment for each of them. But not necessarily this patch's problem. >
07.07.2018 00:32, Eric Blake wrote: > On 07/05/2018 02:46 AM, Vladimir Sementsov-Ogievskiy wrote: >> Before commit 9ded4a01149 "backup: Use copy offloading", >> BDRV_REQ_NO_SERIALISING was used for only one case: read in >> copy-on-write operation during backup. Also, the flag was handled only >> on read path (in bdrv_co_preadv and bdrv_aligned_preadv). >> >> After 9ded4a01149, flag is used for not waiting serializing operations >> on backup target (in same case of copy-on-write operation). This >> behavior change is unsubstantiated and potentially dangerous, let's >> drop it. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> include/block/block.h | 13 +++++++++++++ >> block/io.c | 7 ++++++- >> 2 files changed, 19 insertions(+), 1 deletion(-) >> > > Commenting only on the grammar: > >> diff --git a/include/block/block.h b/include/block/block.h >> index e5c7759a0c..a06a4d27de 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -50,6 +50,19 @@ typedef enum { >> * opened with BDRV_O_UNMAP. >> */ >> BDRV_REQ_MAY_UNMAP = 0x4, >> + >> + /* The BDRV_REQ_NO_SERIALISING means that we don't want to >> + * wait_serialising_requests(), when reading. > > Either: > > /* BDRV_REQ_NO_SERALISING means that... > > or > > /* The BDRV_REQ_NO_SERIALISING flag means that... > > s/want to/want/ > >> + * >> + * This flag is used for backup copy on write operation, when we >> need to >> + * read old data before write (write notifier triggered). It is >> ok, due to >> + * we already waited for serializing requests in initiative >> write (see >> + * bdrv_aligned_pwritev), and it is necessary for the case when >> initiative >> + * write is serializing itself (we'll dead lock waiting it). > > It is okay since we already waited for other serializing requests in > the initiating write (see bdrv_aligned_pwritev), and it is necessary > since the initiating write is already serializing (without the flag, > the read would deadlock waiting for the write to complete). it is necessary only for the case ..., not always, isn't it? I'll use It is okay since we already waited for other serializing requests in the initiating write (see bdrv_aligned_pwritev), and it is necessary if the initiating write is already serializing (without the flag, the read would deadlock waiting for the serialising write to complete). > >> + * >> + * The described case is the only usage for the flag for now, >> so, it is >> + * supported only for read operation and restricted for write. > > This last sentence is rather wordy; I'm fine with just: > > The flag is only valid during read operations. ok > >> + */ >> BDRV_REQ_NO_SERIALISING = 0x8, >> BDRV_REQ_FUA = 0x10, >> BDRV_REQ_WRITE_COMPRESSED = 0x20, > > We're inconsistent on which flags we document; it might be nice to > have a comment for each of them. But not necessarily this patch's > problem. >
07.07.2018 00:46, Eric Blake wrote: > On 07/06/2018 04:32 PM, Eric Blake wrote: >> On 07/05/2018 02:46 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Before commit 9ded4a01149 "backup: Use copy offloading", >>> BDRV_REQ_NO_SERIALISING was used for only one case: read in >>> copy-on-write operation during backup. Also, the flag was handled only >>> on read path (in bdrv_co_preadv and bdrv_aligned_preadv). >>> >>> After 9ded4a01149, flag is used for not waiting serializing operations >>> on backup target (in same case of copy-on-write operation). This >>> behavior change is unsubstantiated and potentially dangerous, let's >>> drop it. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> include/block/block.h | 13 +++++++++++++ >>> block/io.c | 7 ++++++- >>> 2 files changed, 19 insertions(+), 1 deletion(-) >>> >> >> Commenting only on the grammar: >> >>> diff --git a/include/block/block.h b/include/block/block.h >>> index e5c7759a0c..a06a4d27de 100644 >>> --- a/include/block/block.h >>> +++ b/include/block/block.h >>> @@ -50,6 +50,19 @@ typedef enum { >>> * opened with BDRV_O_UNMAP. >>> */ >>> BDRV_REQ_MAY_UNMAP = 0x4, >>> + >>> + /* The BDRV_REQ_NO_SERIALISING means that we don't want to >>> + * wait_serialising_requests(), when reading. >> >> Either: >> >> /* BDRV_REQ_NO_SERALISING means that... >> >> or >> >> /* The BDRV_REQ_NO_SERIALISING flag means that... >> >> s/want to/want/ > > Or, after reading patch 3/4, > > BDRV_REQ_NO_SERAILISING bypasses request serialisation during reads. > > (where the counterpart starts: > > BDRV_REQ_SERIALISING forces request serialisation during writes. > ) hmm. "bypasses request serialisation during reads" - it's not quite right, I think. NO_SERIALISING means only that we will not wait for other requests. However, our request may be serialising itself, so other requests will wait for it (which, actually don't really make sense, because they will have to wait for original write operation too, which starts before read and finishes after it). So, it's funny, but currently, all "NO_SERIALISING" requests are strictly serialising in fact (except that they intersect with original writes (but in fact, are done before them). > >> >>> + * >>> + * This flag is used for backup copy on write operation, when >>> we need to >>> + * read old data before write (write notifier triggered). It is >>> ok, due to >>> + * we already waited for serializing requests in initiative >>> write (see >>> + * bdrv_aligned_pwritev), and it is necessary for the case when >>> initiative >>> + * write is serializing itself (we'll dead lock waiting it). >> >> It is okay since we already waited for other serializing requests in >> the initiating write (see bdrv_aligned_pwritev), and it is necessary >> since the initiating write is already serializing (without the flag, >> the read would deadlock waiting for the write to complete). >> >>> + * >>> + * The described case is the only usage for the flag for now, >>> so, it is >>> + * supported only for read operation and restricted for write. >> >> This last sentence is rather wordy; I'm fine with just: >> >> The flag is only valid during read operations. > > Or even just drop this last paragraph, since the first sentence of the > comment already stated it was only for reads. > >> >>> + */ >>> BDRV_REQ_NO_SERIALISING = 0x8, >>> BDRV_REQ_FUA = 0x10, >>> BDRV_REQ_WRITE_COMPRESSED = 0x20, >> >> We're inconsistent on which flags we document; it might be nice to >> have a comment for each of them. But not necessarily this patch's >> problem. >> >
diff --git a/include/block/block.h b/include/block/block.h index e5c7759a0c..a06a4d27de 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -50,6 +50,19 @@ typedef enum { * opened with BDRV_O_UNMAP. */ BDRV_REQ_MAY_UNMAP = 0x4, + + /* The BDRV_REQ_NO_SERIALISING means that we don't want to + * wait_serialising_requests(), when reading. + * + * This flag is used for backup copy on write operation, when we need to + * read old data before write (write notifier triggered). It is ok, due to + * we already waited for serializing requests in initiative write (see + * bdrv_aligned_pwritev), and it is necessary for the case when initiative + * write is serializing itself (we'll dead lock waiting it). + * + * The described case is the only usage for the flag for now, so, it is + * supported only for read operation and restricted for write. + */ BDRV_REQ_NO_SERIALISING = 0x8, BDRV_REQ_FUA = 0x10, BDRV_REQ_WRITE_COMPRESSED = 0x20, diff --git a/block/io.c b/block/io.c index 1a2272fad3..51bef9fc50 100644 --- a/block/io.c +++ b/block/io.c @@ -1572,6 +1572,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child, max_transfer = QEMU_ALIGN_DOWN(MIN_NON_ZERO(bs->bl.max_transfer, INT_MAX), align); + /* BDRV_REQ_NO_SERIALISING is only for read operation */ + assert(!(flags & BDRV_REQ_NO_SERIALISING)); waited = wait_serialising_requests(req); assert(!waited || !req->serialising); assert(req->overlap_offset <= offset); @@ -2931,9 +2933,12 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src, bytes, BDRV_TRACKED_WRITE); if (!(flags & BDRV_REQ_NO_SERIALISING)) { + /* BDRV_REQ_NO_SERIALISING is only for read */ wait_serialising_requests(&src_req); - wait_serialising_requests(&dst_req); } + + wait_serialising_requests(&dst_req); + if (recurse_src) { ret = src->bs->drv->bdrv_co_copy_range_from(src->bs, src, src_offset,
Before commit 9ded4a01149 "backup: Use copy offloading", BDRV_REQ_NO_SERIALISING was used for only one case: read in copy-on-write operation during backup. Also, the flag was handled only on read path (in bdrv_co_preadv and bdrv_aligned_preadv). After 9ded4a01149, flag is used for not waiting serializing operations on backup target (in same case of copy-on-write operation). This behavior change is unsubstantiated and potentially dangerous, let's drop it. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block.h | 13 +++++++++++++ block/io.c | 7 ++++++- 2 files changed, 19 insertions(+), 1 deletion(-)