* [PATCH] rbd: ignore unmapped snapshots that no longer exist @ 2013-08-30 2:19 Josh Durgin 2013-09-03 13:29 ` Alex Elder 0 siblings, 1 reply; 4+ messages in thread From: Josh Durgin @ 2013-08-30 2:19 UTC (permalink / raw) To: ceph-devel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] rbd: ignore unmapped snapshots that no longer exist 2013-08-30 2:19 [PATCH] rbd: ignore unmapped snapshots that no longer exist Josh Durgin @ 2013-09-03 13:29 ` Alex Elder 2013-09-09 7:31 ` Josh Durgin 0 siblings, 1 reply; 4+ messages in thread From: Alex Elder @ 2013-09-03 13:29 UTC (permalink / raw) To: Josh Durgin; +Cc: ceph-devel 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); > } > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rbd: ignore unmapped snapshots that no longer exist 2013-09-03 13:29 ` Alex Elder @ 2013-09-09 7:31 ` Josh Durgin 2013-09-09 7:32 ` Josh Durgin 0 siblings, 1 reply; 4+ messages in thread From: Josh Durgin @ 2013-09-09 7:31 UTC (permalink / raw) To: Alex Elder; +Cc: ceph-devel 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); >> } >> > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rbd: ignore unmapped snapshots that no longer exist 2013-09-09 7:31 ` Josh Durgin @ 2013-09-09 7:32 ` Josh Durgin 0 siblings, 0 replies; 4+ messages in thread From: Josh Durgin @ 2013-09-09 7:32 UTC (permalink / raw) To: Alex Elder; +Cc: ceph-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-09 7:32 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-30 2:19 [PATCH] rbd: ignore unmapped snapshots that no longer exist Josh Durgin 2013-09-03 13:29 ` Alex Elder 2013-09-09 7:31 ` Josh Durgin 2013-09-09 7:32 ` Josh Durgin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.