Re: [Qemu-devel] [PATCH 17/19] block/parallels: delay writing to BAT till bdrv_co_flush_to_os

["Denis V. Lunev" <den@openvz.org>]

On 14/01/15 16:34, Roman Kagan wrote:
> On Wed, Jan 14, 2015 at 04:08:50PM +0300, Denis V. Lunev wrote:
>> On 14/01/15 16:03, Roman Kagan wrote:
>>> On Tue, Dec 30, 2014 at 01:07:10PM +0300, Denis V. Lunev wrote:
>>>> +static int cache_bat(BlockDriverState *bs, uint32_t idx, uint32_t new_data_off)
>>>> +{
>>>> +    int ret, i, off, cache_off;
>>>> +    int64_t first_idx, last_idx;
>>>> +    BDRVParallelsState *s = bs->opaque;
>>>> +    uint32_t *cache = s->bat_cache;
>>>> +
>>>> +    off = bat_offset(idx);
>>>> +    cache_off = (off / s->bat_cache_size) * s->bat_cache_size;
>>>> +
>>>> +    if (s->bat_cache_off != -1 && s->bat_cache_off != cache_off) {
>>>> +        ret = write_bat_cache(bs);
>>>> +        if (ret < 0) {
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    first_idx = idx - (off - cache_off) / sizeof(uint32_t);
>>>> +    last_idx = first_idx + s->bat_cache_size / sizeof(uint32_t);
>>>> +    if (first_idx < 0) {
>>>> +        memcpy(s->bat_cache, &s->ph, sizeof(s->ph));
>>>> +        first_idx = 0;
>>>> +        cache = s->bat_cache + sizeof(s->ph) / sizeof(uint32_t);
>>>> +    }
>>>> +
>>>> +    if (last_idx > s->bat_size) {
>>>> +        memset(cache + s->bat_size - first_idx, 0,
>>>> +               sizeof(uint32_t) * (last_idx - s->bat_size));
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < last_idx - first_idx; i++) {
>>>> +        cache[i] = cpu_to_le32(s->bat[first_idx + i]);
>>>> +    }
>>> You're re-populating the bat_cache on every bat update.  Why?
>>>
>>> Shouldn't this be done only with empty cache, i.e. under
>>> if (s->bat_cache_off == -1)?
>> reasonable, but condition should be a bit different, namely
>>
>> if (s->bat_cache_off != -1 && s->bat_cache_off != cache_off) {
> No, this is the condition to flush the cache which you already do.
>
> Then, either upon flushing it or on the first entry into cache_bat(),
> the cache is empty which is indicated by ->bat_cache_off == -1, at which
> point you need to populate it from ->bat.

you are wrong. BAT cache is a single page of the whole BAT
which can be more than several MBs. This page should be
repopulated when the off is changed.