On Tue, Jul 10, 2018 at 6:02 AM Jan Kara <jack@xxxxxxx> wrote:
> Allocate fsnotify mark independently instead of embedding it inside
> chunk. This will allow us to just replace chunk attached to mark when
> growing / shrinking chunk instead of replacing mark attached to inode
> which is a more complex operation.
>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
> kernel/audit_tree.c | 59 ++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 45 insertions(+), 14 deletions(-)
...
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index bce3b04a365d..aec9b27a20ff 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -38,6 +38,11 @@ struct audit_chunk {
> } owners[];
> };
>
> +struct audit_tree_mark {
> + struct fsnotify_mark fsn_mark;
> + struct audit_chunk *chunk;
> +};
It's probably okay to just call it "mark" considering we call
fsnotify_mark fields "mark" elsewhere. If we are going to change it
in one spot we should probably change it other places as well for the
sake of readability.
2,10 +148,28 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk)
> call_rcu(&chunk->head, __put_chunk);
> }
>
> +static inline struct audit_tree_mark *AUDIT_M(struct fsnotify_mark *entry)
> +{
> + return container_of(entry, struct audit_tree_mark, fsn_mark);
> +}
When I see a symbol in all caps I think "macro definition", but this
isn't a macro definition. I would suggest a different name, dropping
the caps, or converting it into a macro.
Also, unless I missed a call, it seems like after patch 10 all callers
of AUDIT_M end up getting the chunk field; maybe it is better if
AUDIT_M() ends up returning the audit_chunk pointer instead of the
audit_tree_mark pointer (and of course a name change if this is a
reasonable change)?
> static void audit_tree_destroy_watch(struct fsnotify_mark *entry)
> {
> - struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark);
> + struct audit_chunk *chunk = AUDIT_M(entry)->chunk;
> audit_mark_put_chunk(chunk);
> + kmem_cache_free(audit_tree_mark_cachep, entry);
> +}
I think you've said you've already fixed the above (thanks for the
review Amir!).
> +static struct fsnotify_mark *alloc_fsnotify_mark(void)
> +{
> + struct audit_tree_mark *mark;
> +
> + mark = kmem_cache_zalloc(audit_tree_mark_cachep, GFP_KERNEL);
> + if (!mark)
> + return NULL;
> + fsnotify_init_mark(&mark->fsn_mark, audit_tree_group);
> + mark->fsn_mark.mask = FS_IN_IGNORED;
> + return &mark->fsn_mark;
> }
Can't we just call it alloc_mark()? Or did you create the function
earlier in the patchset and I'm missing it now?
[SIDE NOTE: I have some rather big disagreements with the current
naming in this file, but keeping things consistent seems to be a Good
Thing (once again, this is an existing problem not specific to your
patches).]
--
paul moore
www.paul-moore.com
