Re: [PATCH v3 3/8] xen/hypfs: add new enter() and exit() per node callbacks

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4446 bytes --]

On 16.12.20 17:16, Jan Beulich wrote:
> On 09.12.2020 17:09, Juergen Gross wrote:
>> In order to better support resource allocation and locking for dynamic
>> hypfs nodes add enter() and exit() callbacks to struct hypfs_funcs.
>>
>> The enter() callback is called when entering a node during hypfs user
>> actions (traversing, reading or writing it), while the exit() callback
>> is called when leaving a node (accessing another node at the same or a
>> higher directory level, or when returning to the user).
>>
>> For avoiding recursion this requires a parent pointer in each node.
>> Let the enter() callback return the entry address which is stored as
>> the last accessed node in order to be able to use a template entry for
>> that purpose in case of dynamic entries.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - new patch
>>
>> V3:
>> - add ASSERT(entry); (Jan Beulich)
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/common/hypfs.c      | 80 +++++++++++++++++++++++++++++++++++++++++
>>   xen/include/xen/hypfs.h |  5 +++
>>   2 files changed, 85 insertions(+)
>>
>> diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
>> index 6f822ae097..f04934db10 100644
>> --- a/xen/common/hypfs.c
>> +++ b/xen/common/hypfs.c
>> @@ -25,30 +25,40 @@ CHECK_hypfs_dirlistentry;
>>        ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
>>   
>>   const struct hypfs_funcs hypfs_dir_funcs = {
>> +    .enter = hypfs_node_enter,
>> +    .exit = hypfs_node_exit,
>>       .read = hypfs_read_dir,
>>       .write = hypfs_write_deny,
>>       .getsize = hypfs_getsize,
>>       .findentry = hypfs_dir_findentry,
>>   };
>>   const struct hypfs_funcs hypfs_leaf_ro_funcs = {
>> +    .enter = hypfs_node_enter,
>> +    .exit = hypfs_node_exit,
>>       .read = hypfs_read_leaf,
>>       .write = hypfs_write_deny,
>>       .getsize = hypfs_getsize,
>>       .findentry = hypfs_leaf_findentry,
>>   };
>>   const struct hypfs_funcs hypfs_leaf_wr_funcs = {
>> +    .enter = hypfs_node_enter,
>> +    .exit = hypfs_node_exit,
>>       .read = hypfs_read_leaf,
>>       .write = hypfs_write_leaf,
>>       .getsize = hypfs_getsize,
>>       .findentry = hypfs_leaf_findentry,
>>   };
>>   const struct hypfs_funcs hypfs_bool_wr_funcs = {
>> +    .enter = hypfs_node_enter,
>> +    .exit = hypfs_node_exit,
>>       .read = hypfs_read_leaf,
>>       .write = hypfs_write_bool,
>>       .getsize = hypfs_getsize,
>>       .findentry = hypfs_leaf_findentry,
>>   };
>>   const struct hypfs_funcs hypfs_custom_wr_funcs = {
>> +    .enter = hypfs_node_enter,
>> +    .exit = hypfs_node_exit,
>>       .read = hypfs_read_leaf,
>>       .write = hypfs_write_custom,
>>       .getsize = hypfs_getsize,
>> @@ -63,6 +73,8 @@ enum hypfs_lock_state {
>>   };
>>   static DEFINE_PER_CPU(enum hypfs_lock_state, hypfs_locked);
>>   
>> +static DEFINE_PER_CPU(const struct hypfs_entry *, hypfs_last_node_entered);
>> +
>>   HYPFS_DIR_INIT(hypfs_root, "");
>>   
>>   static void hypfs_read_lock(void)
>> @@ -100,11 +112,59 @@ static void hypfs_unlock(void)
>>       }
>>   }
>>   
>> +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry)
>> +{
>> +    return entry;
>> +}
>> +
>> +void hypfs_node_exit(const struct hypfs_entry *entry)
>> +{
>> +}
>> +
>> +static int node_enter(const struct hypfs_entry *entry)
>> +{
>> +    const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered);
>> +
>> +    entry = entry->funcs->enter(entry);
>> +    if ( IS_ERR(entry) )
>> +        return PTR_ERR(entry);
>> +
>> +    ASSERT(entry);
>> +    ASSERT(!*last || *last == entry->parent);
>> +
>> +    *last = entry;
>> +
>> +    return 0;
>> +}
>> +
>> +static void node_exit(const struct hypfs_entry *entry)
>> +{
>> +    const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered);
>> +
>> +    if ( !*last )
>> +        return;
> 
> To my question regarding this in v2 you replied
> 
> "I rechecked and have found that this was a remnant from an earlier
>   variant. *last won't ever be NULL, so the if can be dropped (a NULL
>   will be catched by the following ASSERT())."
> 
> Now this if() is still there. Why?

I really thought I did remove the if(). Seems as if I did that on
my test machine only and not in my git tree. Sorry for that.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]