[dm-devel] [PATCH 7/9] dm snapshot: add shared exception store

[Konrad Rzeszutek]

On Mon, Oct 27, 2008 at 09:07:54PM +0900, FUJITA Tomonori wrote:
> This adds a new exception store implementation, shared exception
> store. The important design differences from the current two exception
> store implementations:
> 
> - It uses one exception store (cow disk) per origin device that is
>   shared by all snapshots.
> - It doesn't keep the complete exception tables in memory.
> 
> The shared exception store uses struct pstore, which the persistent
> exception store uses because there are many useful functions that
> struct pstore works with. The shared exception adds some variants to
> struct pstore, but not many.

Thank you for posting this. I've just two questions:

... 
> +static unsigned long shared_allocate_chunk(struct pstore *ps)
> +{
> +	unsigned long idx;
> +	unsigned long limit;
> +	unsigned long start_chunk;
> +	unsigned long nr = (ps->snap->chunk_size << SECTOR_SHIFT) * 8;
> +
> +	start_chunk = ps->cur_bitmap_chunk;
> +again:
> +	if (ps->cur_bitmap_chunk == ps->nr_bitmap_chunks)
> +		limit = ps->nr_chunks - (nr * (ps->nr_bitmap_chunks - 1));

Is this correct? If the chunk size is 16 the nr_chunks would be the bit-shift
value (5). Hence you are subtracting 8192*(some positivie number)-1 from 5 -
is that what you intend to do?

> +	else
> +		limit = nr;
> +
> +	idx = find_next_zero_bit(ps->bitmap, limit, ps->cur_bitmap_index);
> +	if (idx < limit) {
> +		set_bit(idx, ps->bitmap);
> +
> +		if (idx == limit - 1) {
> +			ps->cur_bitmap_index = 0;
> +
> +			read_new_bitmap_chunk(ps);
> +		} else
> +			ps->cur_bitmap_index++;
> +	} else {
> +		chunk_io(ps, ps->cur_bitmap_chunk, WRITE, 1, ps->bitmap);
> +
> +		read_new_bitmap_chunk(ps);
> +
> +		/* todo: check # free chunks */
> +		if (start_chunk == ps->cur_bitmap_chunk) {
> +			DMERR("%s %d: fail to find a new chunk",
> +			      __func__, __LINE__);
> +			return 0;
> +		}
> +
> +		goto again;
> +	}
> +
> +	return idx + (ps->cur_bitmap_chunk - FIRST_BITMAP_CHUNK) * nr;

.. snip..
> +static int shared_create_bitmap(struct exception_store *store)
> +{
> +	struct pstore *ps = get_info(store);
> +	int i, r, rest, this;
> +	chunk_t chunk;
> +
> +	/* bitmap + superblock */
> +	rest = ps->nr_bitmap_chunks + 1;
> +
> +	for (chunk = 0; chunk < ps->nr_bitmap_chunks; chunk++) {
> +		memset(ps->area, 0, ps->snap->chunk_size << SECTOR_SHIFT);
> +
> +		this = min_t(int, rest, ps->snap->chunk_size * 512 * 8);

512? Why not do the << SECTOR_SHIFT as in the other places?

> +		if (this) {
> +			rest -= this;
> +
> +			memset(ps->area, 0xff, this / 8);
> +
> +			for (i = 0; i < this % 8; i++)
> +				set_bit(i, (unsigned long *)ps->area + this / 8);
> +		}
> +
> +		r = chunk_io(ps, chunk + FIRST_BITMAP_CHUNK, WRITE, 1,
> +			     ps->area);
> +		if (r)
> +			return r;
> +	}
> +
> +	return 0;