On Wed, Jul 16, 2014 at 12:07:10PM +0800, Qu Wenruo wrote:
> btrfs uses differnet routine to handle 'subvolid=' and 'subvol=' mount
> option.
> Given 'subvol=' mount option, btrfs will mount btrfs first and then call
> mount_subtree() to mount a subtree of btrfs, making vfs handle the path
> searching.
> This is good since vfs layer know extactly that a subtree mount is done
> and findmnt(8) knows which subtree is mounted.
>
> However when using 'subvolid=' mount option, btrfs will do all the
> internal subvolume objectid searching and checking, making VFS unaware
> about which subtree is mounted, as result, findmnt(8) can't showing any
> useful subtree mount info for end users.
>
> This patch will use the root backref to reverse search the subvolume
> path for a given subvolid, making findmnt(8) works again.
>
> Reported-by: Stefan G.Weichinger <lists@xxxxxxxx>
> Signed-off-by: Qu Wenruo <quwenruo@xxxxxxxxxxxxxx>
Ack for unifying the way subvol= and subvolid= are handled, but I don't
like some aspects of the implementation.
The kmalloc/krealloc makes it really complicated and is not imho
necessary. The mount options length is limited to PAGE_SIZE in the vfs
code. Do the same here, allocate a page, filter the options, do the
necessary processing and just check for overflows.
You can drop u64_to_strlen.
> +#define CLEAR_SUBVOL 1
> +#define CLEAR_SUBVOLID 2
Though they're internal and local to the file, please add BTRFS_ prefix
at least.
> /*
> - * This will strip out the subvol=%s argument for an argument string and add
> - * subvolid=0 to make sure we get the actual tree root for path walking to the
> - * subvol we want.
> + * This will strip out the subvol=%s or subvolid=%s argument for an argumen
> + * string and add subvolid=0 to make sure we get the actual tree root for path
> + * walking to the subvol we want.
> */
> -static char *setup_root_args(char *args)
> +static char *setup_root_args(char *args, int flags, u64 subvol_objectid)
> {
> - unsigned len = strlen(args) + 2 + 1;
> - char *src, *dst, *buf;
> + unsigned len;
> + char *src = NULL, *dst, *buf, *comma;
Please use the recommended style and put each on a separate line. I'm
not sure if you'll need all of them for the implementation witouth the
kmallocs, the comment applies generally.
> + char *subvol_string = "subvolid=";
> + int option_len = 0;
> +
> + if (!args) {
> + /* Case 1, not args, all default mounting
> + * just return 'subvolid=<FS_ROOT>' */
Not the preferred style of comments.
> + len = strlen(subvol_string) +
> + u64_to_strlen(subvol_objectid) + 1;
> + dst = kmalloc(len, GFP_NOFS);
> + if (!dst)
> + return NULL;
> + sprintf(dst, "%s%llu", subvol_string, subvol_objectid);
> + return dst;
> + }
>
> - /*
> - * We need the same args as before, but with this substitution:
> - * s!subvol=[^,]+!subvolid=0!
> - *
> - * Since the replacement string is up to 2 bytes longer than the
> - * original, allocate strlen(args) + 2 + 1 bytes.
> - */
> + switch (flags) {
> + case CLEAR_SUBVOL:
> + src = strstr(args, "subvol=");
> + break;
> + case CLEAR_SUBVOLID:
> + src = strstr(args, "subvolid=");
> + break;
> + }
>
> - src = strstr(args, "subvol=");
> - /* This shouldn't happen, but just in case.. */
> - if (!src)
> - return NULL;
> + if (!src) {
> + /* Case 2, some args, default subvolume mounting
> + * just append ',subvolid=<FS_ROOT>' */
> +
> + /* 1 for ending '\0', 1 for leading ',' */
> + len = strlen(args) + strlen(subvol_string) +
> + u64_to_strlen(subvol_objectid) + 2;
> + dst = kmalloc(len, GFP_NOFS);
> + if (!dst)
> + return NULL;
> + strcpy(dst, args);
> + sprintf(dst + strlen(args), ",%s%llu", subvol_string,
> + subvol_objectid);
> + return dst;
> + }
> +
> + /* Case 3, subvolid=/subvol= mount
> + * repalce the 'subvolid/subvol' options to 'subvolid=<FS_ROOT>' */
> + comma = strchr(src, ',');
> + if (comma)
> + option_len = comma - src;
> + else
> + option_len = strlen(src);
> + len = strlen(args) - option_len + strlen(subvol_string) +
> + u64_to_strlen(subvol_objectid) + 1;
>
> buf = dst = kmalloc(len, GFP_NOFS);
> if (!buf)
> @@ -1154,28 +1208,126 @@ static char *setup_root_args(char *args)
> dst += strlen(args);
> }
>
> - strcpy(dst, "subvolid=0");
> - dst += strlen("subvolid=0");
> + len = sprintf(dst, "%s%llu", subvol_string, subvol_objectid);
> + dst += len;
>
> /*
> * If there is a "," after the original subvol=... string,
> * copy that suffix into our buffer. Otherwise, we're done.
> */
> - src = strchr(src, ',');
> - if (src)
> - strcpy(dst, src);
> + if (comma)
> + strcpy(dst, comma);
>
> return buf;
> }
>
> -static struct dentry *mount_subvol(const char *subvol_name, int flags,
> - const char *device_name, char *data)
> +static char *str_append_head(char *dest, char *src)
I think this is called 'prepend' :)
> +{
> + memmove(dest + strlen(src), dest, strlen(dest) + 1);
> + memcpy(dest, src, strlen(src));
Yes, prepends src to dest inplace.
> + return dest;
> +}
> +
> +/* Find the path for given subvol_objectid.
> + * Caller needs to readlock the root tree and kzalloc PATH_MAX for
> + * subvol_name and namebuf */
> +static char *find_subvol_by_id(struct btrfs_root *root, u64 subvol_objectid)
> +{
> + struct btrfs_key key;
> + struct btrfs_key found_key;
> + struct btrfs_root_ref *ref;
> + struct btrfs_path *path;
> + char *namebuf = NULL;
> + char *new_buf = NULL;
> + char *subvol_ret = NULL;
> + int ret = 0;
> + u16 namelen = 0;
> +
> + path = btrfs_alloc_path();
> + /* Alloc 1 byte for later strlen() calls */
> + subvol_ret = kzalloc(1, GFP_NOFS);
> + if (!path || !subvol_ret) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + key.objectid = subvol_objectid;
> + key.type = BTRFS_ROOT_BACKREF_KEY;
> + key.offset = 0;
> + /* We don't need to lock the tree_root,
> + * if when we do the backref walking, some one deleted/moved
> + * the subvol, we just return -ENOENT or let mount_subtree
> + * return -ENOENT and no disaster will happen.
> + * User should not modify subvolume when trying to mount it */
> + while (key.objectid != BTRFS_FS_TREE_OBJECTID) {
> + ret = btrfs_search_slot_for_read(root, &key, path, 1, 1);
> + if (ret < 0)
> + goto out;
> + if (ret) {
> + ret = -ENOENT;
> + goto out;
> + }
> + btrfs_item_key_to_cpu(path->nodes[0], &found_key,
> + path->slots[0]);
> + if (found_key.objectid != key.objectid ||
> + found_key.type != BTRFS_ROOT_BACKREF_KEY) {
> + ret = -ENOENT;
> + goto out;
> + }
> + key.objectid = found_key.offset;
> + ref = btrfs_item_ptr(path->nodes[0], path->slots[0],
> + struct btrfs_root_ref);
> + namelen = btrfs_root_ref_name_len(path->nodes[0], ref);
> + /* One for ending '\0' One for '/' */
> + new_buf = krealloc(namebuf, namelen + 2, GFP_NOFS);
> + if (!new_buf) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + namebuf = new_buf;
> + read_extent_buffer(path->nodes[0], namebuf,
> + (unsigned long)(ref + 1), namelen);
> + btrfs_release_path(path);
> + *(namebuf + namelen) = '/';
> + *(namebuf + namelen + 1) = '\0';
> +
> + new_buf = krealloc(subvol_ret, strlen(subvol_ret) + namelen + 2,
> + GFP_NOFS);
> + if (!new_buf) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + subvol_ret = new_buf;
> + str_append_head(subvol_ret, namebuf);
> + }
> +out:
> + kfree(namebuf);
> + btrfs_free_path(path);
> + if (ret < 0) {
> + kfree(subvol_ret);
> + return ERR_PTR(ret);
> + } else
> + return subvol_ret;
> +
> +}
> +
> +static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
> + int flags, const char *device_name,
> + char *data)
> {
> struct dentry *root;
> struct vfsmount *mnt;
> + struct btrfs_root *tree_root = NULL;
> char *newargs;
> + char *subvol_ret = NULL;
> + int ret = 0;
>
> - newargs = setup_root_args(data);
> + if (subvol_name)
> + newargs = setup_root_args(data, CLEAR_SUBVOL,
> + BTRFS_FS_TREE_OBJECTID);
> + else
> + newargs = setup_root_args(data, CLEAR_SUBVOLID,
> + BTRFS_FS_TREE_OBJECTID);
There's no other value passed to setup_root_args than
BTRFS_FS_TREE_OBJECTID? So you can put it directly to setup_root_args,
or I'm missing someting.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
