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. > > 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. * 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(). Do you agree? Kevin
signature.asc
Description: PGP signature
