Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: Convert .bdrv_truncate callback to coroutine_fn

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2883 bytes --]

On 2018-06-25 11:51, Kevin Wolf wrote:
> Am 25.06.2018 um 11:02 hat Stefan Hajnoczi geschrieben:
>> On Thu, Jun 21, 2018 at 07:06:56PM +0200, Kevin Wolf wrote:
>>> bdrv_truncate() is an operation that can block (even for a quite long
>>> time, depending on the PreallocMode) in I/O paths that shouldn't block.
>>> Convert it to a coroutine_fn so that we have the infrastructure for
>>> drivers to make their .bdrv_co_truncate implementation asynchronous.

parallels_co_check() is a remaining coroutine_fn that calls
bdrv_truncate(), maybe it should call bdrv_co_truncate() now.

(And vhdx_allocate_block() probably should be a coroutine_fn, but that's
something for another time, I suppose.)

>> block/commit.c:commit_run() invokes blk_truncate() outside of a drained
>> region.  I haven't looked for other instances, but this patch opens the
>> door for races with other I/O requests.  Are you sure it's safe to make
>> this asynchronous without request serialization?
> 
> After trying to explain why it's correct, I start to think that you're
> right at least in one case. The new thing after this patch is that the
> truncate operation isn't atomic any more. What this means depends on the
> block driver:
> 
> * file-posix/win32: I think this one is okay. The truncate
>   implementation doesn't depend in any way on the content of the image.
>   Preallocation could be critical (in that it could overwrite
>   concurrently issued write requests), but the BDS size is only adjusted
>   after the driver callback has returned, so there can't be a concurrent
>   write request.

Except when the BDS is growable (which every BDS is).  qcow2 generally
writes beyond the EOF, so I suppose a concurrent preallocating
truncation may result in a race.

We don't have preallocating truncation over QMP yet, so technically this
is not an issue, but with your jobs series and this series, we may well
have it soon.

> * copy-on-read, crypto, raw-format: Essentially just filter drivers that
>   pass the request to a child node, no problem.
> 
> * gluster, iscsi, nfs, rbd, ssh: Won't yield even after this series, so
>   trivially okay.
> 
> * qcow2: This one is where you're right, it needs to hold s->lock so
>   that the metadata modifications become safe.
> 
> * qed: Does a single header update, should be fine without locking.
> 
> * sheepdog: Doesn't yield until it starts preallocation. For
>   preallocation, the same reasoning as for file-posix applies.
> 
> So, if I got this right, the only thing to change is holding s->lock in
> qcow2_co_truncate().

By holding the lock, we would probably solve the race for qcow2, but
probably not for raw-format.

(And I think qcow2 and raw are the only formats that support
preallocation on truncation.  (Well, raw doesn't really support it, but
it does allow it.))

Max


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