| Message ID | 1550762799-830661-3-git-send-email-andrey.shinkevich@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block-stream: include base into job node list | expand |
21.02.2019 18:26, Andrey Shinkevich wrote: > The block-stream job needs to own the base node as the limiter > of the copy-on-read operation. So, the base node is included in > the job node list (block_job_add_bdrv). > Also, the block-stream job would not allow the base node to go > away due to the graph modification, e.g. when a filter node is > inserted between the bottom node and the base node. > For that reason, the flag BLK_PERM_GRAPH_MOD is unset in the > shared permission bit mask of the base node. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > block/stream.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/block/stream.c b/block/stream.c > index 7a49ac0..c8f93d4 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, > &error_abort); > } > > + if (base) { > + /* > + * The base node should not disappear during the job. > + */ > + block_job_add_bdrv(&s->common, "base", base, 0, > + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, > + &error_abort); > + } > + > s->base = base; > s->backing_file_str = g_strdup(backing_file_str); > s->bs_read_only = bs_read_only; > Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in node graph? bdrv_replace_node don't check this permission. So, I don't understand, how this permission works.. Graph modification operation in difference with read or write are not done through BdrvChild at all. Are there any ideas or work started for some another mechanism of "freezing" a subgraph during an operation?
On Mon 25 Feb 2019 05:39:14 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> --- a/block/stream.c >> +++ b/block/stream.c >> @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, >> &error_abort); >> } >> >> + if (base) { >> + /* >> + * The base node should not disappear during the job. >> + */ >> + block_job_add_bdrv(&s->common, "base", base, 0, >> + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, >> + &error_abort); >> + } >> + >> s->base = base; >> s->backing_file_str = g_strdup(backing_file_str); >> s->bs_read_only = bs_read_only; >> > > > Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in > node graph? > > bdrv_replace_node don't check this permission. So, I don't understand, > how this permission works.. Graph modification operation in difference > with read or write are not done through BdrvChild at all. > > Are there any ideas or work started for some another mechanism of > "freezing" a subgraph during an operation? See this discussion: https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00387.html And these patches (currently under review): https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00622.html Berto
On 26/02/2019 16:33, Alberto Garcia wrote: > On Mon 25 Feb 2019 05:39:14 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>> --- a/block/stream.c >>> +++ b/block/stream.c >>> @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, >>> &error_abort); >>> } >>> >>> + if (base) { >>> + /* >>> + * The base node should not disappear during the job. >>> + */ >>> + block_job_add_bdrv(&s->common, "base", base, 0, >>> + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, >>> + &error_abort); >>> + } >>> + >>> s->base = base; >>> s->backing_file_str = g_strdup(backing_file_str); >>> s->bs_read_only = bs_read_only; >>> >> >> >> Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in >> node graph? >> >> bdrv_replace_node don't check this permission. So, I don't understand, >> how this permission works.. Graph modification operation in difference >> with read or write are not done through BdrvChild at all. >> >> Are there any ideas or work started for some another mechanism of >> "freezing" a subgraph during an operation? > > See this discussion: > > https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00387.html > > And these patches (currently under review): > > https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00622.html > > Berto > The bdrv_freeze_backing_chain() excludes the base BlockDriverState. And we would like to protect the base as well when running the block-stream.
On Thu 21 Feb 2019 04:26:39 PM CET, Andrey Shinkevich wrote: > The block-stream job needs to own the base node as the limiter > of the copy-on-read operation. So, the base node is included in > the job node list (block_job_add_bdrv). > Also, the block-stream job would not allow the base node to go > away due to the graph modification, e.g. when a filter node is > inserted between the bottom node and the base node. > For that reason, the flag BLK_PERM_GRAPH_MOD is unset in the > shared permission bit mask of the base node. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > block/stream.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/block/stream.c b/block/stream.c > index 7a49ac0..c8f93d4 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, > &error_abort); > } > > + if (base) { > + /* > + * The base node should not disappear during the job. > + */ > + block_job_add_bdrv(&s->common, "base", base, 0, > + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, > + &error_abort); I'm not sure if I understand the ~BLK_PERM_GRAPH_MOD bit, what's the consequence of not removing that permission? Berto
On 08/03/2019 17:00, Alberto Garcia wrote: > On Thu 21 Feb 2019 04:26:39 PM CET, Andrey Shinkevich wrote: >> The block-stream job needs to own the base node as the limiter >> of the copy-on-read operation. So, the base node is included in >> the job node list (block_job_add_bdrv). >> Also, the block-stream job would not allow the base node to go >> away due to the graph modification, e.g. when a filter node is >> inserted between the bottom node and the base node. >> For that reason, the flag BLK_PERM_GRAPH_MOD is unset in the >> shared permission bit mask of the base node. >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> --- >> block/stream.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/block/stream.c b/block/stream.c >> index 7a49ac0..c8f93d4 100644 >> --- a/block/stream.c >> +++ b/block/stream.c >> @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, >> &error_abort); >> } >> >> + if (base) { >> + /* >> + * The base node should not disappear during the job. >> + */ >> + block_job_add_bdrv(&s->common, "base", base, 0, >> + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, >> + &error_abort); > > I'm not sure if I understand the ~BLK_PERM_GRAPH_MOD bit, what's the > consequence of not removing that permission? > > Berto > Alberto, Thank you for your question. There will be neither a bad consequence, nor a good one. That means the block_job_add_bdrv() does not help to protect the graph from modification. So, "freezing BdrvChild links" may be a better solution if the chain includes the 'base' node.
On Tue 26 Feb 2019 05:39:41 PM CET, Andrey Shinkevich wrote: > On 26/02/2019 16:33, Alberto Garcia wrote: >> On Mon 25 Feb 2019 05:39:14 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>>> --- a/block/stream.c >>>> +++ b/block/stream.c >>>> @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, >>>> &error_abort); >>>> } >>>> >>>> + if (base) { >>>> + /* >>>> + * The base node should not disappear during the job. >>>> + */ >>>> + block_job_add_bdrv(&s->common, "base", base, 0, >>>> + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, >>>> + &error_abort); >>>> + } >>>> + >>>> s->base = base; >>>> s->backing_file_str = g_strdup(backing_file_str); >>>> s->bs_read_only = bs_read_only; >>>> >>> >>> >>> Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in >>> node graph? >>> >>> bdrv_replace_node don't check this permission. So, I don't understand, >>> how this permission works.. Graph modification operation in difference >>> with read or write are not done through BdrvChild at all. >>> >>> Are there any ideas or work started for some another mechanism of >>> "freezing" a subgraph during an operation? >> >> See this discussion: >> >> https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00387.html >> >> And these patches (currently under review): >> >> https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00622.html > > The bdrv_freeze_backing_chain() excludes the base BlockDriverState. > And we would like to protect the base as well when running the > block-stream. I was just looking into this again. The bdrv_freeze_backing_chain() (now in master) freezes all links between bs and base, both included, so base cannot go away anymore. Is block_job_add_bdrv() still necessary in this scenario? Unless I'm wrong I think that what we can do now is to start getting rid of the op blockers, shouldn't it be possible to get the same functionality with the permission system plus the BdrvChild.frozen attribute ? Berto
On 18/03/2019 17:42, Alberto Garcia wrote: > On Tue 26 Feb 2019 05:39:41 PM CET, Andrey Shinkevich wrote: >> On 26/02/2019 16:33, Alberto Garcia wrote: >>> On Mon 25 Feb 2019 05:39:14 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>>>> --- a/block/stream.c >>>>> +++ b/block/stream.c >>>>> @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, >>>>> &error_abort); >>>>> } >>>>> >>>>> + if (base) { >>>>> + /* >>>>> + * The base node should not disappear during the job. >>>>> + */ >>>>> + block_job_add_bdrv(&s->common, "base", base, 0, >>>>> + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, >>>>> + &error_abort); >>>>> + } >>>>> + >>>>> s->base = base; >>>>> s->backing_file_str = g_strdup(backing_file_str); >>>>> s->bs_read_only = bs_read_only; >>>>> >>>> >>>> >>>> Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in >>>> node graph? >>>> >>>> bdrv_replace_node don't check this permission. So, I don't understand, >>>> how this permission works.. Graph modification operation in difference >>>> with read or write are not done through BdrvChild at all. >>>> >>>> Are there any ideas or work started for some another mechanism of >>>> "freezing" a subgraph during an operation? >>> >>> See this discussion: >>> >>> https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00387.html >>> >>> And these patches (currently under review): >>> >>> https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00622.html >> >> The bdrv_freeze_backing_chain() excludes the base BlockDriverState. >> And we would like to protect the base as well when running the >> block-stream. > > I was just looking into this again. The bdrv_freeze_backing_chain() (now > in master) freezes all links between bs and base, both included, so base > cannot go away anymore. > > Is block_job_add_bdrv() still necessary in this scenario? > > Unless I'm wrong I think that what we can do now is to start getting rid > of the op blockers, shouldn't it be possible to get the same > functionality with the permission system plus the BdrvChild.frozen > attribute ? > > Berto > I have got rid of using the block_job_add_bdrv() for the 'base'. But, as for the "intermediate nodes", I will want to add the BLK_PERM_WRITE shared flag to them for the 'discard block' feature in future. So, I have to check if we can get 'write' permission for the block discard operation without invoking block_job_add_bdrv() for them... Andrey
On 19.03.2019 19:44, Andrey Shinkevich wrote: > > > On 18/03/2019 17:42, Alberto Garcia wrote: >> On Tue 26 Feb 2019 05:39:41 PM CET, Andrey Shinkevich wrote: >>> On 26/02/2019 16:33, Alberto Garcia wrote: >>>> On Mon 25 Feb 2019 05:39:14 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>>>>> --- a/block/stream.c >>>>>> +++ b/block/stream.c >>>>>> @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, >>>>>> &error_abort); >>>>>> } >>>>>> >>>>>> + if (base) { >>>>>> + /* >>>>>> + * The base node should not disappear during the job. >>>>>> + */ >>>>>> + block_job_add_bdrv(&s->common, "base", base, 0, >>>>>> + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, >>>>>> + &error_abort); >>>>>> + } >>>>>> + >>>>>> s->base = base; >>>>>> s->backing_file_str = g_strdup(backing_file_str); >>>>>> s->bs_read_only = bs_read_only; >>>>>> >>>>> >>>>> >>>>> Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in >>>>> node graph? >>>>> >>>>> bdrv_replace_node don't check this permission. So, I don't understand, >>>>> how this permission works.. Graph modification operation in difference >>>>> with read or write are not done through BdrvChild at all. >>>>> >>>>> Are there any ideas or work started for some another mechanism of >>>>> "freezing" a subgraph during an operation? >>>> >>>> See this discussion: >>>> >>>> https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00387.html >>>> >>>> And these patches (currently under review): >>>> >>>> https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00622.html >>> >>> The bdrv_freeze_backing_chain() excludes the base BlockDriverState. >>> And we would like to protect the base as well when running the >>> block-stream. >> >> I was just looking into this again. The bdrv_freeze_backing_chain() (now >> in master) freezes all links between bs and base, both included, so base >> cannot go away anymore. >> >> Is block_job_add_bdrv() still necessary in this scenario? >> >> Unless I'm wrong I think that what we can do now is to start getting rid >> of the op blockers, shouldn't it be possible to get the same >> functionality with the permission system plus the BdrvChild.frozen >> attribute ? >> >> Berto >> I have got rid of using the block_job_add_bdrv() for the 'base'. > But, as for the "intermediate nodes", I will want to add the > BLK_PERM_WRITE shared flag to them for the 'discard block' feature > in future. So, I have to check if we can get 'write' permission for > the block discard operation without invoking block_job_add_bdrv() > for them... > > Andrey > I think Alberto mean only block_job_add_bdrv for base, and I agree that we don't need it after we have frozen attribute. -- Best regards, Vladimir
diff --git a/block/stream.c b/block/stream.c index 7a49ac0..c8f93d4 100644 --- a/block/stream.c +++ b/block/stream.c @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, &error_abort); } + if (base) { + /* + * The base node should not disappear during the job. + */ + block_job_add_bdrv(&s->common, "base", base, 0, + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, + &error_abort); + } + s->base = base; s->backing_file_str = g_strdup(backing_file_str); s->bs_read_only = bs_read_only;
The block-stream job needs to own the base node as the limiter of the copy-on-read operation. So, the base node is included in the job node list (block_job_add_bdrv). Also, the block-stream job would not allow the base node to go away due to the graph modification, e.g. when a filter node is inserted between the bottom node and the base node. For that reason, the flag BLK_PERM_GRAPH_MOD is unset in the shared permission bit mask of the base node. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- block/stream.c | 9 +++++++++ 1 file changed, 9 insertions(+)