[3/9] rbd: treat images mapped read-only seriously
diff mbox series

Message ID 20191118133816.3963-4-idryomov@gmail.com
State New
Headers show
Series
  • wip-krbd-readonly
Related show

Commit Message

Ilya Dryomov Nov. 18, 2019, 1:38 p.m. UTC
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(-)

Comments

Dongsheng Yang Nov. 19, 2019, 8:37 a.m. UTC | #1
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
>   	}
>   
>   	/*
Ilya Dryomov Nov. 19, 2019, 10:55 a.m. UTC | #2
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
Dongsheng Yang Nov. 19, 2019, 12:04 p.m. UTC | #3
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
>
Dongsheng Yang Nov. 19, 2019, 12:12 p.m. UTC | #4
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));
>   	}
>   
>   	/*

Patch
diff mbox series

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));
 	}
 
 	/*