diff mbox series

btrfs: allow more subvol= option

Message ID 20200721203340.275921-2-kreijack@libero.it (mailing list archive)
State New, archived
Headers show
Series btrfs: allow more subvol= option | expand

Commit Message

Goffredo Baroncelli July 21, 2020, 8:33 p.m. UTC 
From: Goffredo Baroncelli <kreijack@inwind.it>

When more than one subvol= options are passed, btrfs try to mount
each subvolume until the first one succeed. Up to 5 subvol= options
can be passed.

Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>

---
 fs/btrfs/super.c | 71 ++++++++++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 26 deletions(-)

Comments

Steven Davies July 21, 2020, 8:50 p.m. UTC | #1 
On 21/07/2020 21:33, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <kreijack@inwind.it>
> 
> When more than one subvol= options are passed, btrfs try to mount
> each subvolume until the first one succeed. Up to 5 subvol= options
> can be passed.
> 
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> 
> ---
>   fs/btrfs/super.c | 71 ++++++++++++++++++++++++++++++------------------
>   1 file changed, 45 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index bc73fd670702..12d066e8d52c 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -52,6 +52,8 @@
>   #define CREATE_TRACE_POINTS
>   #include <trace/events/btrfs.h>
>   
> +#define SUBVOL_NAMES_COUNT 5

As this is a maximum, perhaps MAX_SUBVOL_NAMES or SUBVOL_NAMES_MAX

> +
>   static const struct super_operations btrfs_super_ops;
>   
>   /*
> @@ -974,12 +976,13 @@ static int btrfs_parse_device_options(const char *options, fmode_t flags,
>    *
>    * The value is later passed to mount_subvol()
>    */
> -static int btrfs_parse_subvol_options(const char *options, char **subvol_name,
> -		u64 *subvol_objectid)
> +static int btrfs_parse_subvol_options(const char *options, char **subvol_names,
> +					u64 *subvol_objectid)
>   {
>   	substring_t args[MAX_OPT_ARGS];
>   	char *opts, *orig, *p;
>   	int error = 0;
> +	int svi = 0;
>   	u64 subvolid;
>   
>   	if (!options)
> @@ -1002,12 +1005,17 @@ static int btrfs_parse_subvol_options(const char *options, char **subvol_name,
>   		token = match_token(p, tokens, args);
>   		switch (token) {
>   		case Opt_subvol:
> -			kfree(*subvol_name);
> -			*subvol_name = match_strdup(&args[0]);
> -			if (!*subvol_name) {
> +			if (svi >= SUBVOL_NAMES_COUNT) {
> +				pr_err("BTRFS: too much 'subvol=' mount options\n");

s/too much/too many/

Perhaps also include ", maximum is %d", SUBVOL_NAMES_COUNT

--snip--
kernel test robot July 22, 2020, 1:12 a.m. UTC | #2 
Hi Goffredo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.8-rc6 next-20200721]
[cannot apply to btrfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Goffredo-Baroncelli/btrfs-allow-more-subvol-option/20200722-043357
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: x86_64-randconfig-s022-20200719 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-49-g707c5017-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast from restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected unsigned long flags @@     got restricted gfp_t [usertype] mask @@
   include/trace/events/btrfs.h:1335:1: sparse:     expected unsigned long flags
   include/trace/events/btrfs.h:1335:1: sparse:     got restricted gfp_t [usertype] mask
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast to restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: cast to restricted gfp_t
   include/trace/events/btrfs.h:1335:1: sparse: sparse: restricted gfp_t degrades to integer
   include/trace/events/btrfs.h:1335:1: sparse: sparse: restricted gfp_t degrades to integer
>> fs/btrfs/super.c:1714:51: sparse: sparse: Using plain integer as NULL pointer
   fs/btrfs/super.c:2394:31: sparse: sparse: incompatible types in comparison expression (different address spaces):
   fs/btrfs/super.c:2394:31: sparse:    struct rcu_string [noderef] __rcu *
   fs/btrfs/super.c:2394:31: sparse:    struct rcu_string *

vim +1714 fs/btrfs/super.c

  1685	
  1686	/*
  1687	 * Mount function which is called by VFS layer.
  1688	 *
  1689	 * In order to allow mounting a subvolume directly, btrfs uses mount_subtree()
  1690	 * which needs vfsmount* of device's root (/).  This means device's root has to
  1691	 * be mounted internally in any case.
  1692	 *
  1693	 * Operation flow:
  1694	 *   1. Parse subvol id related options for later use in mount_subvol().
  1695	 *
  1696	 *   2. Mount device's root (/) by calling vfs_kern_mount().
  1697	 *
  1698	 *      NOTE: vfs_kern_mount() is used by VFS to call btrfs_mount() in the
  1699	 *      first place. In order to avoid calling btrfs_mount() again, we use
  1700	 *      different file_system_type which is not registered to VFS by
  1701	 *      register_filesystem() (btrfs_root_fs_type). As a result,
  1702	 *      btrfs_mount_root() is called. The return value will be used by
  1703	 *      mount_subtree() in mount_subvol().
  1704	 *
  1705	 *   3. Call mount_subvol() to get the dentry of subvolume. Since there is
  1706	 *      "btrfs subvolume set-default", mount_subvol() is called always.
  1707	 */
  1708	static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
  1709			const char *device_name, void *data)
  1710	{
  1711		struct vfsmount *mnt_root;
  1712		struct dentry *root;
  1713		int i;
> 1714		char *subvol_names[SUBVOL_NAMES_COUNT] = {0,};
  1715		u64 subvol_objectid = 0;
  1716		int error = 0;
  1717	
  1718		error = btrfs_parse_subvol_options(data, subvol_names,
  1719					&subvol_objectid);
  1720		if (error) {
  1721			root = ERR_PTR(error);
  1722			goto out;
  1723		}
  1724	
  1725		/* mount device's root (/) */
  1726		mnt_root = vfs_kern_mount(&btrfs_root_fs_type, flags, device_name, data);
  1727		if (PTR_ERR_OR_ZERO(mnt_root) == -EBUSY) {
  1728			if (flags & SB_RDONLY) {
  1729				mnt_root = vfs_kern_mount(&btrfs_root_fs_type,
  1730					flags & ~SB_RDONLY, device_name, data);
  1731			} else {
  1732				mnt_root = vfs_kern_mount(&btrfs_root_fs_type,
  1733					flags | SB_RDONLY, device_name, data);
  1734				if (IS_ERR(mnt_root)) {
  1735					root = ERR_CAST(mnt_root);
  1736					goto out;
  1737				}
  1738	
  1739				down_write(&mnt_root->mnt_sb->s_umount);
  1740				error = btrfs_remount(mnt_root->mnt_sb, &flags, NULL);
  1741				up_write(&mnt_root->mnt_sb->s_umount);
  1742				if (error < 0) {
  1743					root = ERR_PTR(error);
  1744					mntput(mnt_root);
  1745					goto out;
  1746				}
  1747			}
  1748		}
  1749		if (IS_ERR(mnt_root)) {
  1750			root = ERR_CAST(mnt_root);
  1751			goto out;
  1752		}
  1753	
  1754		/* mount_subvol() will free mnt_root */
  1755		root = mount_subvol(subvol_names, subvol_objectid, mnt_root);
  1756	
  1757	out:
  1758		for (i = 0 ; i < SUBVOL_NAMES_COUNT ; i++)
  1759			kfree(subvol_names[i]);
  1760		return root;
  1761	}
  1762	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index bc73fd670702..12d066e8d52c 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -52,6 +52,8 @@ 
 #define CREATE_TRACE_POINTS
 #include <trace/events/btrfs.h>
 
+#define SUBVOL_NAMES_COUNT 5
+
 static const struct super_operations btrfs_super_ops;
 
 /*
@@ -974,12 +976,13 @@  static int btrfs_parse_device_options(const char *options, fmode_t flags,
  *
  * The value is later passed to mount_subvol()
  */
-static int btrfs_parse_subvol_options(const char *options, char **subvol_name,
-		u64 *subvol_objectid)
+static int btrfs_parse_subvol_options(const char *options, char **subvol_names,
+					u64 *subvol_objectid)
 {
 	substring_t args[MAX_OPT_ARGS];
 	char *opts, *orig, *p;
 	int error = 0;
+	int svi = 0;
 	u64 subvolid;
 
 	if (!options)
@@ -1002,12 +1005,17 @@  static int btrfs_parse_subvol_options(const char *options, char **subvol_name,
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_subvol:
-			kfree(*subvol_name);
-			*subvol_name = match_strdup(&args[0]);
-			if (!*subvol_name) {
+			if (svi >= SUBVOL_NAMES_COUNT) {
+				pr_err("BTRFS: too much 'subvol=' mount options\n");
+				error = -E2BIG;
+				goto out;
+			}
+			subvol_names[svi] = match_strdup(&args[0]);
+			if (!subvol_names[svi]) {
 				error = -ENOMEM;
 				goto out;
 			}
+			svi++;
 			break;
 		case Opt_subvolid:
 			error = match_u64(&args[0], &subvolid);
@@ -1429,13 +1437,16 @@  static inline int is_subvolume_inode(struct inode *inode)
 	return 0;
 }
 
-static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
+static struct dentry *mount_subvol(char **subvol_names,
+				   u64 subvol_objectid,
 				   struct vfsmount *mnt)
 {
 	struct dentry *root;
 	int ret;
+	const char *sv;
+	int i;
 
-	if (!subvol_name) {
+	if (!subvol_names[0]) {
 		if (!subvol_objectid) {
 			ret = get_default_subvol_objectid(btrfs_sb(mnt->mnt_sb),
 							  &subvol_objectid);
@@ -1444,17 +1455,27 @@  static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
 				goto out;
 			}
 		}
-		subvol_name = btrfs_get_subvol_name_from_objectid(
+		subvol_names[0] = btrfs_get_subvol_name_from_objectid(
 					btrfs_sb(mnt->mnt_sb), subvol_objectid);
-		if (IS_ERR(subvol_name)) {
-			root = ERR_CAST(subvol_name);
-			subvol_name = NULL;
+		if (IS_ERR(subvol_names[0])) {
+			root = ERR_CAST(subvol_names[0]);
+			subvol_names[0] = NULL;
 			goto out;
 		}
 
 	}
 
-	root = mount_subtree(mnt, subvol_name);
+	for (i = 0 ; i < SUBVOL_NAMES_COUNT ; i++) {
+		if (!subvol_names[i])
+			break;
+
+		root = mount_subtree(mnt, subvol_names[i]);
+		if (!IS_ERR(root)) {
+			sv = subvol_names[i];
+			break;
+		}
+	}
+
 	/* mount_subtree() drops our reference on the vfsmount. */
 	mnt = NULL;
 
@@ -1466,8 +1487,7 @@  static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
 
 		ret = 0;
 		if (!is_subvolume_inode(root_inode)) {
-			btrfs_err(fs_info, "'%s' is not a valid subvolume",
-			       subvol_name);
+			btrfs_err(fs_info, "'%s' is not a valid subvolume", sv);
 			ret = -EINVAL;
 		}
 		if (subvol_objectid && root_objectid != subvol_objectid) {
@@ -1478,7 +1498,7 @@  static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
 			 */
 			btrfs_err(fs_info,
 				  "subvol '%s' does not match subvolid %llu",
-				  subvol_name, subvol_objectid);
+				  sv, subvol_objectid);
 			ret = -EINVAL;
 		}
 		if (ret) {
@@ -1490,7 +1510,6 @@  static struct dentry *mount_subvol(const char *subvol_name, u64 subvol_objectid,
 
 out:
 	mntput(mnt);
-	kfree(subvol_name);
 	return root;
 }
 
@@ -1636,15 +1655,16 @@  static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 {
 	struct vfsmount *mnt_root;
 	struct dentry *root;
-	char *subvol_name = NULL;
+	int i;
+	char *subvol_names[SUBVOL_NAMES_COUNT] = {0,};
 	u64 subvol_objectid = 0;
 	int error = 0;
 
-	error = btrfs_parse_subvol_options(data, &subvol_name,
-					&subvol_objectid);
+	error = btrfs_parse_subvol_options(data, subvol_names,
+				&subvol_objectid);
 	if (error) {
-		kfree(subvol_name);
-		return ERR_PTR(error);
+		root = ERR_PTR(error);
+		goto out;
 	}
 
 	/* mount device's root (/) */
@@ -1658,7 +1678,6 @@  static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 				flags | SB_RDONLY, device_name, data);
 			if (IS_ERR(mnt_root)) {
 				root = ERR_CAST(mnt_root);
-				kfree(subvol_name);
 				goto out;
 			}
 
@@ -1668,21 +1687,21 @@  static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 			if (error < 0) {
 				root = ERR_PTR(error);
 				mntput(mnt_root);
-				kfree(subvol_name);
 				goto out;
 			}
 		}
 	}
 	if (IS_ERR(mnt_root)) {
 		root = ERR_CAST(mnt_root);
-		kfree(subvol_name);
 		goto out;
 	}
 
-	/* mount_subvol() will free subvol_name and mnt_root */
-	root = mount_subvol(subvol_name, subvol_objectid, mnt_root);
+	/* mount_subvol() will free mnt_root */
+	root = mount_subvol(subvol_names, subvol_objectid, mnt_root);
 
 out:
+	for (i = 0 ; i < SUBVOL_NAMES_COUNT ; i++)
+		kfree(subvol_names[i]);
 	return root;
 }