Re: [PATCH 8/8] rbd: take snap_id into account when reading in parent info

[Alex Elder <elder@ieee.org>]

On 07/24/2014 03:42 AM, Ilya Dryomov wrote:
> If we are mapping a snapshot, we must read in the parent_overlap value
> of that snapshot instead of that of the base image.  Not doing so may
> in particular result in us returning zeros instead of user data:
> 
>     # cat overlap-snap.sh
>     #!/bin/bash
>     rbd create --size 10 --image-format 2 foo
>     FOO_DEV=$(rbd map foo)
>     dd if=/dev/urandom of=/dev/rbd0 bs=1M &>/dev/null
>     echo "Base image"
>     dd if=$FOO_DEV bs=1 count=16 skip=$(((4 << 20) - 8)) 2>/dev/null | xxd
>     rbd snap create foo@snap
>     rbd snap protect foo@snap
>     rbd clone foo@snap bar
>     rbd snap create bar@snap
>     BAR_DEV=$(rbd map bar@snap)
>     echo "Snapshot"
>     dd if=$BAR_DEV bs=1 count=16 skip=$(((4 << 20) - 8)) 2>/dev/null | xxd
>     rbd resize --allow-shrink --size 4 bar
>     echo "Snapshot after base image resize"
>     dd if=$BAR_DEV bs=1 count=16 skip=$(((4 << 20) - 8)) 2>/dev/null | xxd
> 
>     # ./overlap-snap.sh
>     Base image
>     0000000: e781 e33b d34b 2225 6034 2845 a2e3 36ed  ...;.K"%`4(E..6.
>     Snapshot
>     0000000: e781 e33b d34b 2225 6034 2845 a2e3 36ed  ...;.K"%`4(E..6.
>     Resizing image: 100% complete...done.
>     Snapshot after base image resize
>     0000000: e781 e33b d34b 2225 0000 0000 0000 0000  ...;.K"%........
> 
> Even though bar@snap was taken with the old bar parent_overlap (8M),
> reads from bar@snap beyond the new bar parent_overlap (4M) return
> zeroes.  Fix it.
> 
> Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com>
> ---
>  drivers/block/rbd.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index c4606987e9d1..cbc89fa9a677 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -4020,7 +4020,7 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
>  		goto out_err;
>  	}
>  
> -	snapid = cpu_to_le64(CEPH_NOSNAP);
> +	snapid = cpu_to_le64(rbd_dev->spec->snap_id);

Well that's just an outright bug.  It's been there since the
original commit that added parent support:
    86b00e0 rbd: get parent spec for version 2 images
Parent images *must* be snapshots, so this was never
right.

I bet that was hard to figure out...

Looks good.

Reviewed-by: Alex Elder <elder@linaro.org>


>  	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
>  				"rbd", "get_parent",
>  				&snapid, sizeof (snapid),
>