Am 06.12.2011 17:35, schrieb Marcelo Tosatti: > On Tue, Dec 06, 2011 at 04:20:55PM +0100, Kevin Wolf wrote: >> Am 06.12.2011 16:06, schrieb Marcelo Tosatti: >>> On Tue, Dec 06, 2011 at 12:53:16PM -0200, Marcelo Tosatti wrote: >>>> On Tue, Dec 06, 2011 at 01:59:48PM +0100, Kevin Wolf wrote: >>>>>>> + >>>>>>> + ret = bdrv_pread(bs->file, sizeof(header), state->bitmap, >>>>>>> + state->bitmap_size); >>>>>>> + if (ret != state->bitmap_size) { >>>>>>> + goto fail; >>>>>>> + } >>>>>> >>>>>> Reading the entire bitmap in memory is not acceptable, it may be huge. >>>>>> Better mmap it and use msync(MS_SYNC) when writing it back. This way the >>>>>> host can free memory easily upon pressure. >>>>> >>>>> You can't use mmap in block drivers. It would only work with raw-posix >>>>> backends, if at all. >>>>> >>>>> Kevin >>>> >>>> This is just the bitmap, a plain file. Why would you want to use >>>> anything other than a plain file to use as storage for the bitmap? >>> >>> Well, mmap'ing would make life much simpler, but it has limitations such >>> as portability. >>> >>> Then what is necessary is a cache similar to qcow2's metadata cache. >> >> Right, we can probably generalise the qcow2 code and make it available >> for other drivers as well. > > Hum, generalising sounds overly complicated (and there is a time > constraint to this). IMHO a cache internal to add-cow.c just to avoid > reading the entire bitmap would do the trick.
The cache is mostly self-contained. But maybe we should get the locking right (instead of always locking the whole BlockDriverState) before using it in more drivers. I think this might need some change to it. Kevin