On Tue, Feb 18, 2020 at 6:07 PM David Howells <dhowells@xxxxxxxxxx> 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,
[...]
> @@ -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?
> 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;
[...]
> +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);
