Re: [PATCH v6 08/15] block: introduce preallocate filter

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 11087 bytes --]

On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
> It's intended to be inserted between format and protocol nodes to
> preallocate additional space (expanding protocol file) on writes
> crossing EOF. It improves performance for file-systems with slow
> allocation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  docs/system/qemu-block-drivers.rst.inc |  26 ++
>  qapi/block-core.json                   |  20 +-
>  block/preallocate.c                    | 556 +++++++++++++++++++++++++
>  block/meson.build                      |   1 +
>  4 files changed, 602 insertions(+), 1 deletion(-)
>  create mode 100644 block/preallocate.c

Looks good to me in general.

[...]

> diff --git a/block/preallocate.c b/block/preallocate.c
> new file mode 100644
> index 0000000000..6510ad0149
> --- /dev/null
> +++ b/block/preallocate.c

[...]

> +/*
> + * Handle reopen.
> + *
> + * We must implement reopen handlers, otherwise reopen just don't work. Handle
> + * new options and don't care about preallocation state, as it is handled in
> + * set/check permission handlers.
> + */
> +
> +static int preallocate_reopen_prepare(BDRVReopenState *reopen_state,
> +                                      BlockReopenQueue *queue, Error **errp)
> +{
> +    PreallocateOpts *opts = g_new0(PreallocateOpts, 1);
> +
> +    if (!preallocate_absorb_opts(opts, reopen_state->options,
> +                                 reopen_state->bs->file->bs, errp)) {

I tried to find out whether this refers to the old file child, or the
post-reopen one.  As far as I could find out, there is no generic
implementation for changing the file child as part of x-blockdev-reopen:

{"error": {"class": "GenericError", "desc": "Cannot change the option
'file'"}}

Now that’s a shame.  That means you can’t reasonably integrate a
preallocate filter into an existing node graph unless the format driver
checks for the respective child option and issues a replace_node on
commit or something, right?  I suppose any driver who’d want to
implement child replacement would need to attach the new node in
prepare() as some pseudo-child, and then drop the old one and rename the
new one in commit().  I don’t think any driver does that yet, so I
suppose no format driver allows replacement of children yet (except for
quorum...).

Does anyone know what the status on that is?  Are there any plans for
implementing child replacement in reopen, or did I just miss something?

(It looks like the backing child can be replaced, but that’s probably
not a child where the preallocate filter would be placed on top...).

> +        g_free(opts);
> +        return -EINVAL;
> +    }
> +
> +    reopen_state->opaque = opts;
> +
> +    return 0;
> +}

[...]

> +/*
> + * Call on each write. Returns true if @want_merge_zero is true and the region
> + * [offset, offset + bytes) is zeroed (as a result of this call or earlier
> + * preallocation).
> + *
> + * want_merge_zero is used to merge write-zero request with preallocation in
> + * one bdrv_co_pwrite_zeroes() call.
> + */
> +static bool coroutine_fn handle_write(BlockDriverState *bs, int64_t offset,
> +                                      int64_t bytes, bool want_merge_zero)
> +{
> +    BDRVPreallocateState *s = bs->opaque;
> +    int64_t end = offset + bytes;
> +    int64_t prealloc_start, prealloc_end;
> +    int ret;
> +
> +    if (!has_prealloc_perms(bs)) {

Took me a bit to figure out, because it takes a trip to
preallocate_child_perm() to figure out exactly when we’re going to have
the necessary permissions for this to pass.

Then it turns out that this is going to be the case exactly when the
parents collectively have the same permissions (WRITE+RESIZE) on the
preallocate node.

Then I had to wonder whether it’s appropriate not to preallocate if
WRITE is taken, but RESIZE isn’t.  But that seems entirely correct, yes.
 If noone is going to grow the file, then there is no need for
preallocation.  (Vice versa, if noone is going to write, but only
resize, then there is no need for preallocation either.)

So this seems correct, yes.

(It could be argued that if one parent has WRITE, and another has RESIZE
(but neither has both), then we probably don’t need preallocation
either.  But in such an arcane case (which is impossible to figure out
in .bdrv_child_perm() anyway), we might as well just do preallocation.
Won’t hurt.)

> +        /* We don't have state neither should try to recover it */
> +        return false;
> +    }
> +
> +    if (s->data_end < 0) {
> +        s->data_end = bdrv_getlength(bs->file->bs);
> +        if (s->data_end < 0) {
> +            return false;
> +        }
> +
> +        if (s->file_end < 0) {
> +            s->file_end = s->data_end;
> +        }
> +    }
> +
> +    if (end <= s->data_end) {
> +        return false;
> +    }
> +
> +    /* We have valid s->data_end, and request writes beyond it. */
> +
> +    s->data_end = end;
> +    if (s->zero_start < 0 || !want_merge_zero) {
> +        s->zero_start = end;

Skipping this on want_merge_zero == true means that zero writes can be
cached; if you repeatedly perform zero writes into the preallocated
area, then none of those will actually be executed.  I legitimately
don’t know whether that’s OK.

I suppose until someone tells me it isn’t OK, I’ll believe it is.

> +    }
> +
> +    if (s->file_end < 0) {
> +        s->file_end = bdrv_getlength(bs->file->bs);
> +        if (s->file_end < 0) {
> +            return false;
> +        }
> +    }
> +
> +    /* Now s->data_end, s->zero_start and s->file_end are valid. */
> +
> +    if (end <= s->file_end) {
> +        /* No preallocation needed. */
> +        return want_merge_zero && offset >= s->zero_start;
> +    }
> +
> +    /* Now we want new preallocation, as request writes beyond s->data_end. */

s/data_end/file_end/

> +
> +    prealloc_start = want_merge_zero ? MIN(offset, s->file_end) : s->file_end;

I suppose you intentionally use s->file_end here instead of @end, even
if offset <= s->file_end.  I just mention it because I wonder whether
it’s really better to effectively write twice to the same area in such
cases (once zeroes for preallocation, then immediately the data) instead
of only writing the data and then preallocating past it.

(Though if it were the same code just with @end instead of s->file_end
for offset <= s->file_end, then the order would be to preallocate past
@end, and then to write the data.  Which might be suboptimal in terms of
how the blocks are then ordered in the filesystem.)

> +    prealloc_end = QEMU_ALIGN_UP(offset + bytes + s->opts.prealloc_size,

s/offset + bytes/end/?

> +                                 s->opts.prealloc_align);
> +    s->file_end = end;

Why is this set here, when it’s always overwritten after
bdrv_co_pwrite_zeroes() anyway?

(It also seems a bit wrong, because at this point we don’t know yet
whether the data write is going to succeed, so we don’t know for sure
whether the file end is really going to be @end without the preallocation.)

> +
> +    ret = bdrv_co_pwrite_zeroes(
> +            bs->file, prealloc_start, prealloc_end - prealloc_start,
> +            BDRV_REQ_NO_FALLBACK | BDRV_REQ_SERIALISING | BDRV_REQ_NO_WAIT);
> +    if (ret < 0) {
> +        s->file_end = ret;
> +        return false;
> +    }
> +
> +    s->file_end = prealloc_end;
> +    return want_merge_zero;
> +}
> +
> +static int coroutine_fn preallocate_co_pwrite_zeroes(BlockDriverState *bs,
> +        int64_t offset, int bytes, BdrvRequestFlags flags)
> +{
> +    bool want_merge_zero =
> +        (flags & (BDRV_REQ_ZERO_WRITE | BDRV_REQ_NO_FALLBACK)) == flags;

Isn’t this the same as !(flags & ~(ZERO_WRITE | NO_FALLBACK))?  (Maybe
only I would find that simpler to understand, though.)

> +    if (handle_write(bs, offset, bytes, want_merge_zero)) {
> +        return 0;
> +    }
> +
> +    return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
> +}

[...]

> +static int coroutine_fn
> +preallocate_co_truncate(BlockDriverState *bs, int64_t offset,
> +                        bool exact, PreallocMode prealloc,
> +                        BdrvRequestFlags flags, Error **errp)
> +{
> +    ERRP_GUARD();
> +    BDRVPreallocateState *s = bs->opaque;
> +    int ret;
> +
> +    if (s->data_end >= 0 && offset > s->data_end) {
> +        if (s->file_end < 0) {
> +            s->file_end = bdrv_getlength(bs->file->bs);
> +            if (s->file_end < 0) {
> +                error_setg(errp, "failed to get file length");
> +                return s->file_end;
> +            }
> +        }
> +
> +        if (prealloc == PREALLOC_MODE_FALLOC) {
> +            /*
> +             * If offset <= s->file_end, the task is already done, just
> +             * update s->file_end, to move part of "filter preallocation"

s/file_end/data_end/

> +             * to "preallocation requested by user".
> +             * Otherwise just proceed to preallocate missing part.
> +             */
> +            if (offset <= s->file_end) {
> +                s->data_end = offset;
> +                return 0;
> +            }

[...]

> +static int preallocate_check_perm(BlockDriverState *bs,
> +                                  uint64_t perm, uint64_t shared, Error **errp)
> +{
> +    BDRVPreallocateState *s = bs->opaque;
> +
> +    if (s->data_end >= 0 && !can_write_resize(perm)) {
> +        /*
> +         * Loose permissions.

*Lose

(I assume)

> +         * We should truncate in check_perm, as in set_perm bs->file->perm will
> +         * be already changed, and we should not violate it.
> +         */
> +        if (s->file_end < 0) {
> +            s->file_end = bdrv_getlength(bs->file->bs);
> +            if (s->file_end < 0) {
> +                error_setg(errp, "Failed to get file length");
> +                return s->file_end;
> +            }
> +        }
> +
> +        if (s->data_end < s->file_end) {
> +            int ret = bdrv_truncate(bs->file, s->data_end, true,
> +                                    PREALLOC_MODE_OFF, 0, NULL);
> +            if (ret < 0) {
> +                error_setg(errp, "Failed to drop preallocation");
> +                s->file_end = ret;
> +                return ret;
> +            }
> +        }
> +        /*
> +         * We will drop our permissions, as well as allow shared
> +         * permissions, anyone will be able to change the child, so mark
> +         * all states invalid. We'll regain control if get good permissions
> +         * back.
> +         */
> +        s->data_end = s->file_end = s->zero_start = -EINVAL;

Shouldn’t we clear these variables whenever !can_write_resize(perm), not
just if also s->data_end >= 0?

> +    }
> +
> +    return 0;
> +}

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]