| Message ID | 20191118133816.3963-4-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: > Even though -o ro/-o read_only/--read-only options are very old, we > have never really treated them seriously (on par with snapshots). As > a first step, fail writes to images mapped read-only just like we do > for snapshots. > > We need this check in rbd because the block layer basically ignores > read-only setting, see commit a32e236eb93e ("Partially revert "block: > fail op_is_write() requests to read-only partitions""). > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > --- > drivers/block/rbd.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 330d2789f373..842b92ef2c06 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -4820,11 +4820,14 @@ static void rbd_queue_workfn(struct work_struct *work) > goto err_rq; > } > > - if (op_type != OBJ_OP_READ && rbd_is_snap(rbd_dev)) { > - rbd_warn(rbd_dev, "%s on read-only snapshot", > - obj_op_name(op_type)); > - result = -EIO; > - goto err; > + if (op_type != OBJ_OP_READ) { > + if (rbd_is_ro(rbd_dev)) { > + rbd_warn(rbd_dev, "%s on read-only mapping", > + obj_op_name(op_type)); > + result = -EIO; > + goto err; > + } > + rbd_assert(!rbd_is_snap(rbd_dev)); Just one question here, if block layer does not prevent write for readonly disk 100%, should we make it rbd-level readonly in rbd_ioctl_set_ro() when requested ? Thanx > } > > /*
On Tue, Nov 19, 2019 at 9:37 AM Dongsheng Yang <dongsheng.yang@easystack.cn> wrote: > > > > On 11/18/2019 09:38 PM, Ilya Dryomov wrote: > > Even though -o ro/-o read_only/--read-only options are very old, we > > have never really treated them seriously (on par with snapshots). As > > a first step, fail writes to images mapped read-only just like we do > > for snapshots. > > > > We need this check in rbd because the block layer basically ignores > > read-only setting, see commit a32e236eb93e ("Partially revert "block: > > fail op_is_write() requests to read-only partitions""). > > > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > > --- > > drivers/block/rbd.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > > index 330d2789f373..842b92ef2c06 100644 > > --- a/drivers/block/rbd.c > > +++ b/drivers/block/rbd.c > > @@ -4820,11 +4820,14 @@ static void rbd_queue_workfn(struct work_struct *work) > > goto err_rq; > > } > > > > - if (op_type != OBJ_OP_READ && rbd_is_snap(rbd_dev)) { > > - rbd_warn(rbd_dev, "%s on read-only snapshot", > > - obj_op_name(op_type)); > > - result = -EIO; > > - goto err; > > + if (op_type != OBJ_OP_READ) { > > + if (rbd_is_ro(rbd_dev)) { > > + rbd_warn(rbd_dev, "%s on read-only mapping", > > + obj_op_name(op_type)); > > + result = -EIO; > > + goto err; > > + } > > + rbd_assert(!rbd_is_snap(rbd_dev)); > > Just one question here, if block layer does not prevent write for > readonly disk 100%, > should we make it rbd-level readonly in rbd_ioctl_set_ro() when requested ? No, the point is to divorce the read-only setting at the block layer level from read-only setting in rbd. Enforcing the block layer setting is up to the block layer, rbd only enforces the rbd setting. We allow the block layer setting to be tweaked with BLKROSET, while rbd setting is immutable (i.e. if you mapped with -o ro, you would have to unmap and map without -o ro). So we propagate rbd setting up to the block layer, but the block layer setting isn't propagated down to rbd. Thanks, Ilya
On 11/19/2019 06:55 PM, Ilya Dryomov wrote: > On Tue, Nov 19, 2019 at 9:37 AM Dongsheng Yang > <dongsheng.yang@easystack.cn> wrote: >> >> >> On 11/18/2019 09:38 PM, Ilya Dryomov wrote: >>> Even though -o ro/-o read_only/--read-only options are very old, we >>> have never really treated them seriously (on par with snapshots). As >>> a first step, fail writes to images mapped read-only just like we do >>> for snapshots. >>> >>> We need this check in rbd because the block layer basically ignores >>> read-only setting, see commit a32e236eb93e ("Partially revert "block: >>> fail op_is_write() requests to read-only partitions""). >>> >>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com> >>> --- >>> drivers/block/rbd.c | 13 ++++++++----- >>> 1 file changed, 8 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >>> index 330d2789f373..842b92ef2c06 100644 >>> --- a/drivers/block/rbd.c >>> +++ b/drivers/block/rbd.c >>> @@ -4820,11 +4820,14 @@ static void rbd_queue_workfn(struct work_struct *work) >>> goto err_rq; >>> } >>> >>> - if (op_type != OBJ_OP_READ && rbd_is_snap(rbd_dev)) { >>> - rbd_warn(rbd_dev, "%s on read-only snapshot", >>> - obj_op_name(op_type)); >>> - result = -EIO; >>> - goto err; >>> + if (op_type != OBJ_OP_READ) { >>> + if (rbd_is_ro(rbd_dev)) { >>> + rbd_warn(rbd_dev, "%s on read-only mapping", >>> + obj_op_name(op_type)); >>> + result = -EIO; >>> + goto err; >>> + } >>> + rbd_assert(!rbd_is_snap(rbd_dev)); >> Just one question here, if block layer does not prevent write for >> readonly disk 100%, >> should we make it rbd-level readonly in rbd_ioctl_set_ro() when requested ? > No, the point is to divorce the read-only setting at the block layer > level from read-only setting in rbd. Enforcing the block layer setting > is up to the block layer, rbd only enforces the rbd setting. We allow > the block layer setting to be tweaked with BLKROSET, while rbd setting > is immutable (i.e. if you mapped with -o ro, you would have to unmap > and map without -o ro). So we propagate rbd setting up to the block > layer, but the block layer setting isn't propagated down to rbd. makes sense > > Thanks, > > Ilya >
On 11/18/2019 09:38 PM, Ilya Dryomov wrote: > Even though -o ro/-o read_only/--read-only options are very old, we > have never really treated them seriously (on par with snapshots). As > a first step, fail writes to images mapped read-only just like we do > for snapshots. > > We need this check in rbd because the block layer basically ignores > read-only setting, see commit a32e236eb93e ("Partially revert "block: > fail op_is_write() requests to read-only partitions""). > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn> > --- > drivers/block/rbd.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 330d2789f373..842b92ef2c06 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -4820,11 +4820,14 @@ static void rbd_queue_workfn(struct work_struct *work) > goto err_rq; > } > > - if (op_type != OBJ_OP_READ && rbd_is_snap(rbd_dev)) { > - rbd_warn(rbd_dev, "%s on read-only snapshot", > - obj_op_name(op_type)); > - result = -EIO; > - goto err; > + if (op_type != OBJ_OP_READ) { > + if (rbd_is_ro(rbd_dev)) { > + rbd_warn(rbd_dev, "%s on read-only mapping", > + obj_op_name(op_type)); > + result = -EIO; > + goto err; > + } > + rbd_assert(!rbd_is_snap(rbd_dev)); > } > > /*
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 330d2789f373..842b92ef2c06 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -4820,11 +4820,14 @@ static void rbd_queue_workfn(struct work_struct *work) goto err_rq; } - if (op_type != OBJ_OP_READ && rbd_is_snap(rbd_dev)) { - rbd_warn(rbd_dev, "%s on read-only snapshot", - obj_op_name(op_type)); - result = -EIO; - goto err; + if (op_type != OBJ_OP_READ) { + if (rbd_is_ro(rbd_dev)) { + rbd_warn(rbd_dev, "%s on read-only mapping", + obj_op_name(op_type)); + result = -EIO; + goto err; + } + rbd_assert(!rbd_is_snap(rbd_dev)); } /*
Even though -o ro/-o read_only/--read-only options are very old, we have never really treated them seriously (on par with snapshots). As a first step, fail writes to images mapped read-only just like we do for snapshots. We need this check in rbd because the block layer basically ignores read-only setting, see commit a32e236eb93e ("Partially revert "block: fail op_is_write() requests to read-only partitions""). Signed-off-by: Ilya Dryomov <idryomov@gmail.com> --- drivers/block/rbd.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)