From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:50386)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1YBOxJ-0000mF-ID
	for qemu-devel@nongnu.org; Wed, 14 Jan 2015 09:29:58 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1YBOxG-0005xB-7B
	for qemu-devel@nongnu.org; Wed, 14 Jan 2015 09:29:57 -0500
Received: from mx2.parallels.com ([199.115.105.18]:42542)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <den@openvz.org>) id 1YBOxG-0005x4-18
	for qemu-devel@nongnu.org; Wed, 14 Jan 2015 09:29:54 -0500
Message-ID: <54B67D58.3060806@openvz.org>
Date: Wed, 14 Jan 2015 17:29:44 +0300
From: "Denis V. Lunev" <den@openvz.org>
MIME-Version: 1.0
References: <1419931701-19362-1-git-send-email-den@openvz.org>
	<1419934032-24216-1-git-send-email-den@openvz.org>
	<1419934032-24216-8-git-send-email-den@openvz.org>
	<20150114130339.GB27418@rkaganb.sw.ru>
	<54B66A62.1050202@openvz.org>
	<20150114133447.GC27418@rkaganb.sw.ru>
In-Reply-To: <20150114133447.GC27418@rkaganb.sw.ru>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 17/19] block/parallels: delay writing to
 BAT till bdrv_co_flush_to_os
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Roman Kagan <rkagan@parallels.com>, Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>

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.