Re: [PATCH for-4.2 v2 3/3] block/file-posix: Let post-EOF fallocate serialize

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

01.11.2019 18:25, Max Reitz wrote:

Sorry for being late, I have some comments

> The XFS kernel driver has a bug that may cause data corruption for qcow2
> images as of qemu commit c8bb23cbdbe32f.  We can work around it by
> treating post-EOF fallocates as serializing up until infinity (INT64_MAX
> in practice).
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/file-posix.c | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 0b7e904d48..1f0f61a02b 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2721,6 +2721,42 @@ raw_do_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int bytes,
>       RawPosixAIOData acb;
>       ThreadPoolFunc *handler;
>   
> +#ifdef CONFIG_FALLOCATE
> +    if (offset + bytes > bs->total_sectors * BDRV_SECTOR_SIZE) {
> +        BdrvTrackedRequest *req;
> +        uint64_t end;
> +
> +        /*
> +         * This is a workaround for a bug in the Linux XFS driver,
> +         * where writes submitted through the AIO interface will be
> +         * discarded if they happen beyond a concurrently running
> +         * fallocate() that increases the file length (i.e., both the
> +         * write and the fallocate() happen beyond the EOF).
> +         *
> +         * To work around it, we extend the tracked request for this
> +         * zero write until INT64_MAX (effectively infinity), and mark
> +         * it as serializing.
> +         *
> +         * We have to enable this workaround for all filesystems and
> +         * AIO modes (not just XFS with aio=native), because for
> +         * remote filesystems we do not know the host configuration.
> +         */
> +
> +        req = bdrv_co_get_self_request(bs);
> +        assert(req);
> +        assert(req->type == BDRV_TRACKED_WRITE);
> +        assert(req->offset <= offset);
> +        assert(req->offset + req->bytes >= offset + bytes);

Why these assertions? TrackedRequest offset and bytes fields correspond to the original request. When request is being expanded to satisfy request_alignment, these fields are not updated.
So, maybe, we should assert overlap_offset and overlap_bytes?

> +
> +        end = INT64_MAX & -(uint64_t)bs->bl.request_alignment;
> +        req->bytes = end - req->offset;

And I doubt that we should update req->bytes. We never updated it in other places, it corresponds to original request. It's enough to update overlap_bytes to achieve corresponding serialising.

> +        req->overlap_bytes = req->bytes;
> +
> +        bdrv_mark_request_serialising(req, bs->bl.request_alignment);

Not sure, how much should we care about request_alignment here, I think, it's enough to just set req->overlap_bytes = INT64_MAX - req->overlap_offest, but it doesn't really matter.

> +        bdrv_wait_serialising_requests(req);
> +    }
> +#endif
> +
>       acb = (RawPosixAIOData) {
>           .bs             = bs,
>           .aio_fildes     = s->fd,
> 


-- 
Best regards,
Vladimir