[PATCH 0/2] linux: use stat instead of udevadm for partition lookup + cleanup

[PATCH 0/2] linux: use stat instead of udevadm for partition lookup + cleanup

[Petr Vorel]

Hi,

I'm backporting Jeff's patch from openSUSE + adding little cleanup.

Kind regards,
Petr

Jeff Mahoney (1):
  grub2: use stat instead of udevadm for partition lookup

Petr Vorel (1):
  osdep: Introduce major.h and use it

 grub-core/osdep/devmapper/getroot.c  |  7 +------
 grub-core/osdep/devmapper/hostdisk.c |  7 +------
 grub-core/osdep/linux/getroot.c      |  7 +------
 grub-core/osdep/linux/hostdisk.c     |  8 ++++++++
 grub-core/osdep/unix/getroot.c       |  7 +------
 include/grub/osdep/major.h           | 30 ++++++++++++++++++++++++++++
 6 files changed, 42 insertions(+), 24 deletions(-)
 create mode 100644 include/grub/osdep/major.h

-- 
2.32.0

[PATCH 1/2] osdep: Introduce major.h and use it

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 grub-core/osdep/devmapper/getroot.c  |  7 +------
 grub-core/osdep/devmapper/hostdisk.c |  7 +------
 grub-core/osdep/linux/getroot.c      |  7 +------
 grub-core/osdep/unix/getroot.c       |  7 +------
 include/grub/osdep/major.h           | 30 ++++++++++++++++++++++++++++
 5 files changed, 34 insertions(+), 24 deletions(-)
 create mode 100644 include/grub/osdep/major.h

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index a13a39c96..9ba5c9865 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -40,12 +40,7 @@
 #include <limits.h>
 #endif
 
-#if defined(MAJOR_IN_MKDEV)
-#include <sys/mkdev.h>
-#elif defined(MAJOR_IN_SYSMACROS)
-#include <sys/sysmacros.h>
-#endif
-
+#include <grub/osdep/major.h>
 #include <libdevmapper.h>
 
 #include <grub/types.h>
diff --git a/grub-core/osdep/devmapper/hostdisk.c b/grub-core/osdep/devmapper/hostdisk.c
index a8afc0c94..c8053728b 100644
--- a/grub-core/osdep/devmapper/hostdisk.c
+++ b/grub-core/osdep/devmapper/hostdisk.c
@@ -11,6 +11,7 @@
 #include <grub/misc.h>
 #include <grub/i18n.h>
 #include <grub/list.h>
+#include <grub/osdep/major.h>
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -24,12 +25,6 @@
 #include <errno.h>
 #include <limits.h>
 
-#if defined(MAJOR_IN_MKDEV)
-#include <sys/mkdev.h>
-#elif defined(MAJOR_IN_SYSMACROS)
-#include <sys/sysmacros.h>
-#endif
-
 #ifdef HAVE_DEVICE_MAPPER
 # include <libdevmapper.h>
 
diff --git a/grub-core/osdep/linux/getroot.c b/grub-core/osdep/linux/getroot.c
index 001b818fe..cd588588e 100644
--- a/grub-core/osdep/linux/getroot.c
+++ b/grub-core/osdep/linux/getroot.c
@@ -35,12 +35,7 @@
 #include <limits.h>
 #endif
 
-#if defined(MAJOR_IN_MKDEV)
-#include <sys/mkdev.h>
-#elif defined(MAJOR_IN_SYSMACROS)
-#include <sys/sysmacros.h>
-#endif
-
+#include <grub/osdep/major.h>
 #include <grub/types.h>
 #include <sys/ioctl.h>         /* ioctl */
 #include <sys/mount.h>
diff --git a/grub-core/osdep/unix/getroot.c b/grub-core/osdep/unix/getroot.c
index 46d7116c6..74f69116d 100644
--- a/grub-core/osdep/unix/getroot.c
+++ b/grub-core/osdep/unix/getroot.c
@@ -51,12 +51,7 @@
 #endif /* ! FLOPPY_MAJOR */
 #endif
 
-#include <sys/types.h>
-#if defined(MAJOR_IN_MKDEV)
-#include <sys/mkdev.h>
-#elif defined(MAJOR_IN_SYSMACROS)
-#include <sys/sysmacros.h>
-#endif
+#include <grub/osdep/major.h>
 
 #if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR)
 # include <grub/util/libzfs.h>
diff --git a/include/grub/osdep/major.h b/include/grub/osdep/major.h
new file mode 100644
index 000000000..e3b545551
--- /dev/null
+++ b/include/grub/osdep/major.h
@@ -0,0 +1,30 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2021  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * Fix for glibc 2.25 is deprecating the namespace pollution of sys/types.h
+ * injecting major(), minor(), and makedev() into the compilation environment.
+ * See configure.ac.
+*/
+
+#include <sys/types.h>
+#ifdef MAJOR_IN_MKDEV
+# include <sys/mkdev.h>
+#elif defined(MAJOR_IN_SYSMACROS)
+# include <sys/sysmacros.h>
+#endif
-- 
2.32.0

[PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Petr Vorel]

From: Jeff Mahoney <jeffm@suse.com>

sysfs_partition_path calls udevadm to resolve the sysfs path for
a block device. That can be accomplished by stating the device node
and using the major/minor to follow the symlinks in /sys/dev/block/.

This cuts the execution time of grub2-mkconfig from 10s to 2s on
my system.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
[ pvorel: include grub/osdep/major.h ]
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 grub-core/osdep/linux/hostdisk.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
index da62f924e..43dc4b0ba 100644
--- a/grub-core/osdep/linux/hostdisk.c
+++ b/grub-core/osdep/linux/hostdisk.c
@@ -31,6 +31,7 @@
 #include <grub/misc.h>
 #include <grub/i18n.h>
 #include <grub/list.h>
+#include <grub/osdep/major.h>
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char *entry)
   char *buf = NULL;
   size_t len = 0;
   char *path = NULL;
+  struct stat st;
+  int ret;
+
+  ret = stat(dev, &st);
+  if (ret == 0 && S_ISBLK(st.st_mode))
+    return xasprintf ("/sys/dev/block/%u:%u/%s",
+		      major (st.st_rdev), minor (st.st_rdev), entry);
 
   argv[0] = "udevadm";
   argv[1] = "info";
-- 
2.32.0

Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Paul Menzel]

Dear Petr, dear Jeff,


Am 08.07.21 um 17:55 schrieb Petr Vorel:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> sysfs_partition_path calls udevadm to resolve the sysfs path for
> a block device. That can be accomplished by stating the device node
> and using the major/minor to follow the symlinks in /sys/dev/block/.
> 
> This cuts the execution time of grub2-mkconfig from 10s to 2s on
> my system.

Petr, where you able to reproduce this issue? Could the specifications 
of Jeff’s system be added to the commit message?

> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> [ pvorel: include grub/osdep/major.h ]
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>   grub-core/osdep/linux/hostdisk.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
> index da62f924e..43dc4b0ba 100644
> --- a/grub-core/osdep/linux/hostdisk.c
> +++ b/grub-core/osdep/linux/hostdisk.c
> @@ -31,6 +31,7 @@
>   #include <grub/misc.h>
>   #include <grub/i18n.h>
>   #include <grub/list.h>
> +#include <grub/osdep/major.h>
>   
>   #include <stdio.h>
>   #include <stdlib.h>
> @@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char *entry)
>     char *buf = NULL;
>     size_t len = 0;
>     char *path = NULL;
> +  struct stat st;
> +  int ret;
> +
> +  ret = stat(dev, &st);
> +  if (ret == 0 && S_ISBLK(st.st_mode))
> +    return xasprintf ("/sys/dev/block/%u:%u/%s",
> +		      major (st.st_rdev), minor (st.st_rdev), entry);
>   
>     argv[0] = "udevadm";
>     argv[1] = "info";

Now the non-block device case has one stat call more executed each time.


Kind regards,

Paul

Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Petr Vorel]

Hi Paul,

> Dear Petr, dear Jeff,


> Am 08.07.21 um 17:55 schrieb Petr Vorel:
> > From: Jeff Mahoney <jeffm@suse.com>

> > sysfs_partition_path calls udevadm to resolve the sysfs path for
> > a block device. That can be accomplished by stating the device node
> > and using the major/minor to follow the symlinks in /sys/dev/block/.

> > This cuts the execution time of grub2-mkconfig from 10s to 2s on
> > my system.

> Petr, where you able to reproduce this issue?
No, I'm sorry, I haven't even tried, because accessing sysfs seems to me as a
quickest way anyway. But agree that we drag this patch in opensuse from 2017
(for 2.02~rc1), it might not be relevant for nowadays systems.

> Could the specifications of Jeff’s system be added to the commit message?
Jeff, Michael, could you verify if it's still relevant?

Kind regards,
Petr

> > Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> > [ pvorel: include grub/osdep/major.h ]
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >   grub-core/osdep/linux/hostdisk.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)

> > diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
> > index da62f924e..43dc4b0ba 100644
> > --- a/grub-core/osdep/linux/hostdisk.c
> > +++ b/grub-core/osdep/linux/hostdisk.c
> > @@ -31,6 +31,7 @@
> >   #include <grub/misc.h>
> >   #include <grub/i18n.h>
> >   #include <grub/list.h>
> > +#include <grub/osdep/major.h>
> >   #include <stdio.h>
> >   #include <stdlib.h>
> > @@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char *entry)
> >     char *buf = NULL;
> >     size_t len = 0;
> >     char *path = NULL;
> > +  struct stat st;
> > +  int ret;
> > +
> > +  ret = stat(dev, &st);
> > +  if (ret == 0 && S_ISBLK(st.st_mode))
> > +    return xasprintf ("/sys/dev/block/%u:%u/%s",
> > +		      major (st.st_rdev), minor (st.st_rdev), entry);
> >     argv[0] = "udevadm";
> >     argv[1] = "info";

> Now the non-block device case has one stat call more executed each time.


> Kind regards,

> Paul

Re: [PATCH 1/2] osdep: Introduce major.h and use it

[Daniel Kiper]

On Thu, Jul 08, 2021 at 05:55:57PM +0200, Petr Vorel wrote:
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  grub-core/osdep/devmapper/getroot.c  |  7 +------
>  grub-core/osdep/devmapper/hostdisk.c |  7 +------
>  grub-core/osdep/linux/getroot.c      |  7 +------
>  grub-core/osdep/unix/getroot.c       |  7 +------
>  include/grub/osdep/major.h           | 30 ++++++++++++++++++++++++++++
>  5 files changed, 34 insertions(+), 24 deletions(-)
>  create mode 100644 include/grub/osdep/major.h

May I ask you to explain in the commit message why this patch is needed?

Daniel

Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Daniel Kiper]

On Thu, Jul 08, 2021 at 05:55:58PM +0200, Petr Vorel wrote:
> From: Jeff Mahoney <jeffm@suse.com>
>
> sysfs_partition_path calls udevadm to resolve the sysfs path for
> a block device. That can be accomplished by stating the device node
> and using the major/minor to follow the symlinks in /sys/dev/block/.
>
> This cuts the execution time of grub2-mkconfig from 10s to 2s on
> my system.
>
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> [ pvorel: include grub/osdep/major.h ]
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  grub-core/osdep/linux/hostdisk.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
> index da62f924e..43dc4b0ba 100644
> --- a/grub-core/osdep/linux/hostdisk.c
> +++ b/grub-core/osdep/linux/hostdisk.c
> @@ -31,6 +31,7 @@
>  #include <grub/misc.h>
>  #include <grub/i18n.h>
>  #include <grub/list.h>
> +#include <grub/osdep/major.h>
>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char *entry)
>    char *buf = NULL;
>    size_t len = 0;
>    char *path = NULL;
> +  struct stat st;
> +  int ret;
> +
> +  ret = stat(dev, &st);

Missing space between "stat" and "(".

> +  if (ret == 0 && S_ISBLK(st.st_mode))

Same for S_ISBLK...

> +    return xasprintf ("/sys/dev/block/%u:%u/%s",
> +		      major (st.st_rdev), minor (st.st_rdev), entry);
>
>    argv[0] = "udevadm";
>    argv[1] = "info";

Do we really need udevadm fallback mechanism? If something went wrong
here for us I do not expect it will work for udevadm either.

Daniel

Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2268 bytes --]

On 7/13/21 1:48 PM, Daniel Kiper wrote:
> On Thu, Jul 08, 2021 at 05:55:58PM +0200, Petr Vorel wrote:
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> sysfs_partition_path calls udevadm to resolve the sysfs path for
>> a block device. That can be accomplished by stating the device node
>> and using the major/minor to follow the symlinks in /sys/dev/block/.
>>
>> This cuts the execution time of grub2-mkconfig from 10s to 2s on
>> my system.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> [ pvorel: include grub/osdep/major.h ]
>> Signed-off-by: Petr Vorel <pvorel@suse.cz>
>> ---
>>  grub-core/osdep/linux/hostdisk.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
>> index da62f924e..43dc4b0ba 100644
>> --- a/grub-core/osdep/linux/hostdisk.c
>> +++ b/grub-core/osdep/linux/hostdisk.c
>> @@ -31,6 +31,7 @@
>>  #include <grub/misc.h>
>>  #include <grub/i18n.h>
>>  #include <grub/list.h>
>> +#include <grub/osdep/major.h>
>>
>>  #include <stdio.h>
>>  #include <stdlib.h>
>> @@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char *entry)
>>    char *buf = NULL;
>>    size_t len = 0;
>>    char *path = NULL;
>> +  struct stat st;
>> +  int ret;
>> +
>> +  ret = stat(dev, &st);
> 
> Missing space between "stat" and "(".
> 
>> +  if (ret == 0 && S_ISBLK(st.st_mode))
> 
> Same for S_ISBLK...
> 
>> +    return xasprintf ("/sys/dev/block/%u:%u/%s",
>> +		      major (st.st_rdev), minor (st.st_rdev), entry);
>>
>>    argv[0] = "udevadm";
>>    argv[1] = "info";
> 
> Do we really need udevadm fallback mechanism? If something went wrong
> here for us I do not expect it will work for udevadm either.

I suspect not.  'udevadm info --query path --name <dev>' does
essentially the same thing but with many more steps and the cost
starting an external program for every block device.  I left it in with
the assumption that it was probably there for a reason and perhaps
udevadm on early udev systems did something more special than just doing
stat + sysfs path building.  I didn't spend any time on validating that
assumption.

-Jeff

-- 
Jeff Mahoney
Director, SUSE Labs Data & Performance


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/2] osdep: Introduce major.h and use it

[Petr Vorel]

> On Thu, Jul 08, 2021 at 05:55:57PM +0200, Petr Vorel wrote:
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >  grub-core/osdep/devmapper/getroot.c  |  7 +------
> >  grub-core/osdep/devmapper/hostdisk.c |  7 +------
> >  grub-core/osdep/linux/getroot.c      |  7 +------
> >  grub-core/osdep/unix/getroot.c       |  7 +------
> >  include/grub/osdep/major.h           | 30 ++++++++++++++++++++++++++++
> >  5 files changed, 34 insertions(+), 24 deletions(-)
> >  create mode 100644 include/grub/osdep/major.h

> May I ask you to explain in the commit message why this patch is needed?
It's just a small cleanup. If you're not against it, sure, I can mention it in
the commit message.

Kind regards,
Petr

> Daniel

Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Petr Vorel]

Hi all,

> On 7/13/21 1:48 PM, Daniel Kiper wrote:
> > On Thu, Jul 08, 2021 at 05:55:58PM +0200, Petr Vorel wrote:
> >> From: Jeff Mahoney <jeffm@suse.com>

> >> sysfs_partition_path calls udevadm to resolve the sysfs path for
> >> a block device. That can be accomplished by stating the device node
> >> and using the major/minor to follow the symlinks in /sys/dev/block/.

> >> This cuts the execution time of grub2-mkconfig from 10s to 2s on
> >> my system.

> >> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> >> [ pvorel: include grub/osdep/major.h ]
> >> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> >> ---
> >>  grub-core/osdep/linux/hostdisk.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)

> >> diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
> >> index da62f924e..43dc4b0ba 100644
> >> --- a/grub-core/osdep/linux/hostdisk.c
> >> +++ b/grub-core/osdep/linux/hostdisk.c
> >> @@ -31,6 +31,7 @@
> >>  #include <grub/misc.h>
> >>  #include <grub/i18n.h>
> >>  #include <grub/list.h>
> >> +#include <grub/osdep/major.h>

> >>  #include <stdio.h>
> >>  #include <stdlib.h>
> >> @@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char *entry)
> >>    char *buf = NULL;
> >>    size_t len = 0;
> >>    char *path = NULL;
> >> +  struct stat st;
> >> +  int ret;
> >> +
> >> +  ret = stat(dev, &st);

> > Missing space between "stat" and "(".

> >> +  if (ret == 0 && S_ISBLK(st.st_mode))

> > Same for S_ISBLK...

> >> +    return xasprintf ("/sys/dev/block/%u:%u/%s",
> >> +		      major (st.st_rdev), minor (st.st_rdev), entry);

> >>    argv[0] = "udevadm";
> >>    argv[1] = "info";

> > Do we really need udevadm fallback mechanism? If something went wrong
> > here for us I do not expect it will work for udevadm either.

> I suspect not.  'udevadm info --query path --name <dev>' does
> essentially the same thing but with many more steps and the cost
> starting an external program for every block device.  I left it in with
> the assumption that it was probably there for a reason and perhaps
> udevadm on early udev systems did something more special than just doing
> stat + sysfs path building.  I didn't spend any time on validating that
> assumption.

As it'd be wort to remove udevadm fallback, I'll stop being lazy and setup VM to
test.

Kind regards,
Petr

> -Jeff

Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Michael Chang]

On Tue, Jul 13, 2021 at 11:25:49AM +0200, Paul Menzel wrote:
> Dear Petr, dear Jeff,
> 
> 
> Am 08.07.21 um 17:55 schrieb Petr Vorel:
> > From: Jeff Mahoney <jeffm@suse.com>
> > 
> > sysfs_partition_path calls udevadm to resolve the sysfs path for
> > a block device. That can be accomplished by stating the device node
> > and using the major/minor to follow the symlinks in /sys/dev/block/.
> > 
> > This cuts the execution time of grub2-mkconfig from 10s to 2s on
> > my system.
> 
> Petr, where you able to reproduce this issue? Could the specifications of
> Jeff’s system be added to the commit message?
> 
> > Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> > [ pvorel: include grub/osdep/major.h ]
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >   grub-core/osdep/linux/hostdisk.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
> > index da62f924e..43dc4b0ba 100644
> > --- a/grub-core/osdep/linux/hostdisk.c
> > +++ b/grub-core/osdep/linux/hostdisk.c
> > @@ -31,6 +31,7 @@
> >   #include <grub/misc.h>
> >   #include <grub/i18n.h>
> >   #include <grub/list.h>
> > +#include <grub/osdep/major.h>
> >   #include <stdio.h>
> >   #include <stdlib.h>
> > @@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char *entry)
> >     char *buf = NULL;
> >     size_t len = 0;
> >     char *path = NULL;
> > +  struct stat st;
> > +  int ret;
> > +
> > +  ret = stat(dev, &st);
> > +  if (ret == 0 && S_ISBLK(st.st_mode))
> > +    return xasprintf ("/sys/dev/block/%u:%u/%s",
> > +		      major (st.st_rdev), minor (st.st_rdev), entry);
> >     argv[0] = "udevadm";
> >     argv[1] = "info";
> 
> Now the non-block device case has one stat call more executed each time.

Practically this should not happen imho, as the device here are returned
by grub_guess_root_devices() to which underlying block devices is used
by a given path or mount point.

Thanks,
Michael

> 
> 
> Kind regards,
> 
> Paul
> 

Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Michael Chang]

On Tue, Jul 13, 2021 at 12:52:12PM +0200, Petr Vorel wrote:
> Hi Paul,
> 
> > Dear Petr, dear Jeff,
> 
> 
> > Am 08.07.21 um 17:55 schrieb Petr Vorel:
> > > From: Jeff Mahoney <jeffm@suse.com>
> 
> > > sysfs_partition_path calls udevadm to resolve the sysfs path for
> > > a block device. That can be accomplished by stating the device node
> > > and using the major/minor to follow the symlinks in /sys/dev/block/.
> 
> > > This cuts the execution time of grub2-mkconfig from 10s to 2s on
> > > my system.
> 
> > Petr, where you able to reproduce this issue?
> No, I'm sorry, I haven't even tried, because accessing sysfs seems to me as a
> quickest way anyway. But agree that we drag this patch in opensuse from 2017
> (for 2.02~rc1), it might not be relevant for nowadays systems.
> 
> > Could the specifications of Jeff’s system be added to the commit message?
> Jeff, Michael, could you verify if it's still relevant?

It is still relevant per my test, although not as considerable as Jeff's
system.

I was using openSUSE Tumbleweed, pretty much all defaults and simple,
btrfs as root file system without any abstraction disks (lvm, mdadm and
encryption).

 NAME MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
 sr0   11:0    1  663M  0 rom  
 vda  253:0    0   20G  0 disk 
 vda1 253:1    0  1.4G  0 part [SWAP]
 vda2 253:2    0 18.6G  0 part /

The output of `time grub2-mkconfig -o /boot/grub2/grub.cfg` with the
patch applied.

 real    0m1.292s
 user    0m0.782s
 sys     0m0.427s

and without

 real    0m2.323s
 user    0m1.295s
 sys     0m0.956s

This cuts the execution to somewhere near 55%, which is pretty signifcant.

Please note if testing on lvm, there wouldn't have difference on the
execution time as libdevmapper is used instead of udevadm to get the
partition start.

Thanks,
Michael


> 
> Kind regards,
> Petr
> 
> > > Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> > > [ pvorel: include grub/osdep/major.h ]
> > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > ---
> > >   grub-core/osdep/linux/hostdisk.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> 
> > > diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
> > > index da62f924e..43dc4b0ba 100644
> > > --- a/grub-core/osdep/linux/hostdisk.c
> > > +++ b/grub-core/osdep/linux/hostdisk.c
> > > @@ -31,6 +31,7 @@
> > >   #include <grub/misc.h>
> > >   #include <grub/i18n.h>
> > >   #include <grub/list.h>
> > > +#include <grub/osdep/major.h>
> > >   #include <stdio.h>
> > >   #include <stdlib.h>
> > > @@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char *entry)
> > >     char *buf = NULL;
> > >     size_t len = 0;
> > >     char *path = NULL;
> > > +  struct stat st;
> > > +  int ret;
> > > +
> > > +  ret = stat(dev, &st);
> > > +  if (ret == 0 && S_ISBLK(st.st_mode))
> > > +    return xasprintf ("/sys/dev/block/%u:%u/%s",
> > > +		      major (st.st_rdev), minor (st.st_rdev), entry);
> > >     argv[0] = "udevadm";
> > >     argv[1] = "info";
> 
> > Now the non-block device case has one stat call more executed each time.
> 
> 
> > Kind regards,
> 
> > Paul
> 

Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Petr Vorel]

Hi Michael,

> On Tue, Jul 13, 2021 at 12:52:12PM +0200, Petr Vorel wrote:
> > Hi Paul,

> > > Dear Petr, dear Jeff,


> > > Am 08.07.21 um 17:55 schrieb Petr Vorel:
> > > > From: Jeff Mahoney <jeffm@suse.com>

> > > > sysfs_partition_path calls udevadm to resolve the sysfs path for
> > > > a block device. That can be accomplished by stating the device node
> > > > and using the major/minor to follow the symlinks in /sys/dev/block/.

> > > > This cuts the execution time of grub2-mkconfig from 10s to 2s on
> > > > my system.

> > > Petr, where you able to reproduce this issue?
> > No, I'm sorry, I haven't even tried, because accessing sysfs seems to me as a
> > quickest way anyway. But agree that we drag this patch in opensuse from 2017
> > (for 2.02~rc1), it might not be relevant for nowadays systems.

> > > Could the specifications of Jeff’s system be added to the commit message?
> > Jeff, Michael, could you verify if it's still relevant?

> It is still relevant per my test, although not as considerable as Jeff's
> system.

> I was using openSUSE Tumbleweed, pretty much all defaults and simple,
> btrfs as root file system without any abstraction disks (lvm, mdadm and
> encryption).

>  NAME MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
>  sr0   11:0    1  663M  0 rom  
>  vda  253:0    0   20G  0 disk 
>  vda1 253:1    0  1.4G  0 part [SWAP]
>  vda2 253:2    0 18.6G  0 part /

> The output of `time grub2-mkconfig -o /boot/grub2/grub.cfg` with the
> patch applied.

>  real    0m1.292s
>  user    0m0.782s
>  sys     0m0.427s

> and without

>  real    0m2.323s
>  user    0m1.295s
>  sys     0m0.956s

> This cuts the execution to somewhere near 55%, which is pretty signifcant.

Thanks a lot for testing. I'll send v2 shortly.

> Please note if testing on lvm, there wouldn't have difference on the
> execution time as libdevmapper is used instead of udevadm to get the
> partition start.
Good to know (some of my system have lvm).

Kind regards,
Petr

> Thanks,
> Michael

Re: [PATCH 1/2] osdep: Introduce major.h and use it

[Petr Vorel]

Hi Daniel,

> > On Thu, Jul 08, 2021 at 05:55:57PM +0200, Petr Vorel wrote:
> > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > ---
> > >  grub-core/osdep/devmapper/getroot.c  |  7 +------
> > >  grub-core/osdep/devmapper/hostdisk.c |  7 +------
> > >  grub-core/osdep/linux/getroot.c      |  7 +------
> > >  grub-core/osdep/unix/getroot.c       |  7 +------
> > >  include/grub/osdep/major.h           | 30 ++++++++++++++++++++++++++++
> > >  5 files changed, 34 insertions(+), 24 deletions(-)
> > >  create mode 100644 include/grub/osdep/major.h

> > May I ask you to explain in the commit message why this patch is needed?
> It's just a small cleanup. If you're not against it, sure, I can mention it in
> the commit message.
Ah, you probably mean to put the description from major.h also to the commit
message.

Kind regards,
Petr

+/*
+ * Fix for glibc 2.25 is deprecating the namespace pollution of sys/types.h
+ * injecting major(), minor(), and makedev() into the compilation environment.
+ * See configure.ac.
+*/

> Kind regards,
> Petr

> > Daniel

Re: [PATCH 1/2] osdep: Introduce major.h and use it

[Daniel Kiper]

On Wed, Jul 14, 2021 at 08:54:29AM +0200, Petr Vorel wrote:
> Hi Daniel,
>
> > > On Thu, Jul 08, 2021 at 05:55:57PM +0200, Petr Vorel wrote:
> > > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > > ---
> > > >  grub-core/osdep/devmapper/getroot.c  |  7 +------
> > > >  grub-core/osdep/devmapper/hostdisk.c |  7 +------
> > > >  grub-core/osdep/linux/getroot.c      |  7 +------
> > > >  grub-core/osdep/unix/getroot.c       |  7 +------
> > > >  include/grub/osdep/major.h           | 30 ++++++++++++++++++++++++++++
> > > >  5 files changed, 34 insertions(+), 24 deletions(-)
> > > >  create mode 100644 include/grub/osdep/major.h
>
> > > May I ask you to explain in the commit message why this patch is needed?
> > It's just a small cleanup. If you're not against it, sure, I can mention it in
> > the commit message.

I am OK with the cleanup.

> Ah, you probably mean to put the description from major.h also to the commit
> message.

To some extent. I think it should be phrased a bit better in the commit message.

> Kind regards,
> Petr
>
> +/*
> + * Fix for glibc 2.25 is deprecating the namespace pollution of sys/types.h
> + * injecting major(), minor(), and makedev() into the compilation environment.
> + * See configure.ac.

It seems to me "See configure.ac." is not relevant in the GRUB source.
So, probably it should be replaced with some text from glibc configure.ac.

Daniel

Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

[Daniel Kiper]

On Tue, Jul 13, 2021 at 09:10:39PM +0200, Petr Vorel wrote:
> Hi all,
>
> > On 7/13/21 1:48 PM, Daniel Kiper wrote:
> > > On Thu, Jul 08, 2021 at 05:55:58PM +0200, Petr Vorel wrote:
> > >> From: Jeff Mahoney <jeffm@suse.com>
>
> > >> sysfs_partition_path calls udevadm to resolve the sysfs path for
> > >> a block device. That can be accomplished by stating the device node
> > >> and using the major/minor to follow the symlinks in /sys/dev/block/.
>
> > >> This cuts the execution time of grub2-mkconfig from 10s to 2s on
> > >> my system.
>
> > >> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> > >> [ pvorel: include grub/osdep/major.h ]
> > >> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > >> ---
> > >>  grub-core/osdep/linux/hostdisk.c | 8 ++++++++
> > >>  1 file changed, 8 insertions(+)
>
> > >> diff --git a/grub-core/osdep/linux/hostdisk.c b/grub-core/osdep/linux/hostdisk.c
> > >> index da62f924e..43dc4b0ba 100644
> > >> --- a/grub-core/osdep/linux/hostdisk.c
> > >> +++ b/grub-core/osdep/linux/hostdisk.c
> > >> @@ -31,6 +31,7 @@
> > >>  #include <grub/misc.h>
> > >>  #include <grub/i18n.h>
> > >>  #include <grub/list.h>
> > >> +#include <grub/osdep/major.h>
>
> > >>  #include <stdio.h>
> > >>  #include <stdlib.h>
> > >> @@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char *entry)
> > >>    char *buf = NULL;
> > >>    size_t len = 0;
> > >>    char *path = NULL;
> > >> +  struct stat st;
> > >> +  int ret;
> > >> +
> > >> +  ret = stat(dev, &st);
>
> > > Missing space between "stat" and "(".
>
> > >> +  if (ret == 0 && S_ISBLK(st.st_mode))
>
> > > Same for S_ISBLK...
>
> > >> +    return xasprintf ("/sys/dev/block/%u:%u/%s",
> > >> +		      major (st.st_rdev), minor (st.st_rdev), entry);
>
> > >>    argv[0] = "udevadm";
> > >>    argv[1] = "info";
>
> > > Do we really need udevadm fallback mechanism? If something went wrong
> > > here for us I do not expect it will work for udevadm either.
>
> > I suspect not.  'udevadm info --query path --name <dev>' does
> > essentially the same thing but with many more steps and the cost
> > starting an external program for every block device.  I left it in with
> > the assumption that it was probably there for a reason and perhaps
> > udevadm on early udev systems did something more special than just doing
> > stat + sysfs path building.  I didn't spend any time on validating that
> > assumption.
>
> As it'd be wort to remove udevadm fallback, I'll stop being lazy and setup VM to
> test.

Please do. I prefer to remove this fallback because it will be mostly
dead code after your fix.

Daniel

Re: [PATCH 1/2] osdep: Introduce major.h and use it

[Petr Vorel]

Hi Daniel,

> On Wed, Jul 14, 2021 at 08:54:29AM +0200, Petr Vorel wrote:
> > Hi Daniel,

> > > > On Thu, Jul 08, 2021 at 05:55:57PM +0200, Petr Vorel wrote:
> > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > > > ---
> > > > >  grub-core/osdep/devmapper/getroot.c  |  7 +------
> > > > >  grub-core/osdep/devmapper/hostdisk.c |  7 +------
> > > > >  grub-core/osdep/linux/getroot.c      |  7 +------
> > > > >  grub-core/osdep/unix/getroot.c       |  7 +------
> > > > >  include/grub/osdep/major.h           | 30 ++++++++++++++++++++++++++++
> > > > >  5 files changed, 34 insertions(+), 24 deletions(-)
> > > > >  create mode 100644 include/grub/osdep/major.h

> > > > May I ask you to explain in the commit message why this patch is needed?
> > > It's just a small cleanup. If you're not against it, sure, I can mention it in
> > > the commit message.

> I am OK with the cleanup.
thx for ack!

> > Ah, you probably mean to put the description from major.h also to the commit
> > message.

> To some extent. I think it should be phrased a bit better in the commit message.
sure.

> > Kind regards,
> > Petr

> > +/*
> > + * Fix for glibc 2.25 is deprecating the namespace pollution of sys/types.h
> > + * injecting major(), minor(), and makedev() into the compilation environment.
> > + * See configure.ac.

> It seems to me "See configure.ac." is not relevant in the GRUB source.
> So, probably it should be replaced with some text from glibc configure.ac.

I didn't know how to link AC_HEADER_MAJOR with include/grub/osdep/major.h.
Because in the future when glibc 2.25 is old enough the header will be removed
and AC_HEADER_MAJOR might be left in configure.ac (name is different from
MAJOR_IN_{MKDEV,SYSMACROS}). But I can note this in commit message instead.

Kind regards,
Petr

> Daniel

Re: [PATCH 1/2] osdep: Introduce major.h and use it

[Petr Vorel]

Hi Daniel,

...
> > > +/*
> > > + * Fix for glibc 2.25 is deprecating the namespace pollution of sys/types.h
> > > + * injecting major(), minor(), and makedev() into the compilation environment.
> > > + * See configure.ac.

> > It seems to me "See configure.ac." is not relevant in the GRUB source.
> > So, probably it should be replaced with some text from glibc configure.ac.

> I didn't know how to link AC_HEADER_MAJOR with include/grub/osdep/major.h.
> Because in the future when glibc 2.25 is old enough the header will be removed
> and AC_HEADER_MAJOR might be left in configure.ac (name is different from
> MAJOR_IN_{MKDEV,SYSMACROS}). But I can note this in commit message instead.
But thinking about it twice also note in configure.ac instead (+ commit message)
would be good. But that's just a minor detail.

Kind regards,
Petr

> Kind regards,
> Petr

> > Daniel