diff mbox series

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

Message ID 20200420133214.28921-6-kwolf@redhat.com
State New, archived
Headers show
Series block: Fix resize (extending) of short overlays | expand

Commit Message

Kevin Wolf April 20, 2020, 1:32 p.m. UTC
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(-)

Comments

Alberto Garcia April 20, 2020, 2:03 p.m. UTC | #1
On Mon 20 Apr 2020 03:32:10 PM CEST, 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>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto
Eric Blake April 20, 2020, 3:14 p.m. UTC | #2
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?
Kevin Wolf April 20, 2020, 3:32 p.m. UTC | #3
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
Eric Blake April 20, 2020, 3:53 p.m. UTC | #4
On 4/20/20 10:32 AM, Kevin Wolf wrote:

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

If nothing else, it is more consistent with what we are doing for 
supported_zero_flags.  I also argue that having a reference to the 
passthrough is easier to grep for, if we ever add new flags in the 
future.  That is, while keeping passthrough as opt-in rather than blind 
copying or blind assignment is slightly more code, it is easier to maintain.

> 
> 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).

Probably true, but consistency and ease of maintenance are better than 
proving action at a distance :)
diff mbox series

Patch

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;
 
     if (bs->probed && !bdrv_is_read_only(bs)) {
         bdrv_refresh_filename(bs->file->bs);