Re: [PATCH] rbd: ignore unmapped snapshots that no longer exist

[Alex Elder <alex.elder@linaro.org>]

On 08/29/2013 09:19 PM, Josh Durgin wrote:
> This prevents erroring out while adding a device when a snapshot
> unrelated to the current mapping is deleted between reading the
> snapshot context and reading the snapshot names. If the mapped
> snapshot name is not found an error still occurs as usual.

I mention something about a one of your comments, but even so
this looks good to me, so for this patch:

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

However, in looking at this, I found something else that probably
should be resolved.

Note that rbd_dev_spec_update() calls rbd_snap_name(), but it
only checks its return value for NULL.  This test needs to be
*at least* changed to test IS_ERR_OR_NULL(), but I think it
would be better to do the change I suggest next, and make this
spot just use IS_ERR().

Right now rbd_dev_v1_snap_name() just returns NULL if any error
occurs.  It should be made to return a pointer-coded-error like
its v2 counterpart function does.  It would look something like:

static const char *rbd_dev_v1_snap_name(struct rbd_device *rbd_dev,
                                        u64 snap_id)
{
        u32 which;
	const char *snap_name;

        which = rbd_dev_snap_index(rbd_dev, snap_id);
        if (which == BAD_SNAP_INDEX)
                return ERR_PTR(-ENOENT);

	snap_name = _rbd_dev_v1_snap_name(rbd_dev, which);

	return snap_name ? snap_name : ERR_PTR(-ENOMEM);
}

I'd make this change myself but at the moment I'm not set up
to be able to rigorously test it, so I'm just suggesting it
to you and hoping for the best...

					-Alex


> Signed-off-by: Josh Durgin <josh.durgin@inktank.com>
> ---
>  drivers/block/rbd.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 0e83a10..545ceff 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -4077,8 +4077,13 @@ static u64 rbd_v2_snap_id_by_name(struct rbd_device *rbd_dev, const char *name)
>  
>  		snap_id = snapc->snaps[which];
>  		snap_name = rbd_dev_v2_snap_name(rbd_dev, snap_id);
> -		if (IS_ERR(snap_name))
> -			break;
> +		if (IS_ERR(snap_name)) {
> +			/* ignore no-longer existing snapshots */

I feel like this comment doesn't really capture why it might be
normal and acceptable for a snapshot to no longer exist.  (At
least mention that it would be due to a race?)

> +			if (PTR_ERR(snap_name) == -ENOENT)
> +				continue;
> +			else
> +				break;
> +		}
>  		found = !strcmp(name, snap_name);
>  		kfree(snap_name);
>  	}
>