Re: [PATCH v4 5/9] raw-format: Support BDRV_REQ_ZERO_WRITE for truncate

From: Kevin Wolf <kwolf@redhat.com>
To: Eric Blake <eblake@redhat.com>
Cc: vsementsov@virtuozzo.com, berto@igalia.com,
	qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [PATCH v4 5/9] raw-format: Support BDRV_REQ_ZERO_WRITE for truncate
Date: Mon, 20 Apr 2020 17:32:17 +0200	[thread overview]
Message-ID: <20200420153217.GA29316@linux.fritz.box> (raw)
In-Reply-To: <b30bde4c-1ac1-f7cf-0c1f-1e8f52c4c821@redhat.com>

Am 20.04.2020 um 17:14 hat Eric Blake geschrieben:
> On 4/20/20 8:32 AM, Kevin Wolf wrote:
> > The raw format driver can simply forward the flag and let its bs->file
> > child take care of actually providing the zeros.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/raw-format.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/raw-format.c b/block/raw-format.c
> > index 3465c9a865..ab69ac46d3 100644
> > --- a/block/raw-format.c
> > +++ b/block/raw-format.c
> > @@ -387,7 +387,7 @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
> >       s->size = offset;
> >       offset += s->offset;
> > -    return bdrv_co_truncate(bs->file, offset, exact, prealloc, 0, errp);
> > +    return bdrv_co_truncate(bs->file, offset, exact, prealloc, flags, errp);
> >   }
> >   static void raw_eject(BlockDriverState *bs, bool eject_flag)
> > @@ -445,6 +445,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
> >       bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
> >           ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
> >               bs->file->bs->supported_zero_flags);
> > +    bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
> 
> Shouldn't this be:
> 
> bs->supported_truncate_flags = (bs->file->bs->supported_truncate_flags &
>                                 BDRV_REQ_ZERO_WRITE);
> 
> rather than unconditionally advertising something that the underlying layer
> may lack?

Maybe that makes more sense, yes.

I think in practice it wouldn't make a difference because the nested
bdrv_co_truncate() would still fail rather than silently ignoring the
flag. It would behave the same as filter drivers, which also recursively
call bdrv_co_truncate() without checking the flag first (which is, of
course, because I don't want to modify each filter driver).

Kevin