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

[FUJITA Tomonori]

On Mon, 27 Oct 2008 12:55:38 -0400
Konrad Rzeszutek <konrad at virtualiron.com> wrote:

> 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:

Thanks for the comments.

> ... 
> > +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?

nr_chunks is the total number of chunks. What I try to do here is to
calculate the number of chunks that the last bitmap chunk holds.

I guess that it's better to do something simpler like this:

if (ps->cur_bitmap_chunk == ps->nr_bitmap_chunks)
        limit = ps->nr_chunks % nr;
else
	limit = nr;


> > +	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?

Yeah, '<< SECTOR_SHIFT' is better. It will be fixed at the next round.


Thanks,