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

[Liu Yuan]

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