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

[avnish <avnish@imap.linux.ibm.com>]

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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel