On Wed, Dec 12, 2007 at 12:12:47PM +0300, Dmitry Monakhov (dmonakhov@xxxxx) wrote:
> On 14:47 Mon 10 Dec , Evgeniy Polyakov wrote:
> >
> > Algorithms used in distributed storage.
> > Mirror and linear mapping code.
> Hi, i've finally take a look on your DST solution.
> It seems what your current implementation will not work on nonstandard
> devices for example software raid0.
> other comments are follows:
> > +static int dst_mirror_process_node_data(struct dst_node *n,
> > + struct dst_mirror_node_data *ndata, int op)
> > +
> > + kunmap(cmp->page);
> << MINOR_BUG:
> You has forgot to unmap page on error path, so IMHO it is better to move
> kunmap to "err_out_free_cmp" label.
Yep, I will fix this.
> > + priv = kzalloc(sizeof(struct dst_mirror_priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->chunk_num = st->disk_size;
> > +
> > + priv->chunk = vmalloc(DIV_ROUND_UP(priv->chunk_num, BITS_PER_LONG) * sizeof(long));
> << Ohhh. My. I want to add my 500G hdd. Do you really wanna
> say what i have to store 128Mb in memory object for this.
Right now yes. There was a code which used single bit for bigger
data units, but I dropped it because of resync troubles (i.e. when
one single sector has been updated, it requires to resync the whole
block). I can not say which case is better though.
> > + dprintk("%s: start: %llu, size: %llu/%u, bio: %p, req: %p, "
> > + "node: %p.\n",
> > + __func__, req->start, req->size, nr_pages, bio,
> > + req, req->node);
> > +
> > + err = n->st->queue->make_request_fn(n->st->queue, bio);
> << Why direct make_request_fn instead of generic_make_request?
generic_make_request() will queue the bio in this case,
so I call request_fn directly.
> > + for (i = 0; i < DIV_ROUND_UP(priv->chunk_num, BITS_PER_LONG); ++i) {
> > + int bit, num, start;
> > + unsigned long word = priv->chunk[i];
> > +
> > + if (!word)
> > + continue;
> > +
> > + num = 0;
> > + start = -1;
> > + while (word && num < BITS_PER_LONG) {
> > + bit = __ffs(word);
> > + if (start == -1)
> > + start = bit;
> > + num++;
> << MINOR_BUG: Seems you have misstyped here. AFAIU @num represent position
> of last non zero bit (start + num == last_non_zero_bit_pos)
> if (start == -1) {
> start = bit;
> num = 1;
> } else
> num += bit;
Yes, you are right of course.
Since I shift word to more than a single bit, @num has to be update
accordingly.
> > + word >>= (bit+1);
Dmitry, thanks a lot for comments, I will fix issues you pointed in the
next release, although will stay bitmap case opened for a while.
--
Evgeniy Polyakov
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
