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

[Roman Kagan]

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.

> >>  static void parallels_close(BlockDriverState *bs)
> >>  {
> >>      BDRVParallelsState *s = bs->opaque;
> >>+    qemu_vfree(s->bat_cache);
> >Don't you need to flush the bat cache here first?
> 
> parallels_co_flush_to_os should happen beforehand
> 
> 
> void bdrv_close(BlockDriverState *bs)
> {
>     ....
>     bdrv_flush(bs); <-- namely inside this

Indeed.

Roman.