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

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

[Josh Durgin]

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.

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 */
+			if (PTR_ERR(snap_name) == -ENOENT)
+				continue;
+			else
+				break;
+		}
 		found = !strcmp(name, snap_name);
 		kfree(snap_name);
 	}
-- 
1.7.2.5

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

[Alex Elder]

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);
>  	}
> 

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

[Josh Durgin]

On 09/03/2013 06:29 AM, Alex Elder wrote:
> 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...

This is 4/5 in the series I just posted. The caller needed to be fixed
too, unsurprisingly.

Josh

> 					-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);
>>   	}
>>
>

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

[Josh Durgin]

On 09/09/2013 12:31 AM, Josh Durgin wrote:
> On 09/03/2013 06:29 AM, Alex Elder wrote:
>> 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...
>
> This is 4/5 in the series I just posted. The caller needed to be fixed

Err, make that 5/5.

> too, unsurprisingly.
>
> Josh
>
>>                     -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);
>>>       }
>>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html