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

[Jan Beulich <jbeulich@suse.com>]

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? (My alternative suggestion was
to have ASSERT(!entry->parent) inside the if() body, since prior to
that you said this would be an indication of the root entry.)

Jan