diff mbox

[v13,02/20] block: Drop consistent read perm if opened unsafe

Message ID 20170420075237.18219-3-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng April 20, 2017, 7:52 a.m. UTC
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Kevin Wolf April 20, 2017, 10:58 a.m. UTC | #1
Am 20.04.2017 um 09:52 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1fbbb8d..f5182d8 100644
> --- a/block.c
> +++ b/block.c
> @@ -1722,9 +1722,15 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>          }
>  
>          /* bs->file always needs to be consistent because of the metadata. We
> -         * can never allow other users to resize or write to it. */
> -        perm |= BLK_PERM_CONSISTENT_READ;
> -        shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> +         * cannot allow other users to resize or write to it unless the caller
> +         * explicitly expects unsafe readings. */
> +        if (!(bdrv_get_flags(bs) & BDRV_O_UNSAFE_READ)) {

We have already spent considerable time to get rid of flags and instead
convert them into options passed in the QDict, so that they become
configurable with things like blockdev-add. Adding new flags potentially
moves in the opposite direction, so we have to be careful there.

I would be okay with patch 1, because in this case it's basically just a
shortcut for callers of blk_new_open(), which is fine. As soon as we
start querying the flag later and even rely on it being inherited, like
in this patch, I think it becomes a problem.

So if we need the flag in all nodes, can we make it an option that is
parsed in bdrv_open_common() into a bool bs->unsafe_read and inherited
explicitly in bdrv_inherited_options() and bdrv_backing_options()?

> +            perm |= BLK_PERM_CONSISTENT_READ;
> +            shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> +        } else {
> +            perm &= ~BLK_PERM_CONSISTENT_READ;
> +            shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> +        }

I'm not completely sure why we would be interested in CONSISTENT_READ
anyway, isn't allowing shared writes what we really need? (Which you
already do here in addition to dropping CONSISTENT_READ, without it
being mentioned in the commit message.)

Also, another thought: Being only at the start of the series, I'm not
sure what this will be used for, but can we make sure that unsafe_read
is only set if the image is opened read-only? If this is for the
libguestfs use case, this restriction should be fine.

Kevin
Fam Zheng April 20, 2017, 11:19 a.m. UTC | #2
On Thu, 04/20 12:58, Kevin Wolf wrote:
> Am 20.04.2017 um 09:52 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 1fbbb8d..f5182d8 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1722,9 +1722,15 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> >          }
> >  
> >          /* bs->file always needs to be consistent because of the metadata. We
> > -         * can never allow other users to resize or write to it. */
> > -        perm |= BLK_PERM_CONSISTENT_READ;
> > -        shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> > +         * cannot allow other users to resize or write to it unless the caller
> > +         * explicitly expects unsafe readings. */
> > +        if (!(bdrv_get_flags(bs) & BDRV_O_UNSAFE_READ)) {
> 
> We have already spent considerable time to get rid of flags and instead
> convert them into options passed in the QDict, so that they become
> configurable with things like blockdev-add. Adding new flags potentially
> moves in the opposite direction, so we have to be careful there.
> 
> I would be okay with patch 1, because in this case it's basically just a
> shortcut for callers of blk_new_open(), which is fine. As soon as we
> start querying the flag later and even rely on it being inherited, like
> in this patch, I think it becomes a problem.
> 
> So if we need the flag in all nodes, can we make it an option that is
> parsed in bdrv_open_common() into a bool bs->unsafe_read and inherited
> explicitly in bdrv_inherited_options() and bdrv_backing_options()?

OK, I knew new flags are bad, but this is perhaps what I was missing, as an
alternative.

> 
> > +            perm |= BLK_PERM_CONSISTENT_READ;
> > +            shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> > +        } else {
> > +            perm &= ~BLK_PERM_CONSISTENT_READ;
> > +            shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> > +        }
> 
> I'm not completely sure why we would be interested in CONSISTENT_READ
> anyway, isn't allowing shared writes what we really need? (Which you
> already do here in addition to dropping CONSISTENT_READ, without it
> being mentioned in the commit message.)

I think taking external programs into account, CONSISTENT_READ and shared write
are related: if another writer can modify file, our view is not consistent.
That's why I handle them together.

> 
> Also, another thought: Being only at the start of the series, I'm not
> sure what this will be used for, but can we make sure that unsafe_read
> is only set if the image is opened read-only? If this is for the
> libguestfs use case, this restriction should be fine.

I guess you are right. I will give a try to your QDict idea, and only apply it
if read-only.

Fam
diff mbox

Patch

diff --git a/block.c b/block.c
index 1fbbb8d..f5182d8 100644
--- a/block.c
+++ b/block.c
@@ -1722,9 +1722,15 @@  void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
         }
 
         /* bs->file always needs to be consistent because of the metadata. We
-         * can never allow other users to resize or write to it. */
-        perm |= BLK_PERM_CONSISTENT_READ;
-        shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
+         * cannot allow other users to resize or write to it unless the caller
+         * explicitly expects unsafe readings. */
+        if (!(bdrv_get_flags(bs) & BDRV_O_UNSAFE_READ)) {
+            perm |= BLK_PERM_CONSISTENT_READ;
+            shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
+        } else {
+            perm &= ~BLK_PERM_CONSISTENT_READ;
+            shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
+        }
     } else {
         /* We want consistent read from backing files if the parent needs it.
          * No other operations are performed on backing files. */