Re: [RFC] ieee1275: Avoiding many unnecessary open/close

[diegodo]

On 2020-09-24 20:58, Eric Snowberg wrote:
> > On Sep 24, 2020, at 11:11 AM, diegodo <diegodo@linux.vnet.ibm.com> 
> > wrote:
> >
> > Hi,
> >
> > we are facing some performance issues with ieee1275 platform (ppc64le 
> > architecture to be more specific) and I would like to suggest a couple 
> > of changes to try to avoid them.
> >
> > Sometimes the system can take more than 10 minutes to boot (in fact, 
> > it can takes more than 30 and 40 minutes in some cases). From my 
> > investigations, I noticied the following points:
> >
> > 1. Some types of storage devices (NVMe, Fibre Channel, ...) can have a 
> > long time for shutdown notification. Our investigation here showed us 
> > this shutdown increase with the size of the storage. For example, 
> > 6.4TB NVMe can takes something like 7 seconds just to issue a shutdown 
> > notification.
> >
> > 2. The grub calls a lot for OPEN/READ/CLOSE for each device. I ran 
> > some tests with just one disk (NVMe 6.4TB) and found that, even with 
> > one disk, grub call the OPEN/CLOSE functions more than 60 times. 
> > Considering the opening time of the device something like 1.5 seconds, 
> > we have almost 9 minutes just opening and closing the disk.
> >
> > I did some changes in the code and the time during the boot dropped 
> > drastically - from 492s to only 15s.
> >
> > The idea is to handle the _actual close_ while _opening_ the disk. So, 
> > during the _grub_ofdisk_open_ function, we check if the requested disk 
> > is the same as the already opened and close the previous one if we are 
> > dealing with a different disk. Yet, I removed the OPEN/CLOSE calls 
> > inside the _grub_ofdisk_get_block_size_ function and let it just 
> > checking for the block size.
> >
> > What are your suggestions? Is there a better way to handle this? I 
> > really appreciate your help here.
>
> I ran into the same issue with the ieee1275 platform on SPARC when 
> using
> ofdisk. It would take 20+ minutes to get to the GRUB menu.  I found the 
> same
> problem, open and close can take a long time and upper level grub code 
> would
> issue hundreds of them. I ended up writing obdisk.c. Within it, each 
> disk is
> only opened once. I tried to make it as generic as possible. You might 
> consider
> moving from ofdisk to obdisk.  If you did, ppc64le device specific code 
> would
> need to be included in iterate_devtree.
>
> It also includes new features not found in ofdisk, such as hot-plug 
> support and
> the ability to find disks that don’t have a device alias.
>
>
> > Thanks
> >
> >
> > ---
> > grub-core/disk/ieee1275/ofdisk.c | 63 +++++++++++++++++---------------
> > 1 file changed, 34 insertions(+), 29 deletions(-)
> >
> > diff --git a/grub-core/disk/ieee1275/ofdisk.c 
> > b/grub-core/disk/ieee1275/ofdisk.c
> > index d887d4b..d0ee4ae 100644
> > --- a/grub-core/disk/ieee1275/ofdisk.c
> > +++ b/grub-core/disk/ieee1275/ofdisk.c
> > @@ -44,7 +44,7 @@ struct ofdisk_hash_ent
> > };
> >
> > static grub_err_t
> > -grub_ofdisk_get_block_size (const char *device, grub_uint32_t 
> > *block_size,
> > +grub_ofdisk_get_block_size (grub_uint32_t *block_size,
> >                             struct ofdisk_hash_ent *op);
> >
> > #define OFDISK_HASH_SZ  8
> > @@ -461,6 +461,7 @@ grub_ofdisk_open (const char *name, grub_disk_t 
> > disk)
> >   grub_ssize_t actual;
> >   grub_uint32_t block_size = 0;
> >   grub_err_t err;
> > +  struct ofdisk_hash_ent *op;
> >
> >   if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) != 0)
> >       return grub_error (GRUB_ERR_UNKNOWN_DEVICE,
> > @@ -471,6 +472,34 @@ grub_ofdisk_open (const char *name, grub_disk_t 
> > disk)
> >
> >   grub_dprintf ("disk", "Opening `%s'.\n", devpath);
> >
> > +  op = ofdisk_hash_find (devpath);
> > +  if (!op)
> > +    op = ofdisk_hash_add (devpath, NULL);
> > +  if (!op)
> > +    {
> > +      grub_free (devpath);
> > +      return grub_errno;
> > +    }
> > +
> > +  /* Check if the call to open is the same to the last disk already 
> > opened */
> > +  if (last_devpath && !grub_strcmp(op->open_path,last_devpath))
> > +  {
> > +      goto finish;
> > +  }
> > +
> > + /* If not, we need to close the previous disk and open the new one 
> > */
> > +  else {
> > +    if (last_ihandle){
> > +        grub_ieee1275_close (last_ihandle);
> > +    }
> > +    last_ihandle = 0;
> > +    last_devpath = NULL;
> > +
> > +    grub_ieee1275_open (op->open_path, &last_ihandle);
> > +    if (! last_ihandle)
> > +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open 
> > device");
> > +  }
> > +
> >   if (grub_ieee1275_finddevice (devpath, &dev))
> >     {
> >       grub_free (devpath);
> > @@ -491,25 +520,18 @@ grub_ofdisk_open (const char *name, grub_disk_t 
> > disk)
> >       return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "not a block 
> > device");
> >     }
> >
> > +
> > +  finish:
> >   /* XXX: There is no property to read the number of blocks.  There
> >      should be a property `#blocks', but it is not there.  Perhaps it
> >      is possible to use seek for this.  */
> >   disk->total_sectors = GRUB_DISK_SIZE_UNKNOWN;
> >
> >   {
> > -    struct ofdisk_hash_ent *op;
> > -    op = ofdisk_hash_find (devpath);
> > -    if (!op)
> > -      op = ofdisk_hash_add (devpath, NULL);
> > -    if (!op)
> > -      {
> > -        grub_free (devpath);
> > -        return grub_errno;
> > -      }
> >     disk->id = (unsigned long) op;
> >     disk->data = op->open_path;
> >
> > -    err = grub_ofdisk_get_block_size (devpath, &block_size, op);
> > +    err = grub_ofdisk_get_block_size (&block_size, op);
> >     if (err)
> >       {
> >         grub_free (devpath);
> > @@ -532,13 +554,6 @@ grub_ofdisk_open (const char *name, grub_disk_t 
> > disk)
> > static void
> > grub_ofdisk_close (grub_disk_t disk)
> > {
> > -  if (disk->data == last_devpath)
> > -    {
> > -      if (last_ihandle)
> > -       grub_ieee1275_close (last_ihandle);
> > -      last_ihandle = 0;
> > -      last_devpath = NULL;
> > -    }
> >   disk->data = 0;
> > }
> >
> > @@ -685,7 +700,7 @@ grub_ofdisk_init (void)
> > }
> >
> > static grub_err_t
> > -grub_ofdisk_get_block_size (const char *device, grub_uint32_t 
> > *block_size,
> > +grub_ofdisk_get_block_size (grub_uint32_t *block_size,
> >                             struct ofdisk_hash_ent *op)
> > {
> >   struct size_args_ieee1275
> > @@ -698,16 +713,6 @@ grub_ofdisk_get_block_size (const char *device, 
> > grub_uint32_t *block_size,
> >       grub_ieee1275_cell_t size2;
> >     } args_ieee1275;
> >
> > -  if (last_ihandle)
> > -    grub_ieee1275_close (last_ihandle);
> > -
> > -  last_ihandle = 0;
> > -  last_devpath = NULL;
> > -
> > -  grub_ieee1275_open (device, &last_ihandle);
> > -  if (! last_ihandle)
> > -    return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "can't open device");
> > -
> >   *block_size = 0;
> >
> >   if (op->block_size_fails >= 2)
> > --
> > 2.21.3
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel


Thanks for the help here Eric and good job on writing this new module.

Well, I'm still considering this patch because:

1. We are facing some problems with powerpc and it seems the code of 
ofdisk.c is somewhat bogus. IMO would be nice to have this code at least 
working well on community. It would help us almost instantly with our 
problems regarding grub on powerpc.
2. Although we are considering/discussing to switch to obdisk module, 
this will need an extra effort and time on writing specific code, 
testing, etc.

From our tests here with powerpc, this code is promising to solve many 
problems that we are currently facing.

So, since we already have obdisk for SPARC, do you think it could be a 
problem to update this code as suggested in the patch?

Thanks a lot