From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56586) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBR3t-00056T-GM for qemu-devel@nongnu.org; Wed, 14 Jan 2015 11:44:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YBR3n-0003QL-Sc for qemu-devel@nongnu.org; Wed, 14 Jan 2015 11:44:53 -0500 Received: from mx2.parallels.com ([199.115.105.18]:42137) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBR3n-0003Q0-Mg for qemu-devel@nongnu.org; Wed, 14 Jan 2015 11:44:47 -0500 Message-ID: <54B69CF1.6020802@parallels.com> Date: Wed, 14 Jan 2015 19:44:33 +0300 From: "Denis V. Lunev" 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> <54B67D58.3060806@openvz.org> In-Reply-To: <54B67D58.3060806@openvz.org> 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: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Kagan , Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi 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.