[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: Convert .bdrv_truncate
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: Convert .bdrv_truncate callback to coroutine_fn |
Date: |
Mon, 25 Jun 2018 16:15:45 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
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
signature.asc
Description: OpenPGP digital signature