On Wed, Dec 12, 2007 at 12:12:47PM +0300, Dmitry Monakhov ([EMAIL PROTECTED]) 
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-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to