Re: [PATCH 11/19] afs: Support fsinfo() [ver #16]
flat view
From: Jann Horn <jannh@google.com>
Date: 2020-02-19 21:02:16
Also in:
linux-fsdevel, lkml
On Tue, Feb 18, 2020 at 6:07 PM David Howells [off-list ref] wrote:
Add fsinfo support to the AFS filesystem.
[...]
quoted hunk ↗ jump to hunk
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,
[...]
quoted hunk ↗ jump to hunk
@@ -432,9 +454,12 @@ static int afs_fill_super(struct super_block *sb, struct afs_fs_context *ctx) sb->s_blocksize_bits = PAGE_SHIFT; sb->s_maxbytes = MAX_LFS_FILESIZE; sb->s_magic = AFS_FS_MAGIC; - sb->s_op = &afs_super_ops; - if (!as->dyn_root) + if (!as->dyn_root) { + sb->s_op = &afs_super_ops; sb->s_xattr = afs_xattr_handlers; + } else { + sb->s_op = &afs_dyn_super_ops; + }
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?
quoted hunk ↗ jump to hunk
ret = super_setup_bdi(sb); if (ret) return ret;@@ -444,7 +469,7 @@ static int afs_fill_super(struct super_block *sb, struct afs_fs_context *ctx) if (as->dyn_root) { inode = afs_iget_pseudo_dir(sb, true); } else { - sprintf(sb->s_id, "%llu", as->volume->vid); + sprintf(sb->s_id, "%llx", as->volume->vid);
(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;[...]
quoted hunk ↗ jump to hunk
+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".
quoted hunk ↗ jump to hunk
+ 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); +}
[...]
quoted hunk ↗ jump to hunk
+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);