Re: [PATCH v5 29/45] block: introduce BDRV_O_NOPERM flag

[Vladimir Sementsov-Ogievskiy]

On 6/13/22 12:54, Hanna Reitz wrote:
> On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> > Now copy-before-write filter has weak permission model: when it has no
> > parents, it share write permission on source. Otherwise we just can't
> > blockdev-add it, when existing user of source has write permission.
> >
> > The situation is bad, it means that copy-before-write filter doesn't
> > guarantee that all write goes through it.
>
> I don’t understand how this situation really is bad, because it sounds like 
> anything else would just be a safeguard against users adding a CBW filter 
> without making use of it.  Which I’d think is their own fault.
>
> As far as I remember the actual problem is that we cannot do transactional 
> graph modifications, where e.g. a CBW node is inserted and a bitmap is created 
> in a single atomic transaction[1].  Which is a problem.  And now I just don’t 
> quite understand how unsharing WRITE unconditionally would help with the actual 
> problem.
>
> [1] Then again, would then even be “atomic”?  For that transaction to work as 
> intended, the node would need to be drained during the transaction (so that the 
> bitmap stays in sync with the CBW state). It doesn’t look like that would be 
> the case.

I think, we should already be in a drained section, when do the transaction.

In qmp_transaction we have bdrv_drain_all() call. It's enough if we don't yield 
during transaction actions (and mostly, we shouldn't) (is it enough, when we 
have iothreads?). Probably, it should be bdrv_drain_all_being() before all 
actions and bdrv_drain_all_end() after them.

> So perhaps I’m just remembering incorrectly.

OK, the same answer: I should try to split these features, as they are separate:

1. transactional API

2. strict permissions for CBW

Seems that [2] is not necessary for [1]. If so, we can consider smaller picture 
(only [1]), and do [2] later (or not do, if it remains too complicated for the 
small profit).

> > And a lot better is unshare
> > write always. But how to insert the filter in this case?
> >
> > The solution is to do blockdev-add and blockdev-replace in one
> > transaction, and more, update permissions only after both command.
> >
> > For now, let's create a possibility to not update permission on file
> > child of copy-before-write filter at time of open.
> >
> > New interfaces are:
> >
> > - bds_tree_init() with flags argument, so that caller may pass
> >    additional flags, for example the new BDRV_O_NOPERM.
> >
> > - bdrv_open_file_child_common() with boolean refresh_perms arguments.
> >    Drivers may use this function with refresh_perms = true, if they want
> >    to satisfy BDRV_O_NOPERM. No one such driver for now.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>


--
Best regards,
Vladimir