| Message ID | 20180704061320.2041-2-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Fix dst reading after tail copy offloading | expand |
Am 04.07.2018 um 08:13 hat Fam Zheng geschrieben: > This was noticed by the new image fleecing tests case 222. The issue is > apparent and we should just do the same right things as in > bdrv_aligned_pwritev. > > Reported-by: John Snow <jsnow@redhat.com> > Signed-off-by: Fam Zheng <famz@redhat.com> > --- a/block/io.c > +++ b/block/io.c > @@ -2945,6 +2945,10 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src, > dst, dst_offset, > bytes, flags); > } > + if (ret == 0) { > + int64_t end_sector = DIV_ROUND_UP(dst_offset + bytes, BDRV_SECTOR_SIZE); > + dst->bs->total_sectors = MAX(dst->bs->total_sectors, end_sector); > + } I think it's time to extract this stuff to a common function. I was already aware that a write request that extends the image isn't behaving exactly the same as bdrv_co_truncate(), and this one is bound to diverge from the other two instances as well. This is what bdrv_aligned_pwritev() does after the write: atomic_inc(&bs->write_gen); bdrv_set_dirty(bs, offset, bytes); stat64_max(&bs->wr_highest_offset, offset + bytes); if (ret >= 0) { bs->total_sectors = MAX(bs->total_sectors, end_sector); ret = 0; } Before the write, it also calls bs->before_write_notifiers. This is what bdrv_co_truncate() does after truncating the image: ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); if (ret < 0) { error_setg_errno(errp, -ret, "Could not refresh total sector count"); } else { offset = bs->total_sectors * BDRV_SECTOR_SIZE; } bdrv_dirty_bitmap_truncate(bs, offset); bdrv_parent_cb_resize(bs); atomic_inc(&bs->write_gen); This means that we probably have at least the following bugs in bdrv_co_copy_range_internal(): 1. bs->write_gen is not increased, a following flush is ignored 2. Dirty bitmaps are not dirtied 3. Dirty bitmaps are not resized when extending the image 4. bs->wr_highest_offset is not adjusted correctly 5. bs->total_sectors is not resized (the bug this patch fixes) 6. The parent node isn't notified about an image size change Of these, 3. and 6. also seem to be bugs in bdrv_aligned_pwritev(). Kevin
On Wed, 07/04 09:44, Kevin Wolf wrote: > Am 04.07.2018 um 08:13 hat Fam Zheng geschrieben: > > This was noticed by the new image fleecing tests case 222. The issue is > > apparent and we should just do the same right things as in > > bdrv_aligned_pwritev. > > > > Reported-by: John Snow <jsnow@redhat.com> > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > --- a/block/io.c > > +++ b/block/io.c > > @@ -2945,6 +2945,10 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src, > > dst, dst_offset, > > bytes, flags); > > } > > + if (ret == 0) { > > + int64_t end_sector = DIV_ROUND_UP(dst_offset + bytes, BDRV_SECTOR_SIZE); > > + dst->bs->total_sectors = MAX(dst->bs->total_sectors, end_sector); > > + } > > I think it's time to extract this stuff to a common function. I was > already aware that a write request that extends the image isn't behaving > exactly the same as bdrv_co_truncate(), and this one is bound to diverge > from the other two instances as well. > > This is what bdrv_aligned_pwritev() does after the write: > > atomic_inc(&bs->write_gen); > bdrv_set_dirty(bs, offset, bytes); > > stat64_max(&bs->wr_highest_offset, offset + bytes); > > if (ret >= 0) { > bs->total_sectors = MAX(bs->total_sectors, end_sector); > ret = 0; > } > > Before the write, it also calls bs->before_write_notifiers. > > This is what bdrv_co_truncate() does after truncating the image: > > ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); > if (ret < 0) { > error_setg_errno(errp, -ret, "Could not refresh total sector count"); > } else { > offset = bs->total_sectors * BDRV_SECTOR_SIZE; > } > bdrv_dirty_bitmap_truncate(bs, offset); > bdrv_parent_cb_resize(bs); > atomic_inc(&bs->write_gen); > > This means that we probably have at least the following bugs in > bdrv_co_copy_range_internal(): > > 1. bs->write_gen is not increased, a following flush is ignored > 2. Dirty bitmaps are not dirtied > 3. Dirty bitmaps are not resized when extending the image > 4. bs->wr_highest_offset is not adjusted correctly > 5. bs->total_sectors is not resized (the bug this patch fixes) > 6. The parent node isn't notified about an image size change > > Of these, 3. and 6. also seem to be bugs in bdrv_aligned_pwritev(). > Indeed, great insight. I'll work on v2. Fam
diff --git a/block/io.c b/block/io.c index 1a2272fad3..8e02f4ab95 100644 --- a/block/io.c +++ b/block/io.c @@ -2945,6 +2945,10 @@ static int coroutine_fn bdrv_co_copy_range_internal(BdrvChild *src, dst, dst_offset, bytes, flags); } + if (ret == 0) { + int64_t end_sector = DIV_ROUND_UP(dst_offset + bytes, BDRV_SECTOR_SIZE); + dst->bs->total_sectors = MAX(dst->bs->total_sectors, end_sector); + } tracked_request_end(&src_req); tracked_request_end(&dst_req); bdrv_dec_in_flight(src->bs);
This was noticed by the new image fleecing tests case 222. The issue is apparent and we should just do the same right things as in bdrv_aligned_pwritev. Reported-by: John Snow <jsnow@redhat.com> Signed-off-by: Fam Zheng <famz@redhat.com> --- block/io.c | 4 ++++ 1 file changed, 4 insertions(+)