[1/2] block: Fix dst total_sectors after copy offloading
diff mbox series

Message ID 20180704061320.2041-2-famz@redhat.com
State New
Headers show
Series
  • block: Fix dst reading after tail copy offloading
Related show

Commit Message

Fam Zheng July 4, 2018, 6:13 a.m. UTC
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(+)

Comments

Kevin Wolf July 4, 2018, 7:44 a.m. UTC | #1
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
Fam Zheng July 4, 2018, 8:42 a.m. UTC | #2
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

Patch
diff mbox series

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);