Re: [PATCH 1/3] 9p: add support for root file systems

[Changbin Du <changbin.du@gmail.com>]

On Wed, May 05, 2021 at 09:50:50PM +0900, Dominique Martinet wrote:
> Why has this only been sent to direct maintainers and linux-doc@vger ?
> Please resend to at least v9fs-developer@sf, linux-fs@vger and whoever
> get_maintainer.pl deems appropriate for init/do_mounts.c changes
> (added the two lists to this mail)
>
I made a typing mistake and then...

> 
> As a whole for the series: I'm personally not sure I'd encourage this,
> it can currently be done with an initrd if someone cares enough and if
> we're going to add all remote filesystems that way there's going to be
> no end to it.
> 
> That being said, I'm not 100% opposed to it if there is demand; but I'll
> expect you to help with whatever bug reports people come up with using
> this option.
> 
No problem, I can help with any issues related to this. It is worth doint this
especially for some virtulizaion scenarioes.

> 
> Changbin Du wrote on Wed, May 05, 2021 at 08:07:46PM +0800:
> > diff --git a/fs/9p/v9fsroot.c b/fs/9p/v9fsroot.c
> > new file mode 100644
> > index 000000000000..7dd91cc3814f
> > --- /dev/null
> > +++ b/fs/9p/v9fsroot.c
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SMB root file system support
> 
> copy-paste error from fs/cifs/cifsroot.c
> 
Sorry, I'll fix it.

> > + *
> > + * Copyright (c) 2021 Changbin Du <changbin.du@gmail.com>
> > + */
> > +#include <linux/init.h>
> > +#include <linux/fs.h>
> > +#include <linux/types.h>
> > +#include <linux/ctype.h>
> > +#include <linux/string.h>
> > +#include <linux/root_dev.h>
> > +#include <linux/kernel.h>
> > +
> > +static char root_dev[2048] __initdata = "";
> > +static char root_opts[1024] __initdata = "";
> > +
> > +/* v9fsroot=<path>[,options] */
> > +static int __init v9fs_root_setup(char *line)
> > +{
> > +	char *s;
> > +	int len;
> > +
> > +	ROOT_DEV = Root_V9FS;
> > +
> > +	if (strlen(line) >= 1) {
> > +		/* make s point to ',' or '\0' at end of line */
> > +		s = strchrnul(line, ',');
> > +		/* len is strlen(unc) + '\0' */
> > +		len = s - line + 1;
> > +		if (len > sizeof(root_dev)) {
> > +			printk(KERN_ERR "Root-V9FS: path too long\n");
> 
> 9p has its own p9_debug helper; it's a bit awkward because even
> P9_DEBUG_ERROR right now is not displayed by default so I'm not against
> pr_err or pr_warn for important messages (I'd like to replace all such
> messages by either at some point), but at least stick to pr_xyz(...)
> rather than printk(KERN_XYZ...)
> 
okay, will use pr_err() instead then.

> > +			return 1;
> > +		}
> > +		strlcpy(root_dev, line, len);
> > +		if (*s) {
> > +			int n = snprintf(root_opts,
> > +					 sizeof(root_opts), "%s,%s",
> > +					 DEFAULT_MNT_OPTS, s + 1);
> 
> Did you actually run with this? DEFAULT_MNT_OPTS is not defined here.
> 
oops, my bad that I forgot to test the latest change.

> > +			if (n >= sizeof(root_opts)) {
> > +				printk(KERN_ERR "Root-V9FS: mount options string too long\n");
> > +				root_opts[sizeof(root_opts)-1] = '\0';
> > +				return 1;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 1;
> 
> I'm also surprised this only ever returns 1, the cifs codes does the
> same but I'd be surprised the mount can work without a tag so there
> certainly should be some difference between working and not working?
>
I think this returns 1 just means the "v9fsroot=" kernel parameter is handled
so do not need further processing. But this doesn't mean the mount options can
work.

> I'd also expect ROOT_DEV to only be set on success, this doesn't make
> sense to trigger 9p mounting mechanisms with something that can't be
> mounted, leading to a long timeout (5+10+15+30*2 seconds) before a hard
> error when the hard error can be given right away.
> 
Sure, will only set ROOT_DEV after basic option checking.

> > +}
> > +
> > +__setup("v9fsroot=", v9fs_root_setup);
> > +
> > +int __init v9fs_root_data(char **dev, char **opts)
> > +{
> > +	if (!root_dev[0]) {
> > +		printk(KERN_ERR "Root-V9FS: no rootdev specified\n");
> > +		return -1;
> > +	}
> > +
> > +	*dev = root_dev;
> > +	*opts = root_opts;
> > +
> > +	return 0;
> > +}
> > diff --git a/include/linux/root_dev.h b/include/linux/root_dev.h
> > index 4e78651371ba..becd0ee2ff87 100644
> > --- a/include/linux/root_dev.h
> > +++ b/include/linux/root_dev.h
> > @@ -9,6 +9,7 @@
> >  enum {
> >  	Root_NFS = MKDEV(UNNAMED_MAJOR, 255),
> >  	Root_CIFS = MKDEV(UNNAMED_MAJOR, 254),
> > +	Root_V9FS = MKDEV(UNNAMED_MAJOR, 253),
> >  	Root_RAM0 = MKDEV(RAMDISK_MAJOR, 0),
> >  	Root_RAM1 = MKDEV(RAMDISK_MAJOR, 1),
> >  	Root_FD0 = MKDEV(FLOPPY_MAJOR, 0),
> > diff --git a/init/do_mounts.c b/init/do_mounts.c
> > index a78e44ee6adb..60af89983a6b 100644
> > --- a/init/do_mounts.c
> > +++ b/init/do_mounts.c
> > @@ -287,6 +287,8 @@ dev_t name_to_dev_t(const char *name)
> >  		return Root_NFS;
> >  	if (strcmp(name, "/dev/cifs") == 0)
> >  		return Root_CIFS;
> > +	if (strcmp(name, "/dev/v9fs") == 0)
> > +		return Root_V9FS;
> >  	if (strcmp(name, "/dev/ram") == 0)
> >  		return Root_RAM0;
> >  #ifdef CONFIG_BLOCK
> > @@ -536,6 +538,43 @@ static int __init mount_cifs_root(void)
> >  }
> >  #endif
> >  
> > +#ifdef CONFIG_9P_FS_ROOT
> > +
> > +extern int v9fs_root_data(char **dev, char **opts);
> > +
> > +#define V9FSROOT_TIMEOUT_MIN	5
> > +#define V9FSROOT_TIMEOUT_MAX	30
> > +#define V9FSROOT_RETRY_MAX	5
> > +
> > +static int __init mount_v9fs_root(void)
> > +{
> > +	char *root_dev, *root_data;
> > +	unsigned int timeout;
> > +	int try, err;
> > +
> > +	err = v9fs_root_data(&root_dev, &root_data);
> > +	if (err != 0)
> > +		return 0;
> > +
> > +	timeout = V9FSROOT_TIMEOUT_MIN;
> > +	for (try = 1; ; try++) {
> > +		err = do_mount_root(root_dev, "9p",
> > +				    root_mountflags, root_data);
> > +		if (err == 0)
> > +			return 1;
> > +		if (try > V9FSROOT_RETRY_MAX)
> > +			break;
> > +
> > +		/* Wait, in case the server refused us immediately */
> > +		ssleep(timeout);
> > +		timeout <<= 1;
> > +		if (timeout > V9FSROOT_TIMEOUT_MAX)
> > +			timeout = V9FSROOT_TIMEOUT_MAX;
> > +	}
> > +	return 0;
> > +}
> > +#endif
> > +
> >  void __init mount_root(void)
> >  {
> >  #ifdef CONFIG_ROOT_NFS
> > @@ -552,6 +591,13 @@ void __init mount_root(void)
> >  		return;
> >  	}
> >  #endif
> > +#ifdef CONFIG_9P_FS_ROOT
> > +	if (ROOT_DEV == Root_V9FS) {
> > +		if (!mount_v9fs_root())
> > +			printk(KERN_ERR "VFS: Unable to mount root fs via 9p.\n");
> > +		return;
> > +	}
> > +#endif
> >  #ifdef CONFIG_BLOCK
> >  	{
> >  		int err = create_dev("/dev/root", ROOT_DEV);
> -- 
> Dominique

-- 
Cheers,
Changbin Du