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

[Josh Durgin <josh.durgin@inktank.com>]

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