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
