On Tue, Feb 18, 2020 at 6:07 PM David Howells [off-list ref] wrote:

Add fsinfo support to the AFS filesystem.

[...]

 static const struct super_operations afs_super_ops = {
        .statfs         = afs_statfs,
+#ifdef CONFIG_FSINFO
+       .fsinfo_attributes = afs_fsinfo_attributes,
+#endif
+       .alloc_inode    = afs_alloc_inode,
+       .drop_inode     = afs_drop_inode,
+       .destroy_inode  = afs_destroy_inode,
+       .free_inode     = afs_free_inode,
+       .evict_inode    = afs_evict_inode,
+       .show_devname   = afs_show_devname,
+       .show_options   = afs_show_options,
+};
+
+static const struct super_operations afs_dyn_super_ops = {
+       .statfs         = afs_statfs,
+#ifdef CONFIG_FSINFO
+       .fsinfo_attributes = afs_dyn_fsinfo_attributes,
+#endif
        .alloc_inode    = afs_alloc_inode,
        .drop_inode     = afs_drop_inode,
        .destroy_inode  = afs_destroy_inode,

[...]

Ewww. So basically, having one static set of .fsinfo_attributes is not
sufficiently flexible for everyone, but instead of allowing the
filesystem to dynamically provide a list of supported attributes, you
just duplicate the super_operations? Seems to me like it'd be cleaner
to add a function pointer to the super_operations that can dynamically
fill out the supported fsinfo attributes.

It seems to me like the current API is going to be a dead end if you
ever want to have decent passthrough of these things for e.g. FUSE, or
overlayfs, or VirtFS?

(This is technically a (small) UAPI change for audit logging of AFS
filesystems, right? You may want to note that in the commit message.)

                afs_activate_volume(as->volume);
                iget_data.fid.vid       = as->volume->vid;
                iget_data.fid.vnode     = 1;

[...]

+static int afs_fsinfo_get_supports(struct path *path, struct fsinfo_context *ctx)
+{
+       struct fsinfo_supports *sup = ctx->buffer;
+
+       sup = ctx->buffer;

Duplicate assignment to "sup".

+       sup->stx_mask = (STATX_TYPE | STATX_MODE |
+                        STATX_NLINK |
+                        STATX_UID | STATX_GID |
+                        STATX_MTIME | STATX_INO |
+                        STATX_SIZE);
+       sup->stx_attributes = STATX_ATTR_AUTOMOUNT;
+       return sizeof(*sup);
+}

[...]

+static int afs_fsinfo_get_server_address(struct path *path, struct fsinfo_context *ctx)
+{
+       struct fsinfo_afs_server_address *addr = ctx->buffer;
+       struct afs_server_list *slist;
+       struct afs_super_info *as = AFS_FS_S(path->dentry->d_sb);
+       struct afs_addr_list *alist;
+       struct afs_volume *volume = as->volume;
+       struct afs_server *server;
+       struct afs_net *net = afs_d2net(path->dentry);
+       unsigned int i;
+       int ret = -ENODATA;
+
+       read_lock(&volume->servers_lock);
+       slist = afs_get_serverlist(volume->servers);
+       read_unlock(&volume->servers_lock);
+
+       if (ctx->Nth >= slist->nr_servers)
+               goto put_slist;
+       server = slist->servers[ctx->Nth].server;
+
+       read_lock(&server->fs_lock);
+       alist = afs_get_addrlist(rcu_access_pointer(server->addresses));

Documentation for rcu_access_pointer() says:

 * Return the value of the specified RCU-protected pointer, but omit the
 * lockdep checks for being in an RCU read-side critical section.  This is
 * useful when the value of this pointer is accessed, but the pointer is
 * not dereferenced, for example, when testing an RCU-protected pointer
 * against NULL.  Although rcu_access_pointer() may also be used in cases
 * where update-side locks prevent the value of the pointer from changing,
 * you should instead use rcu_dereference_protected() for this use case.
 *
 * It is also permissible to use rcu_access_pointer() when read-side
 * access to the pointer was removed at least one grace period ago, as
 * is the case in the context of the RCU callback that is freeing up
 * the data, or after a synchronize_rcu() returns.  This can be useful
 * when tearing down multi-linked structures after a grace period
 * has elapsed.

+       read_unlock(&server->fs_lock);