| Message ID | 1377829140-13635-1-git-send-email-josh.durgin@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > } > -- 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
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); >> } >> > -- 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
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 -- 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
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); }
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(-)