On Wed, 01/06 18:53, Max Reitz wrote:
> On 05.01.2016 09:46, Fam Zheng wrote:
> > The "pnum < nb_sectors" condition in deciding whether to actually copy
> > data is unnecessarily strict, and the qiov initialization is
> > unnecessarily for bdrv_aio_write_zeroes and bdrv_aio_discard.
> >
> > Rewrite mirror_iteration to fix both flaws.
> >
> > Signed-off-by: Fam Zheng <f...@redhat.com>
> > ---
> > block/mirror.c | 347
> > +++++++++++++++++++++++++++++++++++----------------------
> > trace-events | 1 -
> > 2 files changed, 216 insertions(+), 132 deletions(-)
> >
> > diff --git a/block/mirror.c b/block/mirror.c
> > index f201f2b..e3e9fad 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -46,7 +46,6 @@ typedef struct MirrorBlockJob {
> > BlockdevOnError on_source_error, on_target_error;
> > bool synced;
> > bool should_complete;
> > - int64_t sector_num;
> > int64_t granularity;
> > size_t buf_size;
> > int64_t bdev_length;
> > @@ -63,6 +62,8 @@ typedef struct MirrorBlockJob {
> > int ret;
> > bool unmap;
> > bool waiting_for_io;
> > + int target_cluster_sectors;
> > + int max_iov;
> > } MirrorBlockJob;
> >
> > typedef struct MirrorOp {
> > @@ -158,115 +159,93 @@ static void mirror_read_complete(void *opaque, int
> > ret)
> > mirror_write_complete, op);
> > }
> >
> > -static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
> > +/* Round sector_num and/or nb_sectors to target cluster if COW is needed,
> > and
> > + * return the offset of the adjusted tail sector against original. */
> > +static int mirror_cow_align(MirrorBlockJob *s,
> > + int64_t *sector_num,
> > + int *nb_sectors)
> > +{
> > + bool head_need_cow, tail_need_cow;
> > + int diff = 0;
> > + int chunk_sectors = s->granularity >> BDRV_SECTOR_BITS;
> > +
> > + head_need_cow = !test_bit(*sector_num / chunk_sectors, s->cow_bitmap);
> > + tail_need_cow = !test_bit((*sector_num + *nb_sectors - 1) /
> > chunk_sectors,
> > + s->cow_bitmap);
> > + if (head_need_cow || tail_need_cow) {
> > + int64_t align_sector_num;
> > + int align_nb_sectors;
> > + bdrv_round_to_clusters(s->target, *sector_num, *nb_sectors,
> > + &align_sector_num, &align_nb_sectors);
> > + if (tail_need_cow) {
> > + diff = align_sector_num + align_nb_sectors
> > + - (*sector_num + *nb_sectors);
> > + assert(diff >= 0);
> > + *nb_sectors += diff;
> > + }
> > + if (head_need_cow) {
> > + int d = *sector_num - align_sector_num;
> > + assert(d >= 0);
> > + *sector_num = align_sector_num;
> > + *nb_sectors += d;
> > + }
> > + }
> > +
> > + /* If the resulting chunks are more than max_iov, we have to shrink it
> > + * under the alignment restriction. */
> > + if (*nb_sectors > chunk_sectors * s->max_iov) {
> > + int shrink = *nb_sectors - chunk_sectors * s->max_iov;
> > + if (tail_need_cow) {
> > + /* In this case, tail must be aligned already, so we just make
> > sure
> > + * the shrink is also aligned. */
> > + shrink -= shrink % s->target_cluster_sectors;
> > + }
> > + assert(shrink);
> > + diff -= shrink;
> > + *nb_sectors -= shrink;
> > + }
>
> Hm, looking at this closer... If we get here with tail_need_cow not
> being set, we may end up with an unaligned tail, which then may need COW
> (because it points to somewhere else than before).
>
> On the other hand, if we get here with tail_need_cow being set, shrink
> will be decreased so that it will only remove an aligned number of
> sectors from *nb_sectors; however, because shrink is increased, that
> means that *nb_sectors may then still be too large. Also, because of the
> shrink, the tail may in fact not need COW any more.
You're right. I'll fix this again. But I don't think we care about the "not
need COW any more" case. Let's keep this function simple as it's not the
hottest path.
Fam
>
> Should we do this check before we test whether we need COW and do the
> correction in a way that ensures that the cluster adjustment can never
> increase *nb_sectors beyond chunk_sectors * s->max_iov?
