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
>