* [PATCH 0/2] linux: use stat instead of udevadm for partition lookup + cleanup @ 2021-07-08 15:55 Petr Vorel 2021-07-08 15:55 ` [PATCH 1/2] osdep: Introduce major.h and use it Petr Vorel 2021-07-08 15:55 ` [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup Petr Vorel 0 siblings, 2 replies; 18+ messages in thread From: Petr Vorel @ 2021-07-08 15:55 UTC (permalink / raw) To: grub-devel Cc: Petr Vorel, Daniel Kiper, Michael Chang, Mike Gilbert, Jeff Mahoney 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] osdep: Introduce major.h and use it 2021-07-08 15:55 [PATCH 0/2] linux: use stat instead of udevadm for partition lookup + cleanup Petr Vorel @ 2021-07-08 15:55 ` Petr Vorel 2021-07-13 17:36 ` Daniel Kiper 2021-07-08 15:55 ` [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup Petr Vorel 1 sibling, 1 reply; 18+ messages in thread From: Petr Vorel @ 2021-07-08 15:55 UTC (permalink / raw) To: grub-devel Cc: Petr Vorel, Daniel Kiper, Michael Chang, Mike Gilbert, Jeff Mahoney 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] osdep: Introduce major.h and use it 2021-07-08 15:55 ` [PATCH 1/2] osdep: Introduce major.h and use it Petr Vorel @ 2021-07-13 17:36 ` Daniel Kiper 2021-07-13 19:07 ` Petr Vorel 0 siblings, 1 reply; 18+ messages in thread From: Daniel Kiper @ 2021-07-13 17:36 UTC (permalink / raw) To: Petr Vorel; +Cc: grub-devel, Michael Chang, Mike Gilbert, Jeff Mahoney 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] osdep: Introduce major.h and use it 2021-07-13 17:36 ` Daniel Kiper @ 2021-07-13 19:07 ` Petr Vorel 2021-07-14 6:54 ` Petr Vorel 0 siblings, 1 reply; 18+ messages in thread From: Petr Vorel @ 2021-07-13 19:07 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel, Michael Chang, Mike Gilbert, Jeff Mahoney > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] osdep: Introduce major.h and use it 2021-07-13 19:07 ` Petr Vorel @ 2021-07-14 6:54 ` Petr Vorel 2021-07-14 12:55 ` Daniel Kiper 0 siblings, 1 reply; 18+ messages in thread From: Petr Vorel @ 2021-07-14 6:54 UTC (permalink / raw) To: Daniel Kiper, grub-devel, Michael Chang, Mike Gilbert, Jeff Mahoney 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] osdep: Introduce major.h and use it 2021-07-14 6:54 ` Petr Vorel @ 2021-07-14 12:55 ` Daniel Kiper 2021-07-14 14:42 ` Petr Vorel 0 siblings, 1 reply; 18+ messages in thread From: Daniel Kiper @ 2021-07-14 12:55 UTC (permalink / raw) To: Petr Vorel; +Cc: grub-devel, Michael Chang, Mike Gilbert, Jeff Mahoney 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] osdep: Introduce major.h and use it 2021-07-14 12:55 ` Daniel Kiper @ 2021-07-14 14:42 ` Petr Vorel 2021-07-14 14:47 ` Petr Vorel 0 siblings, 1 reply; 18+ messages in thread From: Petr Vorel @ 2021-07-14 14:42 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel, Michael Chang, Mike Gilbert, Jeff Mahoney 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] osdep: Introduce major.h and use it 2021-07-14 14:42 ` Petr Vorel @ 2021-07-14 14:47 ` Petr Vorel 0 siblings, 0 replies; 18+ messages in thread From: Petr Vorel @ 2021-07-14 14:47 UTC (permalink / raw) To: Daniel Kiper, grub-devel, Michael Chang, Mike Gilbert, Jeff Mahoney 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-08 15:55 [PATCH 0/2] linux: use stat instead of udevadm for partition lookup + cleanup Petr Vorel 2021-07-08 15:55 ` [PATCH 1/2] osdep: Introduce major.h and use it Petr Vorel @ 2021-07-08 15:55 ` Petr Vorel 2021-07-13 9:25 ` Paul Menzel 2021-07-13 17:48 ` Daniel Kiper 1 sibling, 2 replies; 18+ messages in thread From: Petr Vorel @ 2021-07-08 15:55 UTC (permalink / raw) To: grub-devel Cc: Jeff Mahoney, Daniel Kiper, Michael Chang, Mike Gilbert, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-08 15:55 ` [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup Petr Vorel @ 2021-07-13 9:25 ` Paul Menzel 2021-07-13 10:52 ` Petr Vorel 2021-07-14 4:16 ` Michael Chang 2021-07-13 17:48 ` Daniel Kiper 1 sibling, 2 replies; 18+ messages in thread From: Paul Menzel @ 2021-07-13 9:25 UTC (permalink / raw) To: Petr Vorel Cc: Jeff Mahoney, Daniel Kiper, Michael Chang, Mike Gilbert, The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-13 9:25 ` Paul Menzel @ 2021-07-13 10:52 ` Petr Vorel 2021-07-14 6:24 ` Michael Chang 2021-07-14 4:16 ` Michael Chang 1 sibling, 1 reply; 18+ messages in thread From: Petr Vorel @ 2021-07-13 10:52 UTC (permalink / raw) To: Paul Menzel Cc: Jeff Mahoney, Daniel Kiper, Michael Chang, Mike Gilbert, The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-13 10:52 ` Petr Vorel @ 2021-07-14 6:24 ` Michael Chang 2021-07-14 6:45 ` Petr Vorel 0 siblings, 1 reply; 18+ messages in thread From: Michael Chang @ 2021-07-14 6:24 UTC (permalink / raw) To: Petr Vorel Cc: Paul Menzel, Jeff Mahoney, Daniel Kiper, Mike Gilbert, The development of GNU GRUB 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-14 6:24 ` Michael Chang @ 2021-07-14 6:45 ` Petr Vorel 0 siblings, 0 replies; 18+ messages in thread From: Petr Vorel @ 2021-07-14 6:45 UTC (permalink / raw) To: Michael Chang Cc: Paul Menzel, Jeff Mahoney, Daniel Kiper, Mike Gilbert, The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-13 9:25 ` Paul Menzel 2021-07-13 10:52 ` Petr Vorel @ 2021-07-14 4:16 ` Michael Chang 1 sibling, 0 replies; 18+ messages in thread From: Michael Chang @ 2021-07-14 4:16 UTC (permalink / raw) To: Paul Menzel Cc: Petr Vorel, Jeff Mahoney, Daniel Kiper, Mike Gilbert, The development of GNU GRUB 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-08 15:55 ` [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup Petr Vorel 2021-07-13 9:25 ` Paul Menzel @ 2021-07-13 17:48 ` Daniel Kiper 2021-07-13 18:03 ` Jeff Mahoney 1 sibling, 1 reply; 18+ messages in thread From: Daniel Kiper @ 2021-07-13 17:48 UTC (permalink / raw) To: Petr Vorel; +Cc: grub-devel, Jeff Mahoney, Michael Chang, Mike Gilbert 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-13 17:48 ` Daniel Kiper @ 2021-07-13 18:03 ` Jeff Mahoney 2021-07-13 19:10 ` Petr Vorel 0 siblings, 1 reply; 18+ messages in thread From: Jeff Mahoney @ 2021-07-13 18:03 UTC (permalink / raw) To: Daniel Kiper, Petr Vorel; +Cc: grub-devel, Michael Chang, Mike Gilbert [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-13 18:03 ` Jeff Mahoney @ 2021-07-13 19:10 ` Petr Vorel 2021-07-14 13:00 ` Daniel Kiper 0 siblings, 1 reply; 18+ messages in thread From: Petr Vorel @ 2021-07-13 19:10 UTC (permalink / raw) To: Jeff Mahoney; +Cc: Daniel Kiper, grub-devel, Michael Chang, Mike Gilbert 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup 2021-07-13 19:10 ` Petr Vorel @ 2021-07-14 13:00 ` Daniel Kiper 0 siblings, 0 replies; 18+ messages in thread From: Daniel Kiper @ 2021-07-14 13:00 UTC (permalink / raw) To: Petr Vorel; +Cc: Jeff Mahoney, grub-devel, Michael Chang, Mike Gilbert 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-07-14 14:47 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-08 15:55 [PATCH 0/2] linux: use stat instead of udevadm for partition lookup + cleanup Petr Vorel 2021-07-08 15:55 ` [PATCH 1/2] osdep: Introduce major.h and use it Petr Vorel 2021-07-13 17:36 ` Daniel Kiper 2021-07-13 19:07 ` Petr Vorel 2021-07-14 6:54 ` Petr Vorel 2021-07-14 12:55 ` Daniel Kiper 2021-07-14 14:42 ` Petr Vorel 2021-07-14 14:47 ` Petr Vorel 2021-07-08 15:55 ` [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup Petr Vorel 2021-07-13 9:25 ` Paul Menzel 2021-07-13 10:52 ` Petr Vorel 2021-07-14 6:24 ` Michael Chang 2021-07-14 6:45 ` Petr Vorel 2021-07-14 4:16 ` Michael Chang 2021-07-13 17:48 ` Daniel Kiper 2021-07-13 18:03 ` Jeff Mahoney 2021-07-13 19:10 ` Petr Vorel 2021-07-14 13:00 ` Daniel Kiper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.