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 |