On 12/26/2018 08:27 PM, Ilya Dryomov wrote:
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.
Makes sense. So let's follow the current assumption now to mark it
ro after any change of features.
Thanx
Thanks,
Ilya
