| Message ID | 1545125540-14815-1-git-send-email-dongsheng.yang@easystack.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rbd: mark rbd disk to ro if we found feature unsupported enabled after mapping | expand |
On Tue, Dec 18, 2018 at 10:33 AM Dongsheng Yang <dongsheng.yang@easystack.cn> 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@easystack.cn> > --- > 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(). Thanks, Ilya
On Fri, Dec 21, 2018 at 1:56 AM Ilya Dryomov <idryomov@gmail.com> wrote: > > On Tue, Dec 18, 2018 at 10:33 AM Dongsheng Yang > <dongsheng.yang@easystack.cn> 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@easystack.cn> > > --- > > 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
On 12/22/2018 10:35 AM, Gregory Farnum wrote: > On Fri, Dec 21, 2018 at 1:56 AM Ilya Dryomov <idryomov@gmail.com> wrote: >> On Tue, Dec 18, 2018 at 10:33 AM Dongsheng Yang >> <dongsheng.yang@easystack.cn> 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@easystack.cn> >>> --- >>> 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. Yang > -Greg >
On Sat, Dec 22, 2018 at 3:35 AM Gregory Farnum <gfarnum@redhat.com> wrote: > > On Fri, Dec 21, 2018 at 1:56 AM Ilya Dryomov <idryomov@gmail.com> wrote: > > > > On Tue, Dec 18, 2018 at 10:33 AM Dongsheng Yang > > <dongsheng.yang@easystack.cn> 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@easystack.cn> > > > --- > > > 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. This is how things work in the kernel client, but this limitation is not currently enforced. librbd allows enabling and disabling specific image features while the image is in use. This will continue to be supported, no question about that. Thanks, Ilya
On Mon, Dec 24, 2018 at 2:24 AM Dongsheng Yang <dongsheng.yang@easystack.cn> wrote: > > > > On 12/22/2018 10:35 AM, Gregory Farnum wrote: > > On Fri, Dec 21, 2018 at 1:56 AM Ilya Dryomov <idryomov@gmail.com> wrote: > >> On Tue, Dec 18, 2018 at 10:33 AM Dongsheng Yang > >> <dongsheng.yang@easystack.cn> 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@easystack.cn> > >>> --- > >>> 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
On 12/26/2018 08:27 PM, Ilya Dryomov wrote: > On Mon, Dec 24, 2018 at 2:24 AM Dongsheng Yang > <dongsheng.yang@easystack.cn> wrote: >> >> >> On 12/22/2018 10:35 AM, Gregory Farnum wrote: >>> On Fri, Dec 21, 2018 at 1:56 AM Ilya Dryomov <idryomov@gmail.com> wrote: >>>> On Tue, Dec 18, 2018 at 10:33 AM Dongsheng Yang >>>> <dongsheng.yang@easystack.cn> 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@easystack.cn> >>>>> --- >>>>> 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. Makes sense. So let's follow the current assumption now to mark it ro after any change of features. Thanx > > Thanks, > > Ilya >
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) {
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. Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn> --- drivers/block/rbd.c | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-)