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

["Denis V. Lunev" <den@parallels.com>]

On 14/01/15 17:29, Denis V. Lunev wrote:
> 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.
ok, you are spoken about write_bat_cache which effectively marks
cache as invalid. Thus we are both spoken about the same
condition and we are on the same page.