[Sheepdog] [PATCH] sheep: fix nr_copies calculation in delete_one

[MORITA Kazutaka]

At Tue, 1 May 2012 11:42:24 -0400,
Christoph Hellwig wrote:
> 
> On Mon, Apr 30, 2012 at 03:54:55PM +0800, Liu Yuan wrote:
> > +       nr_copies = get_nr_copies(dw->vnodes);
> > +       if (nr_copies > inode->nr_copies)
> > +               nr_copies = inode->nr_copies;
> > +
> >         ret = read_object(dw->vnodes, dw->epoch, vid_to_vdi_oid(vdi_id),
> >                           (void *)inode, sizeof(*inode),
> > -                         0, sys->nr_sobjs);
> > +                         0, nr_copies);
> 
> > I think it is really kind of mess and upset me. If we don't get a
> > overall understanding of the code, we shouldn't change the old code
> > WITHOUT a good reason.
> 
> I think the change very much makese sense modulo the bug fixed in this
> patch.
> 
> Let's get back and think about how nr_copies handling works now and
> should work.
> 
> On the VDI level:
> 
>  1) the user specifies a nr_copies level in the vdi creation call
>  2) we need to limit by nr_zones as an upper ceiling.   If the limit
>     is not handled we could either
> 
> 	a) refuse the creation or 
> 	b) only create with the higher number set in the inode, but
>            limiting the actual I/O later on
> 
>  3) all other VDI level I/O should use inode->nr_vnodes and limit it
>     by nr_zones
> 
> The big question is if we want to limit it by sys->nr_copies too, my
> patch did that because it makes little sense to create an object with
> more redudancy if system objects like the inode don't have it.  I think

If nr_copies has a larger value than inode->nr_copies, it should be a
bug.


> the big failure was to not document it.  We could also allow higher
> redundancy, but I don't see a real use case and it makes the code
> harder.

Ideally, sys->nr_copies should be used only when setting
inode->nr_copies in the case the user doesn't specify it.
So I think allowing higher redundancy doesn't make much difference
since sheep doesn't need to know the default redundancy level when
processing I/Os, no?

> 
> Now for the actual data I/O we expect the client to set hdr.copies
> from inode->nr_copies, which the client does, but we don't really
> respect it in sheep right now except for a single place in
> forward_read_obj_req.

I completely agree with changing the code to use hdr.copies.


Thanks,

Kazutaka