Re: [PATCH v5 21/45] block: add bdrv_try_set_aio_context_tran transaction action
On 6/21/22 14:04, Hanna Reitz wrote:
On 20.06.22 22:57, Vladimir Sementsov-Ogievskiy wrote:
On 6/13/22 10:46, Hanna Reitz wrote:
On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
To be used in further commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 48
1 file changed, 48 insertions(+)
diff --git a/block.c b/block.c
index be19964f89..1900cdf277 100644
--- a/block.c
+++ b/block.c
@@ -2907,6 +2907,54 @@ static void bdrv_child_free(BdrvChild *child)
g_free(child);
}
+typedef struct BdrvTrySetAioContextState {
+ BlockDriverState *bs;
+ AioContext *old_ctx;
+} BdrvTrySetAioContextState;
+
+static void bdrv_try_set_aio_context_abort(void *opaque)
+{
+ BdrvTrySetAioContextState *s = opaque;
+
+ if (bdrv_get_aio_context(s->bs) != s->old_ctx) {
+ bdrv_try_set_aio_context(s->bs, s->old_ctx, _abort);
As far as I understand, users of this transaction will need to do a bit of
AioContext lock shuffling: To set the context, they need to hold old_ctx, but
not new_ctx; but in case of abort, they need to release old_ctx and acquire
new_ctx before the abort handlers are called. (Due to the constraints on
bdrv_set_aio_context_ignore().)
If that’s true, I think that should be documented somewhere.
Hmm.. Actually, I think that bdrv_try_set_aio_context_abort() should do this
shuffling by it self. The only hope to correctly rollback a transaction, is
operation in assumption that on .abort() we are exactly on the same state as on
.prepare(), regardless of other actions. And this means that old_ctx is
acquired and new_ctx is not.
But if old_ctx is acquired and new_ctx is not, you cannot invoke
bdrv_try_set_aio_context(bs, old_ctx), because that requires the current
context (bdrv_get_aio_context(bs)) to be held, but not old_ctx (the “new”
context for this call).
Yes and that means that .abort() should release old_ctx and acquire new_ctx
before calling bdrv_try_set_aio_context(). And release new_ctx and acquire back
old_ctx. Does it make sense?
--
Best regards,
Vladimir
Re: [PATCH v5 21/45] block: add bdrv_try_set_aio_context_tran transaction action
On 20.06.22 22:57, Vladimir Sementsov-Ogievskiy wrote:
On 6/13/22 10:46, Hanna Reitz wrote:
On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
To be used in further commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 48
1 file changed, 48 insertions(+)
diff --git a/block.c b/block.c
index be19964f89..1900cdf277 100644
--- a/block.c
+++ b/block.c
@@ -2907,6 +2907,54 @@ static void bdrv_child_free(BdrvChild *child)
g_free(child);
}
+typedef struct BdrvTrySetAioContextState {
+ BlockDriverState *bs;
+ AioContext *old_ctx;
+} BdrvTrySetAioContextState;
+
+static void bdrv_try_set_aio_context_abort(void *opaque)
+{
+ BdrvTrySetAioContextState *s = opaque;
+
+ if (bdrv_get_aio_context(s->bs) != s->old_ctx) {
+ bdrv_try_set_aio_context(s->bs, s->old_ctx, _abort);
As far as I understand, users of this transaction will need to do a
bit of AioContext lock shuffling: To set the context, they need to
hold old_ctx, but not new_ctx; but in case of abort, they need to
release old_ctx and acquire new_ctx before the abort handlers are
called. (Due to the constraints on bdrv_set_aio_context_ignore().)
If that’s true, I think that should be documented somewhere.
Hmm.. Actually, I think that bdrv_try_set_aio_context_abort() should
do this shuffling by it self. The only hope to correctly rollback a
transaction, is operation in assumption that on .abort() we are
exactly on the same state as on .prepare(), regardless of other
actions. And this means that old_ctx is acquired and new_ctx is not.
But if old_ctx is acquired and new_ctx is not, you cannot invoke
bdrv_try_set_aio_context(bs, old_ctx), because that requires the current
context (bdrv_get_aio_context(bs)) to be held, but not old_ctx (the
“new” context for this call).
Re: [PATCH v5 21/45] block: add bdrv_try_set_aio_context_tran transaction action
On 6/13/22 10:46, Hanna Reitz wrote:
On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
To be used in further commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 48
1 file changed, 48 insertions(+)
diff --git a/block.c b/block.c
index be19964f89..1900cdf277 100644
--- a/block.c
+++ b/block.c
@@ -2907,6 +2907,54 @@ static void bdrv_child_free(BdrvChild *child)
g_free(child);
}
+typedef struct BdrvTrySetAioContextState {
+ BlockDriverState *bs;
+ AioContext *old_ctx;
+} BdrvTrySetAioContextState;
+
+static void bdrv_try_set_aio_context_abort(void *opaque)
+{
+ BdrvTrySetAioContextState *s = opaque;
+
+ if (bdrv_get_aio_context(s->bs) != s->old_ctx) {
+ bdrv_try_set_aio_context(s->bs, s->old_ctx, _abort);
As far as I understand, users of this transaction will need to do a bit of
AioContext lock shuffling: To set the context, they need to hold old_ctx, but
not new_ctx; but in case of abort, they need to release old_ctx and acquire
new_ctx before the abort handlers are called. (Due to the constraints on
bdrv_set_aio_context_ignore().)
If that’s true, I think that should be documented somewhere.
Hmm.. Actually, I think that bdrv_try_set_aio_context_abort() should do this
shuffling by it self. The only hope to correctly rollback a transaction, is
operation in assumption that on .abort() we are exactly on the same state as on
.prepare(), regardless of other actions. And this means that old_ctx is
acquired and new_ctx is not.
--
Best regards,
Vladimir
Re: [PATCH v5 21/45] block: add bdrv_try_set_aio_context_tran transaction action
On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
To be used in further commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block.c | 48
1 file changed, 48 insertions(+)
diff --git a/block.c b/block.c
index be19964f89..1900cdf277 100644
--- a/block.c
+++ b/block.c
@@ -2907,6 +2907,54 @@ static void bdrv_child_free(BdrvChild *child)
g_free(child);
}
+typedef struct BdrvTrySetAioContextState {
+BlockDriverState *bs;
+AioContext *old_ctx;
+} BdrvTrySetAioContextState;
+
+static void bdrv_try_set_aio_context_abort(void *opaque)
+{
+BdrvTrySetAioContextState *s = opaque;
+
+if (bdrv_get_aio_context(s->bs) != s->old_ctx) {
+bdrv_try_set_aio_context(s->bs, s->old_ctx, _abort);
As far as I understand, users of this transaction will need to do a bit
of AioContext lock shuffling: To set the context, they need to hold
old_ctx, but not new_ctx; but in case of abort, they need to release
old_ctx and acquire new_ctx before the abort handlers are called. (Due
to the constraints on bdrv_set_aio_context_ignore().)
If that’s true, I think that should be documented somewhere.
+}
+}
+
Re: [PATCH v5 21/45] block: add bdrv_try_set_aio_context_tran transaction action
On 09.06.22 16:56, Vladimir Sementsov-Ogievskiy wrote: On 6/8/22 14:49, Hanna Reitz wrote: On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote: To be used in further commit. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block.c | 48 1 file changed, 48 insertions(+) Looking at bdrv_child_try_set_aio_context(), it looks like bdrv_can_set_aio_context() were supposed to be the .prepare action, and bdrv_set_aio_context_ignore() should be the .commit action. Can we not use it that way? The difference is that we want the whole action be done in .prepare stage, not only the check. It's generally better: when do several actions in a transaction, actions usually depend on result of previous actions. Ah, yes. And I think it's necessary for graph update. Graph relations are changed during other actions .prepare phases, so I can't imagine how to postpone aio-context update to .commit phase. OK, sounds good. But I agree, that having both _can_ / _set_ and *tran APIs don't look good. May be we can refactor it.. But not in this series I think)
Re: [PATCH v5 21/45] block: add bdrv_try_set_aio_context_tran transaction action
On 6/8/22 14:49, Hanna Reitz wrote: On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote: To be used in further commit. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block.c | 48 1 file changed, 48 insertions(+) Looking at bdrv_child_try_set_aio_context(), it looks like bdrv_can_set_aio_context() were supposed to be the .prepare action, and bdrv_set_aio_context_ignore() should be the .commit action. Can we not use it that way? The difference is that we want the whole action be done in .prepare stage, not only the check. It's generally better: when do several actions in a transaction, actions usually depend on result of previous actions. And I think it's necessary for graph update. Graph relations are changed during other actions .prepare phases, so I can't imagine how to postpone aio-context update to .commit phase. But I agree, that having both _can_ / _set_ and *tran APIs don't look good. May be we can refactor it.. But not in this series I think) -- Best regards, Vladimir
Re: [PATCH v5 21/45] block: add bdrv_try_set_aio_context_tran transaction action
On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote: To be used in further commit. Signed-off-by: Vladimir Sementsov-Ogievskiy --- block.c | 48 1 file changed, 48 insertions(+) Looking at bdrv_child_try_set_aio_context(), it looks like bdrv_can_set_aio_context() were supposed to be the .prepare action, and bdrv_set_aio_context_ignore() should be the .commit action. Can we not use it that way?
