| Message ID | 503CC6BC.2030101@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Aug 28, 2012 at 2:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 28/08/2012 15:04, Stefan Hajnoczi ha scritto: >> Zero writes have cluster granularity in QED. Therefore they can only be >> used to zero entire clusters. >> >> If the zero write request leaves sectors untouched, zeroing the entire >> cluster would obscure the backing file. Instead return -ENOTSUP, which >> is handled by block.c:bdrv_co_do_write_zeroes() and falls back to a >> regular write. >> >> The qemu-iotests 034 test cases covers this scenario. > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > Makes sense since both streaming and copy-on-read will do cluster-aligned writes. > > The "right fix" would not be much more complex though, something like this, right? > (untested). Yes but it's more complicated. To do a really good job we should slice off the first/last clusters if they are unaligned, handle them like regular allocating writes, and handle the middle of the request as a zero write. I decided to do the simplest implementation since this scenario only occurs in test cases, not real guests. Stefan
Am 28.08.2012 15:25, schrieb Paolo Bonzini: > Il 28/08/2012 15:04, Stefan Hajnoczi ha scritto: >> Zero writes have cluster granularity in QED. Therefore they can only be >> used to zero entire clusters. >> >> If the zero write request leaves sectors untouched, zeroing the entire >> cluster would obscure the backing file. Instead return -ENOTSUP, which >> is handled by block.c:bdrv_co_do_write_zeroes() and falls back to a >> regular write. >> >> The qemu-iotests 034 test cases covers this scenario. > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, applied to the block branch. > Makes sense since both streaming and copy-on-read will do cluster-aligned writes. > > The "right fix" would not be much more complex though, something like this, right? > (untested). I think Stefan's fix is the right one. It does the same thing as yours and it's much simpler. Kevin
diff --git a/block/qed.c b/block/qed.c index a02dbfd..a885671 100644 --- a/block/qed.c +++ b/block/qed.c @@ -1133,8 +1133,14 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len) return; } + if (qed_offset_into_cluster(s, acb->cur_pos) != 0 || + qed_offset_into_cluster(s, acb->cur_pos + acb->cur_qiov.size) != 0) { + goto copy; + } + cb = qed_aio_write_zero_cluster; } else { +copy: cb = qed_aio_write_prefill; acb->cur_cluster = qed_alloc_clusters(s, acb->cur_nclusters); }