On Mon, Dec 24, 2018 at 2:24 AM Dongsheng Yang
<dongsheng.yang@xxxxxxxxxxxx> wrote:
>
>
>
> On 12/22/2018 10:35 AM, Gregory Farnum wrote:
> > 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.
>
> I agree with Greg here. In my use case, we have an operation
> to add a rbd image into DR (disaster recovery, actually enable journaling
> and start rbd-mirroring for it). So we need to change the feature of
> an image online.
It's not just a matter of a less strict check -- allowing manipulating
image features while the image is mapped will require some thought and
possibly a lot of additional code and tests. The current assumption in
the kernel code is that all features are immutable.
Thanks,
Ilya
