On Thu, Feb 14, 2013 at 04:45:06PM +0100, Kevin Wolf wrote: > On Thu, Feb 14, 2013 at 03:48:51PM +0100, Stefan Hajnoczi wrote: > > On Wed, Feb 13, 2013 at 02:21:53PM +0100, Kevin Wolf wrote: > > > + * > > > + * -EAGAIN if we had to wait for another request, previously gathered > > > + * information on cluster allocation may be invalid now. The > > > caller > > > + * must start over anyway, so consider *cur_bytes undefined. > > > */ > > > static int handle_dependencies(BlockDriverState *bs, uint64_t > > > guest_offset, > > > - unsigned int *nb_clusters) > > > + uint64_t *cur_bytes) > > > { > > > BDRVQcowState *s = bs->opaque; > > > QCowL2Meta *old_alloc; > > > + uint64_t bytes = *cur_bytes; > > > > > > QLIST_FOREACH(old_alloc, &s->cluster_allocs, next_in_flight) { > > > > > > - uint64_t start = guest_offset >> s->cluster_bits; > > > - uint64_t end = start + *nb_clusters; > > > - uint64_t old_start = old_alloc->offset >> s->cluster_bits; > > > - uint64_t old_end = old_start + old_alloc->nb_clusters; > > > + uint64_t start = guest_offset; > > > > I'm not sure this is safe. Previously the function caught write > > requests which overlap at cluster granularity, now it will allow them? > > At this point in the series, old_start and old_end are still aligned to > cluster boundaries. So the cases in which an overlap is detected stay exactly > the same with this patch. This is just a more precise description of what we > really wanted to compare.
Good point. In an overlap comparison between A and B only one of them needs to be cluster-aligned. I've used this trick myself in the past but missed it in this code review. Stefan