grub-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 2/6] ieee1275/powerpc: enables device mapper discovery

From: avnish
Subject: Re: [PATCH v3 2/6] ieee1275/powerpc: enables device mapper discovery
Date: Tue, 25 Jun 2024 16:12:29 +0530 
On 2024-06-25 10:01, Michael Chang wrote:

On Thu, Jun 20, 2024 at 03:14:59PM GMT, avnish wrote:

Hi Vladimir,
We have implemented this code to enable the ieee1275 hint for grub. We had scenarios (in SLES) like the disk had PReP partition followed by an LVM and, inside this LVM , the boot partition. So, we implemented this code to make grub able to generate the hint. Without this code, we were unable to find the hint for the boot process. And, in some scenarios, due the amount of 
disks in the machine, the process could take a lot to finish.
As Vladimir pointed out, the correct search hint should be lvmid/<UUID>, 
so grub will try to assemble the root logical volume first in the
process of looking up the root file system. This way, time won't be
wasted on trying those OpenFirmware raw disks, as they are not the
target.

The problem here is that bringing up the logical volume itself can be
very slow, and grub lacks measures like an LVM filter, which would help
to restrict which devices are scanned and improve performance. In the
end, the patch doesn't help either way.

In my opinion, this patch is more about updating the system bootlist
than achieving the proclaimed boot time enhancement.

Thanks,
Michael



Hi Vladimir and Michael,
Thank you so much for helping us on this patch!
Based on the inputs received. We would like to re analyze the fix done by us. We'll check the fix whether this is really required for the problem we have observed earlier. And based on our analysis, we'll update on it.
For NVMeOF patch series, we would like to drop off this patch from patch series (We'll remove this patch in next version). Vladimir and Michael, please share your reviews on the other patches in the NVMeOf series. We'll do the required changes as per suggestions on the other patches. 
Thank you!

Regards,
Avnish Chouhan
Shall we limit this function call specific to PowerPC? Something like this 
below:

#ifdef __powerpc__
  realname = get_slave_from_dm (name_buf);
  if (realname)
    {
      free (name_buf);
      name_buf = realname;
    }
#endif

Please suggest us.
Thank you so much!

Regards,
Avnish Chouhan


On 2024-06-07 15:05, grub-devel-request@gnu.org wrote:
> Send Grub-devel mailing list submissions to
>    grub-devel@gnu.org
> ------------------------------
>
> Message: 4
> Date: Fri, 7 Jun 2024 11:34:45 +0200
> From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
> To: avnish <avnish@imap.linux.ibm.com>
> Cc: The development of GNU GRUB <grub-devel@gnu.org>, Daniel Kiper
>    <daniel.kiper@oracle.com>, brking@linux.ibm.com,
>    meghanaprakash@in.ibm.com, Diego Domingos <diegodo@br.ibm.com>
> Subject: Re: [PATCH v3 2/6] ieee1275/powerpc: enables device mapper
>    discovery
> Message-ID:
>    <CAEaD8JN1BuOYCxSEP0dQTv7VML6vyCNFg3tSUrLigbrN9uY7AQ@mail.gmail.com>
> Content-Type: text/plain; charset="utf-8"
>
> Le ven. 7 juin 2024, 10:34, avnish <avnish@imap.linux.ibm.com> a écrit :
>
> > On 2024-06-06 21:04, Vladimir 'phcoder' Serbinenko wrote:
> > > 2 problems: * How does dm device ends up on ofpathname? It should be
> > > handled by grub internal logic in most cases and not end up in
> > > of-specific paths * Why not use existing devmapper functions already
> > > present in codebase? Le jeu. 6 juin 2024,
> > > > 2 problems:
> > > * How does dm device ends up on ofpathname? It should be handled by
> > > grub internal logic in most cases and not end up in of-specific paths
> > > * Why not use existing devmapper functions already present in
> > > codebase?
> > >
> >
> > Hi Vladimir,
> > Thank you so much for your response!
> >
> > We have observed that whenever we are dealing with the devices like
> > "/dev/dm-*", the ofpath returns null.
> >
> It's expected behavior
>
> > To resolve this, as no such required functions has been implemented to
> > handle this kind of case. We have done changes based on the
> > requirement
> > that will look into /sys/block/dm-* devices and search slave
> > devices recursively inside slaves directory to find the root disk.
>
> Installing on e.g. LVM is invalid. Only 3 cases can your approach work:
> 1) multipath
> 2) maybe RAID1 as well in some cases
> 3) if ofw assembles raid somehow
> Which one do you have? We need more details.
> This patch enables invalid configs like installing on LVM
>
> >
> > Regards,
> > Avnish Chouhan
> >
> > > Le jeu. 6 juin 2024, 14:40, Avnish Chouhan <avnish@linux.ibm.com> a
> > > écrit :
> > >
> > >> This patch enables the device mapper discovery on ofpath.c.
> > >> Currently,
> > >> when we are dealing with a device like /dev/dm-* the ofpath returns
> > >> null
> > >> since there is no function implemented to handle this case.
> > >>
> > >> This patch implements a function that will look into /sys/block/dm-*
> > >> devices and search recursively inside slaves directory to find the
> > >> root
> > >> disk.
> > >>
> > >> Signed-off-by: Diego Domingos <diegodo@br.ibm.com>
> > >> Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
> > >> ---
> > >> grub-core/osdep/linux/ofpath.c | 64
> > >> +++++++++++++++++++++++++++++++++++++++++-
> > >> 1 file changed, 63 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/grub-core/osdep/linux/ofpath.c
> > >> b/grub-core/osdep/linux/ofpath.c
> > >> index 0f5d54e9f2d..cc849d9c94c 100644
> > >> --- a/grub-core/osdep/linux/ofpath.c
> > >> +++ b/grub-core/osdep/linux/ofpath.c
> > >> @@ -37,6 +37,7 @@
> > >> #include <fcntl.h>
> > >> #include <errno.h>
> > >> #include <ctype.h>
> > >> +#include <dirent.h>
> > >>
> > >> #ifdef __sparc__
> > >> typedef enum
> > >> @@ -755,13 +756,74 @@ strip_trailing_digits (const char *p)
> > >> return new;
> > >> }
> > >>
> > >> +static char *
> > >> +get_slave_from_dm (const char * device)
> > >> +{
> > >> +  char *curr_device, *tmp;
> > >> +  char *directory;
> > >> +  char *ret = NULL;
> > >> +  directory = grub_strdup (device);
> > >> +  tmp = get_basename (directory);
> > >> +  curr_device = grub_strdup (tmp);
> > >> +  *tmp = '\0';
> > >> +
> > >> +  /* Recursively check for slaves devices so we can find the root
> > >> device */
> > >> +  while ((curr_device[0] == 'd') && (curr_device[1] == 'm') &&
> > >> (curr_device[2] == '-'))
> > >> +    {
> > >> +      DIR *dp;
> > >> +      struct dirent *ep;
> > >> +      char* device_path;
> > >> +      device_path = grub_xasprintf ("/sys/block/%s/slaves",
> > >> curr_device);
> > >> +      dp = opendir (device_path);
> > >> +
> > >> +      if (dp != NULL)
> > >> +        {
> > >> +          ep = readdir (dp);
> > >> +
> > >> +          while (ep != NULL)
> > >> +            {
> > >> +              /* avoid some system directories */
> > >> +              if (!strcmp(ep->d_name,"."))
> > >> +                goto next_dir;
> > >> +              if (!strcmp(ep->d_name,".."))
> > >> +                goto next_dir;
> > >> +
> > >> +              free (curr_device);
> > >> +              free (ret);
> > >> +              curr_device = grub_strdup (ep->d_name);
> > >> +              ret = grub_xasprintf ("%s%s", directory,
> > >> curr_device);
> > >> +              break;
> > >> +
> > >> +              next_dir:
> > >> +              ep = readdir (dp);
> > >> +              continue;
> > >> +            }
> > >> +          closedir (dp);
> > >> +        }
> > >> +      else
> > >> +        grub_util_warn (_("cannot open directory `%s'"),
> > >> device_path);
> > >> +      free (device_path);
> > >> +    }
> > >> +  free (directory);
> > >> +  free (curr_device);
> > >> +
> > >> +  return ret;
> > >> +}
> > >> +
> > >> char *
> > >> grub_util_devname_to_ofpath (const char *sys_devname)
> > >> {
> > >> -  char *name_buf, *device, *devnode, *devicenode, *ofpath;
> > >> +  char *name_buf, *device, *devnode, *devicenode, *ofpath,
> > >> *realname;
> > >>
> > >> name_buf = xrealpath (sys_devname);
> > >>
> > >> +  realname = get_slave_from_dm (name_buf);
> > >> +  if (realname)
> > >> +    {
> > >> +      free (name_buf);
> > >> +      name_buf = realname;
> > >> +    }
> > >> +
> > >> device = get_basename (name_buf);
> > >> devnode = strip_trailing_digits (name_buf);
> > >> devicenode = strip_trailing_digits (device);
> > >>
> > >> _______________________________________________
> > >> Grub-devel mailing list
> > >> Grub-devel@gnu.org
> > >> https://lists.gnu.org/mailman/listinfo/grub-devel
> > >
> > >
> > > Links:
> > > ------
> > > [1]
> > >
> > 
https://us-phishalarm-ewt.proofpoint.com/EWT/v1/AdhS1Rd-!-XFVHHiYfI50ul9XDgoDMf5HTGn83xsPPR4XT9wPzmu8earnN0GgLmGjgJ1wrY0OEeVCDtYQvZuy51BEuwbJ2Q36rjgqhgETNXQL18Hn2QOfpE6f2H9XOn8Mici5$
> >
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:
> 
<https://lists.gnu.org/archive/html/grub-devel/attachments/20240607/d6db1c71/attachment.htm>
>
> ------------------------------
>
> Subject: Digest Footer
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>
>
> ------------------------------
>
> End of Grub-devel Digest, Vol 244, Issue 12
> *******************************************

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
reply via email to
[Prev in Thread] Current Thread [Next in Thread]