Re: [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on updated graph

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

05.02.2021 20:57, Kevin Wolf wrote:
> Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Move bdrv_reopen_multiple to new paradigm of permission update:
>> first update graph relations, then do refresh the permissions.
>>
>> We have to modify reopen process in file-posix driver: with new scheme
>> we don't have prepared permissions in raw_reopen_prepare(), so we
>> should reconfigure fd in raw_check_perm(). Still this seems more native
>> and simple anyway.
> 
> Hm... The diffstat shows that it is simpler because it needs less code.
> 
> But relying on the permission change callbacks for getting a new file
> descriptor that changes more than just permissions doesn't feel
> completely right either. Can we even expect the permission callbacks to
> be called when the permissions aren't changed?

With new scheme permission update becomes an obvious step of bdrv_reopen_multiple(): we do call bdrv_list_refresh_perms(), for the list of all touched nodes and all their subtrees. And callbacks are called unconditionally bdrv_node_refresh_perm()->bdrv_drv_set_perm(). So, I think, we can rely on it. Probably worth one-two comments.

> 
> But then, reopen and permission updates were already a bit entangled
> before. If we can guarantee that the permission functions will always be
> called, even if the permissions don't change, I guess it's okay.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block.h |   2 +-
>>   block.c               | 183 +++++++++++-------------------------------
>>   block/file-posix.c    |  84 +++++--------------
>>   3 files changed, 70 insertions(+), 199 deletions(-)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 0f21ef313f..82271d9ccd 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -195,7 +195,7 @@ typedef struct BDRVReopenState {
>>       BlockdevDetectZeroesOptions detect_zeroes;
>>       bool backing_missing;
>>       bool replace_backing_bs;  /* new_backing_bs is ignored if this is false */
>> -    BlockDriverState *new_backing_bs; /* If NULL then detach the current bs */
>> +    BlockDriverState *old_backing_bs; /* keep pointer for permissions update */
>>       uint64_t perm, shared_perm;
> 
> perm and shared_perm are unused now and can be removed.
> 
>>       QDict *options;
>>       QDict *explicit_options;
>> diff --git a/block.c b/block.c
>> index 617cba9547..474e624152 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -103,8 +103,9 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
>>                                       GSList **tran, Error **errp);
>>   static void bdrv_remove_backing(BlockDriverState *bs, GSList **tran);
>>   
>> -static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue
>> -                               *queue, Error **errp);
>> +static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
>> +                               BlockReopenQueue *queue,
>> +                               GSList **set_backings_tran, Error **errp);
>>   static void bdrv_reopen_commit(BDRVReopenState *reopen_state);
>>   static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
>>   
>> @@ -2403,6 +2404,7 @@ static void bdrv_list_abort_perm_update(GSList *list)
>>       }
>>   }
>>   
>> +__attribute__((unused))
>>   static void bdrv_abort_perm_update(BlockDriverState *bs)
>>   {
>>       g_autoptr(GSList) list = bdrv_topological_dfs(NULL, NULL, bs);
>> @@ -2498,6 +2500,7 @@ char *bdrv_perm_names(uint64_t perm)
>>    *
>>    * Needs to be followed by a call to either bdrv_set_perm() or
>>    * bdrv_abort_perm_update(). */
>> +__attribute__((unused))
>>   static int bdrv_check_update_perm(BlockDriverState *bs, BlockReopenQueue *q,
>>                                     uint64_t new_used_perm,
>>                                     uint64_t new_shared_perm,
>> @@ -4100,10 +4103,6 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>>       bs_entry->state.explicit_options = explicit_options;
>>       bs_entry->state.flags = flags;
>>   
>> -    /* This needs to be overwritten in bdrv_reopen_prepare() */
>> -    bs_entry->state.perm = UINT64_MAX;
>> -    bs_entry->state.shared_perm = 0;
>> -
>>       /*
>>        * If keep_old_opts is false then it means that unspecified
>>        * options must be reset to their original value. We don't allow
>> @@ -4186,40 +4185,37 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>>    */
>>   int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>>   {
>> -    int ret = -1;
>> +    int ret = 0;
> 
> I would prefer to leave this right before the 'goto cleanup'.
> 
> Not sure if I fully understand all consequences yet, but overall, apart
> from my concerns about file-posix and the potential AioContext locking
> problems, this looks like a nice simplification of the process.
> 
> Come to think of it, the AioContext handling is probably wrong already
> before your series. reopen_commit for one node could move the whole tree
> to a different context and then the later nodes would all be processed
> while holding the wrong lock.
> 

Probably proper way is to acquire all involved aio contexts as I do in 29 and update aio-context updating functions to work in such conditions(all aio contexts are already acquired by caller).


-- 
Best regards,
Vladimir