| Message ID | 20200420133214.28921-6-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Fix resize (extending) of short overlays | expand |
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
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?
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
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 --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);
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(-)