Re: [Qemu-devel] [RFC 4/6] block: request overlap detection

[Stefan Hajnoczi]

On Tue, Nov 08, 2011 at 02:34:22PM +0800, Zhi Yong Wu wrote:
> On Mon, Nov 7, 2011 at 10:37 PM, Stefan Hajnoczi <address@hidden> wrote:
> > On Mon, Nov 7, 2011 at 11:49 AM, Zhi Yong Wu <address@hidden> wrote:
> >> On Mon, Oct 17, 2011 at 11:47 PM, Stefan Hajnoczi
> >> <address@hidden> wrote:
> >>> Detect overlapping requests and remember to align to cluster boundaries
> >>> if the image format uses them.  This assumes that allocating I/O is
> >>> performed in cluster granularity - which is true for qcow2, qed, etc.
> >>>
> >>> Signed-off-by: Stefan Hajnoczi <address@hidden>
> >>> ---
> >>>  block.c |   39 +++++++++++++++++++++++++++++++++++++--
> >>>  1 files changed, 37 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/block.c b/block.c
> >>> index cc3202c..0c22741 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -1052,21 +1052,56 @@ static BdrvTrackedRequest 
> >>> *tracked_request_add(BlockDriverState *bs,
> >>>     return req;
> >>>  }
> >>>
> >>> +/**
> >>> + * Round a region to cluster boundaries
> >>> + */
> >>> +static void round_to_clusters(BlockDriverState *bs,
> >>> +                              int64_t sector_num, int nb_sectors,
> >>> +                              int64_t *cluster_sector_num,
> >>> +                              int *cluster_nb_sectors)
> >>> +{
> >>> +    BlockDriverInfo bdi;
> >>> +
> >>> +    if (bdrv_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) {
> >>> +        *cluster_sector_num = sector_num;
> >>> +        *cluster_nb_sectors = nb_sectors;
> >>> +    } else {
> >>> +        int64_t c = bdi.cluster_size / BDRV_SECTOR_SIZE;
> >>> +        *cluster_sector_num = (sector_num / c) * c;
> >>             I can understand the above formula, but the one below is
> >> very magic. :) and can not be understood by me.
> >>> +        *cluster_nb_sectors = ((sector_num % c) + nb_sectors + c - 1) / 
> >>> c * c;
> >
> > I agree this is ugly.  Here is what is going on:
> >
> > c = number of sectors per cluster
> > cluster_sector_num = sector number rounded *down* to cluster boundary
> > cluster_nb_sectors = number of sectors from cluster_sector_num to
> > rounded up sector_num+nb_sectors
> >
> > So the magic expression is takes the original sector_num to
> > sector_num+nb_sectors region:
> >
> > |---XXX|XXX---|
> >
> > Where |-----| is a cluster and XXXX is the region from sector_num to
> > sector_num+nb_sectors, then the output should be:
> >
> > |RRRRRR|RRRRRR|
> >
> > Everything has been rounded to clusters.  So here is the expression broken 
> > down:
> >
> > *cluster_nb_sectors = ((sector_num % c) + nb_sectors + c - 1) / c * c;
> >                        AAAAAAAAAAAAAA    XXXXXXXXXX   BBBBBBBBBBBBBB
> >
> > |AAAXXX|XXXBBB|
> >
> > A is actually equivalent to sector_num - cluster_sector_num.
> >
> > X is the original unrounded region.
> >
> > B is the rounding up to the next cluster bounary.
> >
> > Another way of writing this:
> >
> > *cluster_nb_sectors = ROUND_UP((sector_num - cluster_sector_num) +
> > nb_sectors, c);
> Above expression seems to not be correct;
> It should be
> *cluster_nb_sectors = ROUND_UP((sector_num - cluster_sector_num) +
> nb_sectors, c) * c;
> 
> *cluster_nb_sectors = ((sector_num % c) + nb_sectors + c - 1) / c * c;
> 
> #define ROUND_UP(x,y) (((x)+(y)-1)/(y))

ALIGN_UP() may be a better macro name, for example:

ALIGN_UP(1024, 4096) = 4096

I see how you're defining ROUND_UP() and it is different.

Stefan