linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vivek Goyal <vgoyal@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Stefan Hajnoczi <stefanha@redhat.com>,
	viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org,
	virtio-fs@redhat.com, linux-kernel@vger.kernel.org
Subject: Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names
Date: Wed, 7 Jul 2021 17:06:36 -0400	[thread overview]
Message-ID: <20210707210636.GC244500@redhat.com> (raw)
In-Reply-To: <20210707210404.GB244500@redhat.com>

On Wed, Jul 07, 2021 at 05:04:04PM -0400, Vivek Goyal wrote:
> On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> > On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > > May be we should modify mount_block_root() code so that it does not
> > > require that extra "\0". Possibly zero initialize page and that should
> > > make sure list_bdev_fs_names() does not have to worry about it.
> > > 
> > > It is possible that a page gets full from the list of filesystems, and
> > > last byte on page is terminating null. In that case just zeroing page
> > > will not help. We can keep track of some sort of end pointer and make
> > > sure we are not searching beyond that for valid filesystem types.
> > > 
> > > end = page + PAGE_SIZE - 1;
> > > 
> > > mount_block_root()
> > > {
> > > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > > 	}
> > > }
> > 
> > Maybe.  To honest I'd prefer to not even touch this unrelated code given
> > how full of landmines it is :)
> 
> Hi Christoph,
> 
> How about following patch. This applies on top of your patches. I noticed
> that Al had suggested to return number of filesystems from helper
> functions. I just did that and used that to iterate in the loop.
> 
> I tested it with a virtual block device (root=/dev/vda1) and it works.
> I also filled page with garbage after allocation to make sure natually
> occurring null is not there in the middle of page to terminate string.
> 
> If you like it, can you please incorporate it in your patches.

I noticed this will break with "root_fs_names=". Sorry, will have to
fix split_fs_names() as well. Will do.

Vivek

> 
> Thanks
> Vivek
> 
> ---
>  fs/filesystems.c   |    5 ++++-
>  include/linux/fs.h |    2 +-
>  init/do_mounts.c   |    7 ++++---
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> Index: redhat-linux/fs/filesystems.c
> ===================================================================
> --- redhat-linux.orig/fs/filesystems.c	2021-07-07 16:12:08.890562576 -0400
> +++ redhat-linux/fs/filesystems.c	2021-07-07 16:27:51.197620063 -0400
> @@ -209,10 +209,11 @@ SYSCALL_DEFINE3(sysfs, int, option, unsi
>  }
>  #endif
>  
> -void __init list_bdev_fs_names(char *buf, size_t size)
> +int __init list_bdev_fs_names(char *buf, size_t size)
>  {
>  	struct file_system_type *p;
>  	size_t len;
> +	int count = 0;
>  
>  	read_lock(&file_systems_lock);
>  	for (p = file_systems; p; p = p->next) {
> @@ -226,8 +227,10 @@ void __init list_bdev_fs_names(char *buf
>  		memcpy(buf, p->name, len);
>  		buf += len;
>  		size -= len;
> +		count++;
>  	}
>  	read_unlock(&file_systems_lock);
> +	return count;
>  }
>  
>  #ifdef CONFIG_PROC_FS
> Index: redhat-linux/include/linux/fs.h
> ===================================================================
> --- redhat-linux.orig/include/linux/fs.h	2021-07-07 15:36:43.224418935 -0400
> +++ redhat-linux/include/linux/fs.h	2021-07-07 16:12:18.232949807 -0400
> @@ -3622,7 +3622,7 @@ int proc_nr_dentry(struct ctl_table *tab
>  		  void *buffer, size_t *lenp, loff_t *ppos);
>  int proc_nr_inodes(struct ctl_table *table, int write,
>  		   void *buffer, size_t *lenp, loff_t *ppos);
> -void __init list_bdev_fs_names(char *buf, size_t size);
> +int __init list_bdev_fs_names(char *buf, size_t size);
>  
>  #define __FMODE_EXEC		((__force int) FMODE_EXEC)
>  #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
> Index: redhat-linux/init/do_mounts.c
> ===================================================================
> --- redhat-linux.orig/init/do_mounts.c	2021-07-07 16:12:08.890562576 -0400
> +++ redhat-linux/init/do_mounts.c	2021-07-07 16:23:32.308889444 -0400
> @@ -391,15 +391,16 @@ void __init mount_block_root(char *name,
>  	char *fs_names = page_address(page);
>  	char *p;
>  	char b[BDEVNAME_SIZE];
> +	int num_fs, i;
>  
>  	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
>  		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
>  	if (root_fs_names)
>  		split_fs_names(fs_names, root_fs_names);
>  	else
> -		list_bdev_fs_names(fs_names, PAGE_SIZE);
> +		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
>  retry:
> -	for (p = fs_names; *p; p += strlen(p)+1) {
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++) {
>  		int err = do_mount_root(name, p, flags, root_mount_data);
>  		switch (err) {
>  			case 0:
> @@ -432,7 +433,7 @@ retry:
>  	printk("List of all partitions:\n");
>  	printk_all_partitions();
>  	printk("No filesystem could mount root, tried: ");
> -	for (p = fs_names; *p; p += strlen(p)+1)
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++)
>  		printk(" %s", p);
>  	printk("\n");
>  	panic("VFS: Unable to mount root fs on %s", b);
> 
> 


  reply	other threads:[~2021-07-07 21:06 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-21  6:26 support booting of arbitrary non-blockdevice file systems v2 Christoph Hellwig
2021-06-21  6:26 ` [PATCH 1/2] init: split get_fs_names Christoph Hellwig
2021-06-21 14:46   ` Al Viro
2021-06-21 14:51     ` Al Viro
2021-06-21 14:53     ` Christoph Hellwig
2021-06-21 14:59       ` Al Viro
2021-06-21 15:09   ` Matthew Wilcox
2021-06-21 15:22     ` Christoph Hellwig
2021-06-21  6:26 ` [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root Christoph Hellwig
2021-06-21 13:31 ` [Virtio-fs] support booting of arbitrary non-blockdevice file systems v2 Stefan Hajnoczi
2021-06-21 14:35 ` Vivek Goyal
2021-06-22  8:12 ` [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names Christoph Hellwig
2021-06-22  8:36   ` [Virtio-fs] " Stefan Hajnoczi
2021-06-29 20:50     ` Vivek Goyal
2021-06-30  5:36       ` Christoph Hellwig
2021-06-30 17:33         ` Vivek Goyal
2021-07-07 21:04         ` Vivek Goyal
2021-07-07 21:06           ` Vivek Goyal [this message]
2021-07-08 12:59             ` Vivek Goyal
2021-07-12 18:21               ` Vivek Goyal
2021-07-13  5:40                 ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210707210636.GC244500@redhat.com \
    --to=vgoyal@redhat.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stefanha@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=virtio-fs@redhat.com \
    --subject='Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).