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.
