| Message ID | 20191118133816.3963-8-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | wip-krbd-readonly | expand |
On 11/18/2019 09:38 PM, Ilya Dryomov wrote: > RBD_DEV_FLAG_EXISTS check in rbd_queue_workfn() is racy and leads to > inconsistent behaviour. If the object (or its snapshot) isn't there, > the OSD returns ENOENT. A read submitted before the snapshot removal > notification is processed would be zero-filled and ended with status > OK, while future reads would be failed with IOERR. It also doesn't > handle a case when an image that is mapped read-only is removed. > > On top of this, because watch is no longer established for read-only > mappings, we no longer get notifications, so rbd_exists_validate() is > effectively dead code. While failing requests rather than returning > zeros is a good thing, RBD_DEV_FLAG_EXISTS is not it. > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn> > --- > drivers/block/rbd.c | 42 +++--------------------------------------- > 1 file changed, 3 insertions(+), 39 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index bfff195e8e23..aba60e37b058 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -462,7 +462,7 @@ struct rbd_device { > * by rbd_dev->lock > */ > enum rbd_dev_flags { > - RBD_DEV_FLAG_EXISTS, /* mapped snapshot has not been deleted */ > + RBD_DEV_FLAG_EXISTS, /* rbd_dev_device_setup() ran */ > RBD_DEV_FLAG_REMOVING, /* this mapping is being removed */ > RBD_DEV_FLAG_READONLY, /* -o ro or snapshot */ > }; > @@ -4848,19 +4848,6 @@ static void rbd_queue_workfn(struct work_struct *work) > rbd_assert(!rbd_is_snap(rbd_dev)); > } > > - /* > - * Quit early if the mapped snapshot no longer exists. It's > - * still possible the snapshot will have disappeared by the > - * time our request arrives at the osd, but there's no sense in > - * sending it if we already know. > - */ > - if (!test_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags)) { > - dout("request for non-existent snapshot"); > - rbd_assert(rbd_is_snap(rbd_dev)); > - result = -ENXIO; > - goto err_rq; > - } > - > if (offset && length > U64_MAX - offset + 1) { > rbd_warn(rbd_dev, "bad request range (%llu~%llu)", offset, > length); > @@ -5040,25 +5027,6 @@ static int rbd_dev_v1_header_info(struct rbd_device *rbd_dev) > return ret; > } > > -/* > - * Clear the rbd device's EXISTS flag if the snapshot it's mapped to > - * has disappeared from the (just updated) snapshot context. > - */ > -static void rbd_exists_validate(struct rbd_device *rbd_dev) > -{ > - u64 snap_id; > - > - if (!test_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags)) > - return; > - > - snap_id = rbd_dev->spec->snap_id; > - if (snap_id == CEPH_NOSNAP) > - return; > - > - if (rbd_dev_snap_index(rbd_dev, snap_id) == BAD_SNAP_INDEX) > - clear_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags); > -} > - > static void rbd_dev_update_size(struct rbd_device *rbd_dev) > { > sector_t size; > @@ -5099,12 +5067,8 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev) > goto out; > } > > - if (!rbd_is_snap(rbd_dev)) { > - rbd_dev->mapping.size = rbd_dev->header.image_size; > - } else { > - /* validate mapped snapshot's EXISTS flag */ > - rbd_exists_validate(rbd_dev); > - } > + rbd_assert(!rbd_is_snap(rbd_dev)); > + rbd_dev->mapping.size = rbd_dev->header.image_size; > > out: > up_write(&rbd_dev->header_rwsem);
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index bfff195e8e23..aba60e37b058 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -462,7 +462,7 @@ struct rbd_device { * by rbd_dev->lock */ enum rbd_dev_flags { - RBD_DEV_FLAG_EXISTS, /* mapped snapshot has not been deleted */ + RBD_DEV_FLAG_EXISTS, /* rbd_dev_device_setup() ran */ RBD_DEV_FLAG_REMOVING, /* this mapping is being removed */ RBD_DEV_FLAG_READONLY, /* -o ro or snapshot */ }; @@ -4848,19 +4848,6 @@ static void rbd_queue_workfn(struct work_struct *work) rbd_assert(!rbd_is_snap(rbd_dev)); } - /* - * Quit early if the mapped snapshot no longer exists. It's - * still possible the snapshot will have disappeared by the - * time our request arrives at the osd, but there's no sense in - * sending it if we already know. - */ - if (!test_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags)) { - dout("request for non-existent snapshot"); - rbd_assert(rbd_is_snap(rbd_dev)); - result = -ENXIO; - goto err_rq; - } - if (offset && length > U64_MAX - offset + 1) { rbd_warn(rbd_dev, "bad request range (%llu~%llu)", offset, length); @@ -5040,25 +5027,6 @@ static int rbd_dev_v1_header_info(struct rbd_device *rbd_dev) return ret; } -/* - * Clear the rbd device's EXISTS flag if the snapshot it's mapped to - * has disappeared from the (just updated) snapshot context. - */ -static void rbd_exists_validate(struct rbd_device *rbd_dev) -{ - u64 snap_id; - - if (!test_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags)) - return; - - snap_id = rbd_dev->spec->snap_id; - if (snap_id == CEPH_NOSNAP) - return; - - if (rbd_dev_snap_index(rbd_dev, snap_id) == BAD_SNAP_INDEX) - clear_bit(RBD_DEV_FLAG_EXISTS, &rbd_dev->flags); -} - static void rbd_dev_update_size(struct rbd_device *rbd_dev) { sector_t size; @@ -5099,12 +5067,8 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev) goto out; } - if (!rbd_is_snap(rbd_dev)) { - rbd_dev->mapping.size = rbd_dev->header.image_size; - } else { - /* validate mapped snapshot's EXISTS flag */ - rbd_exists_validate(rbd_dev); - } + rbd_assert(!rbd_is_snap(rbd_dev)); + rbd_dev->mapping.size = rbd_dev->header.image_size; out: up_write(&rbd_dev->header_rwsem);
RBD_DEV_FLAG_EXISTS check in rbd_queue_workfn() is racy and leads to inconsistent behaviour. If the object (or its snapshot) isn't there, the OSD returns ENOENT. A read submitted before the snapshot removal notification is processed would be zero-filled and ended with status OK, while future reads would be failed with IOERR. It also doesn't handle a case when an image that is mapped read-only is removed. On top of this, because watch is no longer established for read-only mappings, we no longer get notifications, so rbd_exists_validate() is effectively dead code. While failing requests rather than returning zeros is a good thing, RBD_DEV_FLAG_EXISTS is not it. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> --- drivers/block/rbd.c | 42 +++--------------------------------------- 1 file changed, 3 insertions(+), 39 deletions(-)