To be used in further commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
 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, &error_abort);
+    }
+}
+
+static TransactionActionDrv bdrv_try_set_aio_context_drv = {
+    .abort = bdrv_try_set_aio_context_abort,
+    .clean = g_free,
+};
+
+__attribute__((unused))
+static int bdrv_try_set_aio_context_tran(BlockDriverState *bs,
+                                         AioContext *new_ctx,
+                                         Transaction *tran,
+                                         Error **errp)
+{
+    AioContext *old_ctx = bdrv_get_aio_context(bs);
+    BdrvTrySetAioContextState *s;
+    int ret;
+
+    if (old_ctx == new_ctx) {
+        return 0;
+    }
+
+    ret = bdrv_try_set_aio_context(bs, new_ctx, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    s = g_new(BdrvTrySetAioContextState, 1);
+    *s = (BdrvTrySetAioContextState) {
+        .bs = bs,
+        .old_ctx = old_ctx,
+    };
+    tran_add(tran, &bdrv_try_set_aio_context_drv, s);
+
+    return 0;
+}
+
 typedef struct BdrvAttachChildCommonState {
     BdrvChild *child;
     AioContext *old_parent_ctx;
-- 
2.35.1

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> To be used in further commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   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, &error_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.

> +    }
> +}
> +

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 <vsementsov@openvz.org>
>> ---
>>   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, &error_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

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 <vsementsov@openvz.org>
>>> ---
>>>   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, &error_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).

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 <vsementsov@openvz.org>
>>>> ---
>>>>   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, &error_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

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> To be used in further commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   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?

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 <vsementsov@openvz.org>
>> ---
>>   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

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 <vsementsov@openvz.org>
>>> ---
>>>   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)