On 04/29/2012 07:49 PM, Christoph Hellwig wrote: > We can only use inode->nr_copies as a lower ceiling for the number of copies > once inode actually is read in. This fixes a merge error and makes sure this > code behaves the same way as before "sheep: cleanup nr_copies handling". > > Signed-off-by: Christoph Hellwig <hch at lst.de> > > diff --git a/sheep/vdi.c b/sheep/vdi.c > index 0788f26..3885384 100644 > --- a/sheep/vdi.c > +++ b/sheep/vdi.c > @@ -457,8 +457,6 @@ static void delete_one(struct work *work) > } > > 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), > @@ -469,6 +467,9 @@ static void delete_one(struct work *work) > goto out; > } > > + if (nr_copies > inode->nr_copies) > + nr_copies = inode->nr_copies; > + > for (i = 0; i < MAX_DATA_OBJS; i++) { > if (!inode->data_vdi_id[i]) > continue; Take a look up your previous patch: @@ -454,9 +456,13 @@ static void delete_one(struct work *work) goto out; } + 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); if (ret != SD_RES_SUCCESS) { eprintf("cannot find VDI object\n"); @@ -475,7 +481,7 @@ static void delete_one(struct work *work) ret = remove_object(dw->vnodes, dw->epoch, vid_to_data_oid(inode->data_vdi_id[i], i), - inode->nr_copies); + nr_copies); if (ret != SD_RES_SUCCESS) dw->delete_error = 1; @@ -485,8 +491,7 @@ static void delete_one(struct work *work) if (dw->delete_error) { write_object(dw->vnodes, dw->epoch, vid_to_vdi_oid(vdi_id), - (void *)inode, sizeof(*inode), 0, 0, - inode->nr_copies, 0); + (void *)inode, sizeof(*inode), 0, 0, nr_copies, 0); } 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. You don't document why you replace inode->nr_copies with nr_copies. Your later patch just paper over this problem and claim you restores to the previous logic with a clumsy patch, but actually you don't really do it. Besides, with more test, your patch series simply breaks sheep even with very easy setup. I was really surprised and I am of course the one to blame to merge these broken patches. However, please do some basic setup even you are confident with your simple patches next time. I am going to cook up a patch set to address all the regressions Thanks, Yuan |