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

[Zhi Yong Wu]

On Tue, Nov 8, 2011 at 4:16 PM, Stefan Hajnoczi
<address@hidden> wrote:
> 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
OK. Hope to see this in your next revision.
>
> I see how you're defining ROUND_UP() and it is different.
>
> Stefan
>



-- 
Regards,

Zhi Yong Wu