| Message ID | 1339508835-15108-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 12/06/2012 15:47, Kevin Wolf ha scritto: > copy_sectors() always uses the sum (cluster_offset + n_start) or > (start_sect + n_start), so if some value is added to both cluster_offset > and start_sect, and subtracted from n_start, it's cancelled out anyway. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/qcow2-cluster.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 9aee9fc..763b724 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -640,11 +640,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) > } > > if (m->nb_available & (s->cluster_sectors - 1)) { > - uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1); > cow = true; > qemu_co_mutex_unlock(&s->lock); > - ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9), > - m->nb_available - end, s->cluster_sectors); > + ret = copy_sectors(bs, start_sect, cluster_offset, > + m->nb_available, s->cluster_sectors); Do you need to add end to s->cluster_sectors too, so that "start_sect + n_end" and "n_end - n_start" remain the same? Paolo
Am 12.06.2012 16:00, schrieb Paolo Bonzini: > Il 12/06/2012 15:47, Kevin Wolf ha scritto: >> copy_sectors() always uses the sum (cluster_offset + n_start) or >> (start_sect + n_start), so if some value is added to both cluster_offset >> and start_sect, and subtracted from n_start, it's cancelled out anyway. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block/qcow2-cluster.c | 5 ++--- >> 1 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index 9aee9fc..763b724 100644 >> --- a/block/qcow2-cluster.c >> +++ b/block/qcow2-cluster.c >> @@ -640,11 +640,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) >> } >> >> if (m->nb_available & (s->cluster_sectors - 1)) { >> - uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1); >> cow = true; >> qemu_co_mutex_unlock(&s->lock); >> - ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9), >> - m->nb_available - end, s->cluster_sectors); >> + ret = copy_sectors(bs, start_sect, cluster_offset, >> + m->nb_available, s->cluster_sectors); > > Do you need to add end to s->cluster_sectors too, so that "start_sect + > n_end" and "n_end - n_start" remain the same? You mean because n_end is now relative to start_sect instead of start_sect + end, right? I thought about it and I find this code is a bit confusing, but I think you're right that I need to replace n_end as well because it would be wrong for an allocating request than spans multiple clusters. I think this one should be right, would you agree? ret = copy_sectors(bs, start_sect, cluster_offset, m->nb_available, align_offset(m->nb_available, s->cluster_sectors)); The interesting question is why qemu-iotests doesn't catch it. Kevin
Il 12/06/2012 16:21, Kevin Wolf ha scritto: >>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >>> index 9aee9fc..763b724 100644 >>> --- a/block/qcow2-cluster.c >>> +++ b/block/qcow2-cluster.c >>> @@ -640,11 +640,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) >>> } >>> >>> if (m->nb_available & (s->cluster_sectors - 1)) { >>> - uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1); >>> cow = true; >>> qemu_co_mutex_unlock(&s->lock); >>> - ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9), >>> - m->nb_available - end, s->cluster_sectors); >>> + ret = copy_sectors(bs, start_sect, cluster_offset, >>> + m->nb_available, s->cluster_sectors); >> >> Do you need to add end to s->cluster_sectors too, so that "start_sect + >> n_end" and "n_end - n_start" remain the same? > > You mean because n_end is now relative to start_sect instead of > start_sect + end, right? Yes. Or more simply, because I was expecting no other uses of start_sect, cluster_offset and n_start after reading your commit message. :) > I thought about it and I find this code is a bit confusing, but I think > you're right that I need to replace n_end as well because it would be > wrong for an allocating request than spans multiple clusters. I think > this one should be right, would you agree? > > ret = copy_sectors(bs, start_sect, cluster_offset, > m->nb_available, align_offset(m->nb_available, s->cluster_sectors)); The obvious expression would be s->cluster_sectors + (m->nb_available & ~(uint64_t)(s->cluster_sectors - 1)) which is a bit different from align_offset. If m->nb_available is already aligned, it returns the *next* aligned value rather than m->nb_available itself. So the equivalent expression using align_offset would be this one: align_offset(m->nb_available+1, s->cluster_sectors) Paolo
Am 12.06.2012 16:31, schrieb Paolo Bonzini: > Il 12/06/2012 16:21, Kevin Wolf ha scritto: >>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >>>> index 9aee9fc..763b724 100644 >>>> --- a/block/qcow2-cluster.c >>>> +++ b/block/qcow2-cluster.c >>>> @@ -640,11 +640,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) >>>> } >>>> >>>> if (m->nb_available & (s->cluster_sectors - 1)) { >>>> - uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1); >>>> cow = true; >>>> qemu_co_mutex_unlock(&s->lock); >>>> - ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9), >>>> - m->nb_available - end, s->cluster_sectors); >>>> + ret = copy_sectors(bs, start_sect, cluster_offset, >>>> + m->nb_available, s->cluster_sectors); >>> >>> Do you need to add end to s->cluster_sectors too, so that "start_sect + >>> n_end" and "n_end - n_start" remain the same? >> >> You mean because n_end is now relative to start_sect instead of >> start_sect + end, right? > > Yes. Or more simply, because I was expecting no other uses of > start_sect, cluster_offset and n_start after reading your commit message. :) > >> I thought about it and I find this code is a bit confusing, but I think >> you're right that I need to replace n_end as well because it would be >> wrong for an allocating request than spans multiple clusters. I think >> this one should be right, would you agree? >> >> ret = copy_sectors(bs, start_sect, cluster_offset, >> m->nb_available, align_offset(m->nb_available, s->cluster_sectors)); > > The obvious expression would be > > s->cluster_sectors > + (m->nb_available & ~(uint64_t)(s->cluster_sectors - 1)) > > which is a bit different from align_offset. If m->nb_available is > already aligned, it returns the *next* aligned value rather than > m->nb_available itself. > > So the equivalent expression using align_offset would be this one: > > align_offset(m->nb_available+1, s->cluster_sectors) Heh, yes, you're thinking about equivalence of the very formula (which is what I would do in unknown code as well), whereas I think about a COW operation that ranges from a given sector to the end of the same cluster (and not the next one). It's eventually the same, because this statement is only executed when it's not aligned (i.e. the COW range isn't empty): if (m->nb_available & (s->cluster_sectors - 1)) { ... } Kevin
Il 12/06/2012 16:37, Kevin Wolf ha scritto: >> > align_offset(m->nb_available+1, s->cluster_sectors) > Heh, yes, you're thinking about equivalence of the very formula (which > is what I would do in unknown code as well), whereas I think about a COW > operation that ranges from a given sector to the end of the same cluster > (and not the next one). > > It's eventually the same, because this statement is only executed when > it's not aligned (i.e. the COW range isn't empty): > > if (m->nb_available & (s->cluster_sectors - 1)) { > ... > } Indeed, your version is ok. Paolo
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 9aee9fc..763b724 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -640,11 +640,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) } if (m->nb_available & (s->cluster_sectors - 1)) { - uint64_t end = m->nb_available & ~(uint64_t)(s->cluster_sectors - 1); cow = true; qemu_co_mutex_unlock(&s->lock); - ret = copy_sectors(bs, start_sect + end, cluster_offset + (end << 9), - m->nb_available - end, s->cluster_sectors); + ret = copy_sectors(bs, start_sect, cluster_offset, + m->nb_available, s->cluster_sectors); qemu_co_mutex_lock(&s->lock); if (ret < 0) goto err;
copy_sectors() always uses the sum (cluster_offset + n_start) or (start_sect + n_start), so if some value is added to both cluster_offset and start_sect, and subtracted from n_start, it's cancelled out anyway. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-cluster.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)