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

[Kevin Wolf]

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