Re: [PATCH 04/13] block/preallocate: serialise the filter state

[Kevin Wolf <kwolf@redhat.com>]

Am 30.09.2026 um 18:49 hat Denis V. Lunev geschrieben:
> From: Denis V. Lunev <den@openvz.org>
> 
> The filter keeps the file length it has seen, the length it has
> reserved and where the trailing zeroes start in plain fields, and
> updates them with a read, a compare and a store. That was indivisible
> while one node was driven by one AioContext. It is not any more:
> virtio-blk with iothread-vq-mapping drives one node from several.
> 
> Two writers extending the file then both pass the compare, and the
> smaller end lands last:
> 
>   writer A, [1G, 1G+64k)            writer B, [1G+64k, 1G+128k)
>   --------------------------------  --------------------------------
>   reads data_end 1G, 1G+64k > 1G
>                                     reads data_end 1G, 1G+128k > 1G
>                                     stores data_end = 1G+128k
>   stores data_end = 1G+64k
> 
> Both writes are acknowledged, and data_end now sits below B's data.
> Nothing raises it again except a later write ending beyond it, and
> closing the image truncates the file to it, so B's data is cut off the
> end. The last pair of extending writes before a shutdown is exactly
> where this bites.
> 
> Take a mutex across handle_write() and preallocate_co_getlength(). The
> remaining readers, apart from preallocate_co_truncate(), reach the
> state from global state code with the node drained, where no request
> is in flight.

I wanted to complain that you forgot preallocate_co_truncate(), but you
even mention it here explicitly. So I'll change my complaint into a
question: Why is leaving preallocate_co_truncate() alone okay?

I think it would be best to start with just taking the lock everywhere
where it could be potentially relevant, so that we have a state that we
can be reasonably confident in. If then optimisations are needed,
additional patches can explain how they replace the obvious mechanism.

When we never have this state that is quite obviously correct, then it
becomes a lot more difficult to tell if the result covers everything
that needs to be covered.

Another thought I had was that maybe I should give TSA for per-BDS locks
another try, so that we could mark data_end as GUARDED_BY(lock). The
changes made to TSA for the kernel in the past year or two might be
enough to make it workable now.

> Holding it cannot deadlock. A writer waiting for it has not reached
> bs->file yet, so it owns nothing below the filter, and the only calls
> made under it go to the child. preallocate_co_truncate() is left out
> for that reason: growing the file waits for the requests in flight on
> the child, and once the next patch issues the preallocation without
> the mutex, one of those can be a preallocation that needs it back.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: qemu-stable@nongnu.org
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> CC: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  block/preallocate.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/block/preallocate.c b/block/preallocate.c
> index 8cd98684f4..442cdb0176 100644
> --- a/block/preallocate.c
> +++ b/block/preallocate.c
> @@ -76,6 +76,8 @@ typedef struct BDRVPreallocateState {
>       * BLK_PERM_WRITE permissions on file child.
>       */
>  
> +    CoMutex lock;
> +
>      /* Gives up the resize permission on children when parents don't need it */
>      QEMUBH *drop_resize_bh;
>  } BDRVPreallocateState;
> @@ -155,6 +157,7 @@ static int preallocate_open(BlockDriverState *bs, QDict *options, int flags,
>       * For this to work, mark them invalid.
>       */
>      preallocate_set_state(s, -EINVAL);
> +    qemu_co_mutex_init(&s->lock);
>      s->drop_resize_bh = qemu_bh_new(preallocate_drop_resize_bh, bs);
>  
>      ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
> @@ -324,8 +327,8 @@ static bool GRAPH_RDLOCK has_prealloc_perms(BlockDriverState *bs)
>   * one bdrv_co_pwrite_zeroes() call.
>   */
>  static bool coroutine_fn GRAPH_RDLOCK
> -handle_write(BlockDriverState *bs, int64_t offset, int64_t bytes,
> -             bool want_merge_zero)
> +handle_write_locked(BlockDriverState *bs, int64_t offset, int64_t bytes,
> +                    bool want_merge_zero)
>  {
>      BDRVPreallocateState *s = bs->opaque;
>      int64_t end = offset + bytes;
> @@ -404,6 +407,20 @@ handle_write(BlockDriverState *bs, int64_t offset, int64_t bytes,
>      return want_merge_zero;
>  }
>  
> +static bool coroutine_fn GRAPH_RDLOCK
> +handle_write(BlockDriverState *bs, int64_t offset, int64_t bytes,
> +             bool want_merge_zero)
> +{
> +    BDRVPreallocateState *s = bs->opaque;
> +    bool ret;
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    ret = handle_write_locked(bs, offset, bytes, want_merge_zero);
> +    qemu_co_mutex_unlock(&s->lock);
> +
> +    return ret;
> +}

Even at the end of the series, handle_write() is the only called of
handle_write_locked(). Why don't you just take the lock in the existing
function instead of splitting it?

Kevin