Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/4] block: disallow BDRV_REQ_NO_SERIALISING for write

[Eric Blake]

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 <address@hidden>
> > ---
> >   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.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org