[06/24] md: open code vfs_stat in md_setup_drive
diff mbox series

Message ID 20200721162818.197315-7-hch@lst.de
State New
Headers show
Series
  • [01/24] fs: refactor do_mount
Related show

Commit Message

Christoph Hellwig July 21, 2020, 4:28 p.m. UTC
Instead of passing a kernel pointer to vfs_stat by relying on the
implicit set_fs(KERNEL_DS) in md_setup_drive, just open code the
trivial getattr, and use the opportunity to move a little bit more
code from the caller into the new helper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/md/md-autodetect.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

Comments

Al Viro July 21, 2020, 4:55 p.m. UTC | #1
On Tue, Jul 21, 2020 at 06:28:00PM +0200, Christoph Hellwig wrote:
> Instead of passing a kernel pointer to vfs_stat by relying on the
> implicit set_fs(KERNEL_DS) in md_setup_drive, just open code the
> trivial getattr, and use the opportunity to move a little bit more
> code from the caller into the new helper.

Ugh...

> +static void __init md_lookup_dev(const char *devname, dev_t *dev)
> +{
> +	struct kstat stat;
> +	struct path path;
> +	char filename[64];
> +
> +	if (strncmp(devname, "/dev/", 5) == 0)
> +		devname += 5;
> +	snprintf(filename, 63, "/dev/%s", devname);
> +
> +	if (!kern_path(filename, LOOKUP_FOLLOW, &path) &&
> +	    !vfs_getattr(&path, &stat, STATX_BASIC_STATS, AT_NO_AUTOMOUNT) &&
> +	    S_ISBLK(stat.mode))
> +		*dev = new_decode_dev(stat.rdev);
> +	path_put(&path);
> +}

How about fs/for_init.c and putting the damn helpers there?  With
calling conventions as close to syscalls as possible, and a fat
comment regarding their intended use being _ONLY_ the setup
in should-have-been-done-in-userland parts of init?

I really want to keep the surface as small as possible - we had
fun shite several releases ago when somebody tried that kind of
crap (with open(), IIRC).  Let's not go there again...
Christoph Hellwig July 21, 2020, 6:27 p.m. UTC | #2
On Tue, Jul 21, 2020 at 05:55:39PM +0100, Al Viro wrote:
> How about fs/for_init.c and putting the damn helpers there?  With
> calling conventions as close to syscalls as possible, and a fat
> comment regarding their intended use being _ONLY_ the setup
> in should-have-been-done-in-userland parts of init?

Where do you want the prototypes to go?  Also do you want devtmpfs
use the same helpers, which then't can't be marked __init (mount,
chdir, chroot), or separate copies?
Al Viro July 22, 2020, 7:44 a.m. UTC | #3
On Tue, Jul 21, 2020 at 08:27:01PM +0200, Christoph Hellwig wrote:
> On Tue, Jul 21, 2020 at 05:55:39PM +0100, Al Viro wrote:
> > How about fs/for_init.c and putting the damn helpers there?  With
> > calling conventions as close to syscalls as possible, and a fat
> > comment regarding their intended use being _ONLY_ the setup
> > in should-have-been-done-in-userland parts of init?
> 
> Where do you want the prototypes to go?  Also do you want devtmpfs
> use the same helpers, which then't can't be marked __init (mount,
> chdir, chroot), or separate copies?

Hmm...  mount still can be __init (devtmpfs_mount() is), and I suspect
devtmpfs_setup() could also be made such - just turn devtmpfsd()
into
static int __init devtmpfsd(void *p)
{
        int err = devtmpfs_setup(p);

	if (!err)
		devtmpfsd_real();	/* never returns */
	return err;
}
and you are done.  As for the prototypes... include/linux/init_syscalls.h,
perhaps?
Christoph Hellwig July 22, 2020, 2:05 p.m. UTC | #4
On Wed, Jul 22, 2020 at 08:44:32AM +0100, Al Viro wrote:
> On Tue, Jul 21, 2020 at 08:27:01PM +0200, Christoph Hellwig wrote:
> > On Tue, Jul 21, 2020 at 05:55:39PM +0100, Al Viro wrote:
> > > How about fs/for_init.c and putting the damn helpers there?  With
> > > calling conventions as close to syscalls as possible, and a fat
> > > comment regarding their intended use being _ONLY_ the setup
> > > in should-have-been-done-in-userland parts of init?
> > 
> > Where do you want the prototypes to go?  Also do you want devtmpfs
> > use the same helpers, which then't can't be marked __init (mount,
> > chdir, chroot), or separate copies?
> 
> Hmm...  mount still can be __init (devtmpfs_mount() is), and I suspect
> devtmpfs_setup() could also be made such - just turn devtmpfsd()
> into
> static int __init devtmpfsd(void *p)
> {
>         int err = devtmpfs_setup(p);
> 
> 	if (!err)
> 		devtmpfsd_real();	/* never returns */
> 	return err;
> }
> and you are done.

Yes, that seems to work.  We can obviously call non-__init functions
from __init ones, and kthread_run doesn't seem to care if it gets passed
a __init function.

Here is what I have now:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/init_path

Patch
diff mbox series

diff --git a/drivers/md/md-autodetect.c b/drivers/md/md-autodetect.c
index 14b6e86814c061..1e8f1df257a112 100644
--- a/drivers/md/md-autodetect.c
+++ b/drivers/md/md-autodetect.c
@@ -8,6 +8,7 @@ 
 #include <linux/raid/detect.h>
 #include <linux/raid/md_u.h>
 #include <linux/raid/md_p.h>
+#include <linux/namei.h>
 #include "md.h"
 
 /*
@@ -119,6 +120,23 @@  static int __init md_setup(char *str)
 	return 1;
 }
 
+static void __init md_lookup_dev(const char *devname, dev_t *dev)
+{
+	struct kstat stat;
+	struct path path;
+	char filename[64];
+
+	if (strncmp(devname, "/dev/", 5) == 0)
+		devname += 5;
+	snprintf(filename, 63, "/dev/%s", devname);
+
+	if (!kern_path(filename, LOOKUP_FOLLOW, &path) &&
+	    !vfs_getattr(&path, &stat, STATX_BASIC_STATS, AT_NO_AUTOMOUNT) &&
+	    S_ISBLK(stat.mode))
+		*dev = new_decode_dev(stat.rdev);
+	path_put(&path);
+}
+
 static void __init md_setup_drive(struct md_setup_args *args)
 {
 	char *devname = args->device_names;
@@ -138,21 +156,14 @@  static void __init md_setup_drive(struct md_setup_args *args)
 	}
 
 	for (i = 0; i < MD_SB_DISKS && devname != NULL; i++) {
-		struct kstat stat;
-		char *p;
-		char comp_name[64];
 		dev_t dev;
+		char *p;
 
 		p = strchr(devname, ',');
 		if (p)
 			*p++ = 0;
-
 		dev = name_to_dev_t(devname);
-		if (strncmp(devname, "/dev/", 5) == 0)
-			devname += 5;
-		snprintf(comp_name, 63, "/dev/%s", devname);
-		if (vfs_stat(comp_name, &stat) == 0 && S_ISBLK(stat.mode))
-			dev = new_decode_dev(stat.rdev);
+		md_lookup_dev(devname, &dev);
 		if (!dev) {
 			pr_warn("md: Unknown device name: %s\n", devname);
 			break;