> On Nov 20, 2024, at 1:28 AM, Christian Brauner <brauner@xxxxxxxxxx> wrote:
[...]
>>>>> Then whenever you have to populate any of these fields, you just
>>>>> allocate one of these structs and set the inode up to point to it.
>>>>> They're tiny too, so don't bother freeing it until the inode is
>>>>> deallocated.
>>>>>
>>>>> It'd mean rejiggering a fair bit of fsnotify code, but it would give
>>>>> the fsnotify code an easier way to expand per-inode info in the future.
>>>>> It would also slightly shrink struct inode too.
>>
>> I am hoping to make i_bpf_storage available to tracing programs.
>> Therefore, I would rather not limit it to fsnotify context. We can
>> still use the universal on-demand allocator.
>
> Can't we just do something like:
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7e29433c5ecc..cc05a5485365 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -627,6 +627,12 @@ is_uncached_acl(struct posix_acl *acl)
> #define IOP_DEFAULT_READLINK 0x0010
> #define IOP_MGTIME 0x0020
>
> +struct inode_addons {
> + struct fsnotify_mark_connector __rcu *i_fsnotify_marks;
> + struct bpf_local_storage __rcu *i_bpf_storage;
> + __u32 i_fsnotify_mask; /* all events this inode cares about */
> +};
> +
> /*
> * Keep mostly read-only and often accessed (especially for
> * the RCU path lookup and 'stat' data) fields at the beginning
> @@ -731,12 +737,7 @@ struct inode {
> unsigned i_dir_seq;
> };
>
> -
> -#ifdef CONFIG_FSNOTIFY
> - __u32 i_fsnotify_mask; /* all events this inode cares about */
> - /* 32-bit hole reserved for expanding i_fsnotify_mask */
> - struct fsnotify_mark_connector __rcu *i_fsnotify_marks;
> -#endif
> + struct inode_addons *i_addons;
>
> #ifdef CONFIG_FS_ENCRYPTION
> struct fscrypt_inode_info *i_crypt_info;
>
> Then when either fsnotify or bpf needs that storage they can do a
> cmpxchg() based allocation for struct inode_addons just like I did with
> f_owner:
>
> int file_f_owner_allocate(struct file *file)
> {
> struct fown_struct *f_owner;
>
> f_owner = file_f_owner(file);
> if (f_owner)
> return 0;
>
> f_owner = kzalloc(sizeof(struct fown_struct), GFP_KERNEL);
> if (!f_owner)
> return -ENOMEM;
>
> rwlock_init(&f_owner->lock);
> f_owner->file = file;
> /* If someone else raced us, drop our allocation. */
> if (unlikely(cmpxchg(&file->f_owner, NULL, f_owner)))
> kfree(f_owner);
> return 0;
> }
>
> The internal allocations for specific fields are up to the subsystem
> ofc. Does that make sense?
This works for fsnotify/fanotify. However, for tracing use cases,
this is not as reliable as other (task, cgroup, sock) local storage.
BPF tracing programs need to work in any contexts, including NMI.
Therefore, doing kzalloc(GFP_KERNEL) is not always safe for
tracing use cases. OTOH, bpf local storage works in NMI. If we have
a i_bpf_storage pointer in struct inode, bpf inode storage will work
in NMI.
Thanks,
Song
