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

[Michael Neuling]

Diego,

This is awesome. I was digging into this a while back and found a similar
problem but never got a patch.

Testing your patch on my POWER8 phyp machine here gives at least an order of
magnitude speed up in boot time.

Regards,
Mikey


On Thu, 2020-09-24 at 17:11 +0000, diegodo 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.
> 
> 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