On Fri, Dec 21, 2018 at 1:56 AM Ilya Dryomov <idryomov@xxxxxxxxx> wrote:
>
> On Tue, Dec 18, 2018 at 10:33 AM Dongsheng Yang
> <dongsheng.yang@xxxxxxxxxxxx> wrote:
> >
> > If there is unsupported feature enabled after rbd map, we will noticed it in
> > refreshing, then we need to set the disk readonly. In addition, we need to
> > set disk to rw if these unsupportted features disabled later.
>
> Do we really need to revert back to RW? If the block device suddenly
> goes RO, most applications including filesystems will fail irreversibly
> anyway.
>
> >
> > Signed-off-by: Dongsheng Yang <dongsheng.yang@xxxxxxxxxxxx>
> > ---
> > drivers/block/rbd.c | 48 ++++++++++++++++++++++++------------------------
> > 1 file changed, 24 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> > index 8e5140b..e911830 100644
> > --- a/drivers/block/rbd.c
> > +++ b/drivers/block/rbd.c
> > @@ -3695,6 +3695,9 @@ static void rbd_queue_workfn(struct work_struct *work)
> > snapc = rbd_dev->header.snapc;
> > ceph_get_snap_context(snapc);
> > }
> > + must_be_locked =
> > + (rbd_dev->header.features & RBD_FEATURE_EXCLUSIVE_LOCK) &&
> > + (op_type != OBJ_OP_READ || rbd_dev->opts->lock_on_read);
> > up_read(&rbd_dev->header_rwsem);
> >
> > if (offset + length > mapping_size) {
> > @@ -3704,9 +3707,6 @@ static void rbd_queue_workfn(struct work_struct *work)
> > goto err_rq;
> > }
> >
> > - must_be_locked =
> > - (rbd_dev->header.features & RBD_FEATURE_EXCLUSIVE_LOCK) &&
> > - (op_type != OBJ_OP_READ || rbd_dev->opts->lock_on_read);
> > if (must_be_locked) {
> > down_read(&rbd_dev->lock_rwsem);
> > result = rbd_wait_state_locked(rbd_dev,
> > @@ -4567,13 +4567,6 @@ static int _rbd_dev_v2_snap_features(struct rbd_device *rbd_dev, u64 snap_id,
> > if (ret < sizeof (features_buf))
> > return -ERANGE;
> >
> > - unsup = le64_to_cpu(features_buf.incompat) & ~RBD_FEATURES_SUPPORTED;
> > - if (unsup) {
> > - rbd_warn(rbd_dev, "image uses unsupported features: 0x%llx",
> > - unsup);
> > - return -ENXIO;
> > - }
> > -
> > *snap_features = le64_to_cpu(features_buf.features);
> >
> > dout(" snap_id 0x%016llx features = 0x%016llx incompat = 0x%016llx\n",
> > @@ -4581,13 +4574,14 @@ static int _rbd_dev_v2_snap_features(struct rbd_device *rbd_dev, u64 snap_id,
> > (unsigned long long)*snap_features,
> > (unsigned long long)le64_to_cpu(features_buf.incompat));
> >
> > - return 0;
> > -}
> > + unsup = le64_to_cpu(features_buf.incompat) & ~RBD_FEATURES_SUPPORTED;
> > + if (unsup) {
> > + rbd_warn(rbd_dev, "image uses unsupported features: 0x%llx",
> > + unsup);
> > + return -ENXIO;
> > + }
> >
> > -static int rbd_dev_v2_features(struct rbd_device *rbd_dev)
> > -{
> > - return _rbd_dev_v2_snap_features(rbd_dev, CEPH_NOSNAP,
> > - &rbd_dev->header.features);
> > + return 0;
> > }
> >
> > struct parent_image_info {
> > @@ -5184,12 +5178,26 @@ static const char *rbd_dev_v2_snap_name(struct rbd_device *rbd_dev,
> > static int rbd_dev_v2_header_info(struct rbd_device *rbd_dev)
> > {
> > bool first_time = rbd_dev->header.object_prefix == NULL;
> > + u64 features = 0;
> > int ret;
> >
> > ret = rbd_dev_v2_image_size(rbd_dev);
> > if (ret)
> > return ret;
> >
> > + ret = _rbd_dev_v2_snap_features(rbd_dev, CEPH_NOSNAP,
> > + &features);
> > + if (ret) {
> > + if (-ENXIO == ret && !first_time)
> > + set_disk_ro(rbd_dev->disk, true);
> > + else
> > + return ret;
> > + } else {
> > + if (!first_time && !rbd_dev->opts->read_only)
> > + set_disk_ro(rbd_dev->disk, false);
> > + }
> > + rbd_dev->mapping.features = rbd_dev->header.features = features;
> > +
> > if (first_time) {
> > ret = rbd_dev_v2_header_onetime(rbd_dev);
> > if (ret)
> > @@ -5560,14 +5568,6 @@ static int rbd_dev_v2_header_onetime(struct rbd_device *rbd_dev)
> > if (ret)
> > goto out_err;
> >
> > - /*
> > - * Get the and check features for the image. Currently the
> > - * features are assumed to never change.
> > - */
> > - ret = rbd_dev_v2_features(rbd_dev);
> > - if (ret)
> > - goto out_err;
> > -
> > /* If the image supports fancy striping, get its parameters */
> >
> > if (rbd_dev->header.features & RBD_FEATURE_STRIPINGV2) {
>
> I think we should take this further. We don't support _any_ changes
> while the image is mapped, i.e. not just enabling something unsupported
> but also disabling something supported. Let's change all places where
> we check for features to use rbd_dev->mapping.features and simply add
> a new rbd_dev->header.features != rbd_dev->mapping.features check to
> rbd_dev_refresh().
Is that a statement of how things work now, or a rule you'd like to
enforce? I thought it was possible to do things like enable the object
map while images are in use, and it seems like we'd want to continue
supporting that.
-Greg
