[0/9] wip-krbd-readonly
mbox series

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

Message

Ilya Dryomov Nov. 18, 2019, 1:38 p.m. UTC
Hello,

This series makes read-only mappings compatible with read-only caps:
we no longer establish a watch, acquire header and object map locks,
etc.  In addition, because images mapped read-only can no longer be
flipped to read-write, we allow read-only mappings with unsupported
features, as long as the image is still readable.

Thanks,

                Ilya


Ilya Dryomov (9):
  rbd: introduce rbd_is_snap()
  rbd: introduce RBD_DEV_FLAG_READONLY
  rbd: treat images mapped read-only seriously
  rbd: disallow read-write partitions on images mapped read-only
  rbd: don't acquire exclusive lock for read-only mappings
  rbd: don't establish watch for read-only mappings
  rbd: remove snapshot existence validation code
  rbd: don't query snapshot features
  rbd: ask for a weaker incompat mask for read-only mappings

 drivers/block/rbd.c | 203 ++++++++++++++++++++------------------------
 1 file changed, 93 insertions(+), 110 deletions(-)

Comments

Dongsheng Yang Nov. 19, 2019, 8:50 a.m. UTC | #1
Hi Ilya,

On 11/18/2019 09:38 PM, Ilya Dryomov wrote:
> Hello,
>
> This series makes read-only mappings compatible with read-only caps:
> we no longer establish a watch,

Although this is true in userspace librbd, I think that's wired: when
there is someone is reading this image, it can be removed. And the
reader will get all zero for later reads.

What about register a watcher but always ack for notifications? Then
we can prevent removing from image being reading.

Ilya, Jason, what's your opinion?

Thanx
> acquire header and object map locks,
> etc.  In addition, because images mapped read-only can no longer be
> flipped to read-write, we allow read-only mappings with unsupported
> features, as long as the image is still readable.
>
> Thanks,
>
>                  Ilya
>
>
> Ilya Dryomov (9):
>    rbd: introduce rbd_is_snap()
>    rbd: introduce RBD_DEV_FLAG_READONLY
>    rbd: treat images mapped read-only seriously
>    rbd: disallow read-write partitions on images mapped read-only
>    rbd: don't acquire exclusive lock for read-only mappings
>    rbd: don't establish watch for read-only mappings
>    rbd: remove snapshot existence validation code
>    rbd: don't query snapshot features
>    rbd: ask for a weaker incompat mask for read-only mappings
>
>   drivers/block/rbd.c | 203 ++++++++++++++++++++------------------------
>   1 file changed, 93 insertions(+), 110 deletions(-)
>
Ilya Dryomov Nov. 19, 2019, 11:59 a.m. UTC | #2
On Tue, Nov 19, 2019 at 9:50 AM Dongsheng Yang
<dongsheng.yang@easystack.cn> wrote:
>
>
> Hi Ilya,
>
> On 11/18/2019 09:38 PM, Ilya Dryomov wrote:
> > Hello,
> >
> > This series makes read-only mappings compatible with read-only caps:
> > we no longer establish a watch,
>
> Although this is true in userspace librbd, I think that's wired: when
> there is someone is reading this image, it can be removed. And the
> reader will get all zero for later reads.
>
> What about register a watcher but always ack for notifications? Then
> we can prevent removing from image being reading.

We can't register a watch because it is a write operation on the OSD
and we want read-only mappings to be usable with read-only OSD caps:

  $ ceph auth add client.ro ... osd 'profile rbd-read-only'
  $ sudo rbd map --user ro -o ro ...

Further, while returning zeros if an image or a snapshot is removed is
bad, a watch isn't a good solution.  It can be lost, and even when it's
there it's still racy.  See the description of patch 7.

Thanks,

                Ilya
Dongsheng Yang Nov. 19, 2019, 12:11 p.m. UTC | #3
On 11/19/2019 07:59 PM, Ilya Dryomov wrote:
> On Tue, Nov 19, 2019 at 9:50 AM Dongsheng Yang
> <dongsheng.yang@easystack.cn> wrote:
>>
>> Hi Ilya,
>>
>> On 11/18/2019 09:38 PM, Ilya Dryomov wrote:
>>> Hello,
>>>
>>> This series makes read-only mappings compatible with read-only caps:
>>> we no longer establish a watch,
>> Although this is true in userspace librbd, I think that's wired: when
>> there is someone is reading this image, it can be removed. And the
>> reader will get all zero for later reads.
>>
>> What about register a watcher but always ack for notifications? Then
>> we can prevent removing from image being reading.
> We can't register a watch because it is a write operation on the OSD
> and we want read-only mappings to be usable with read-only OSD caps:
>
>    $ ceph auth add client.ro ... osd 'profile rbd-read-only'
>    $ sudo rbd map --user ro -o ro ..
>
> Further, while returning zeros if an image or a snapshot is removed is
> bad, a watch isn't a good solution.  It can be lost, and even when it's
> there it's still racy.  See the description of patch 7


Right, it's not that easy. Maybe we need another series patches to 
improve it.
I am okey for it now, librbd is working in the same way.

Thanx
>
> Thanks,
>
>                  Ilya
>
Ilya Dryomov Nov. 19, 2019, 3:10 p.m. UTC | #4
On Tue, Nov 19, 2019 at 1:11 PM Dongsheng Yang
<dongsheng.yang@easystack.cn> wrote:
>
>
>
> On 11/19/2019 07:59 PM, Ilya Dryomov wrote:
> > On Tue, Nov 19, 2019 at 9:50 AM Dongsheng Yang
> > <dongsheng.yang@easystack.cn> wrote:
> >>
> >> Hi Ilya,
> >>
> >> On 11/18/2019 09:38 PM, Ilya Dryomov wrote:
> >>> Hello,
> >>>
> >>> This series makes read-only mappings compatible with read-only caps:
> >>> we no longer establish a watch,
> >> Although this is true in userspace librbd, I think that's wired: when
> >> there is someone is reading this image, it can be removed. And the
> >> reader will get all zero for later reads.
> >>
> >> What about register a watcher but always ack for notifications? Then
> >> we can prevent removing from image being reading.
> > We can't register a watch because it is a write operation on the OSD
> > and we want read-only mappings to be usable with read-only OSD caps:
> >
> >    $ ceph auth add client.ro ... osd 'profile rbd-read-only'
> >    $ sudo rbd map --user ro -o ro ..
> >
> > Further, while returning zeros if an image or a snapshot is removed is
> > bad, a watch isn't a good solution.  It can be lost, and even when it's
> > there it's still racy.  See the description of patch 7
>
>
> Right, it's not that easy. Maybe we need another series patches to
> improve it.

Yeah, just watching a header as it is is not going to do it.

Thanks for the review!

                Ilya