From mboxrd@z Thu Jan 1 00:00:00 1970 From: Josh Durgin Subject: Re: [PATCH] rbd: ignore unmapped snapshots that no longer exist Date: Mon, 09 Sep 2013 00:32:26 -0700 Message-ID: <522D798A.4090507@inktank.com> References: <1377829140-13635-1-git-send-email-josh.durgin@inktank.com> <5225E453.1080403@linaro.org> <522D7953.70909@inktank.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-oa0-f42.google.com ([209.85.219.42]:40392 "EHLO mail-oa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751001Ab3IIHca (ORCPT ); Mon, 9 Sep 2013 03:32:30 -0400 Received: by mail-oa0-f42.google.com with SMTP id n12so6259376oag.15 for ; Mon, 09 Sep 2013 00:32:30 -0700 (PDT) In-Reply-To: <522D7953.70909@inktank.com> Sender: ceph-devel-owner@vger.kernel.org List-ID: To: Alex Elder Cc: ceph-devel@vger.kernel.org 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 >> >> 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 >>> --- >>> 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