On Mon, 27 Oct 2008 12:55:38 -0400
Konrad Rzeszutek <konrad@xxxxxxxxxxxxxxx> 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,
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
