Re: [PATCH v4] ieee1275/ofdisk: retry on open and read failure

From: Daniel Kiper <dkiper@net-space.pl>
To: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
Cc: grub-devel@gnu.org, meghanaprakash@in.ibm.com,
	avnish@linux.vnet.ibm.com, brking@linux.vnet.ibm.com,
	mamatha4@linux.vnet.ibm.com, mchauras@linux.vnet.ibm.com,
	Diego Domingos <diegodo@linux.vnet.ibm.com>
Subject: Re: [PATCH v4] ieee1275/ofdisk: retry on open and read failure
Date: Thu, 9 Nov 2023 18:27:22 +0100	[thread overview]
Message-ID: <20231109172722.rj7gyr6tohoonpja@tomti.i.net-space.pl> (raw)
In-Reply-To: <20231108142637.327052-2-mchauras@linux.ibm.com>

On Wed, Nov 08, 2023 at 07:56:38PM +0530, Mukesh Kumar Chaurasiya wrote:
> Sometimes, when booting from a very busy SAN, the access to the
> disk can fail and then GRUB will eventually drop to GRUB prompt.
> This scenario is more frequent when deploying many machines at
> the same time using the same SAN.
> This patch aims to force the ofdisk module to retry the open or
> read function for network disks excluding after it fails. We use
> DEFAULT_RETRY_TIMEOUT, which is 15 seconds to specify the time it'll
> retry to access the disk before it definitely fails. The timeout can be
> changed by setting the environment variable ofdisk_retry_timeout.
> If the environment variable fails to read, GRUB will consider the
> default value of 15 seconds.
>
> Signed-off-by: Diego Domingos <diegodo@linux.vnet.ibm.com>
> Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
> ---
>  docs/grub.texi                   |  8 ++++
>  grub-core/disk/ieee1275/ofdisk.c | 82 +++++++++++++++++++++++++++++++-
>  2 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index a3e9ce2d1..7a16f8df5 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3336,6 +3336,7 @@ These variables have special meaning to GRUB.
>  * net_default_ip::
>  * net_default_mac::
>  * net_default_server::
> +* ofdisk_retry_timeout::
>  * pager::
>  * prefix::
>  * pxe_blksize::
> @@ -3758,6 +3759,13 @@ The default is the value of @samp{color_normal} (@pxref{color_normal}).
>  @xref{Network}.
>
>
> +@node ofdisk_retry_timeout
> +@subsection ofdisk_retry_timeout
> +
> +The time in seconds till which the grub will retry to open or read a disk in

s/grub/GRUB/...

> +case of failure to do so. This value defaults to 15 seconds.
> +
> +
>  @node pager
>  @subsection pager
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> index c6cba0c8a..fe02421dd 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -24,6 +24,9 @@
>  #include <grub/ieee1275/ofdisk.h>
>  #include <grub/i18n.h>
>  #include <grub/time.h>
> +#include <grub/env.h>
> +
> +#define RETRY_DEFAULT_TIMEOUT 15
>
>  static char *last_devpath;
>  static grub_ieee1275_ihandle_t last_ihandle;
> @@ -452,7 +455,7 @@ compute_dev_path (const char *name)
>  }
>
>  static grub_err_t
> -grub_ofdisk_open (const char *name, grub_disk_t disk)
> +grub_ofdisk_open_real (const char *name, grub_disk_t disk)
>  {
>    grub_ieee1275_phandle_t dev;
>    char *devpath;
> @@ -525,6 +528,57 @@ grub_ofdisk_open (const char *name, grub_disk_t disk)
>    return 0;
>  }
>
> +static grub_uint64_t
> +grub_ofdisk_disk_timeout (grub_disk_t disk)
> +{
> +  grub_uint64_t retry = RETRY_DEFAULT_TIMEOUT;
> +  const char *timeout = grub_env_get ("ofdisk_retry_timeout");
> +  const char *timeout_end;
> +
> +  if (grub_strstr (disk->name, "fibre-channel@") == NULL &&
> +      grub_strstr (disk->name, "vfc-client") == NULL &&
> +      grub_strstr (disk->name, "nvme-of") != NULL)
> +    {
> +      /* Do not retry in case of non-network drives */
> +      return 0;
> +    }
> +
> +  if (timeout != NULL)
> +    {
> +       retry = grub_strtoul (timeout, &timeout_end, 10);
> +       // Ignore all errors and return default timeout

The "//" comments are not allowed in the GRUB source code [1].

> +       if (grub_errno != GRUB_ERR_NONE)

Taking into account timeout_end variable addition and lack of proper
checks here it seems to me this patch has been unfinished.

> +         {
> +           grub_errno = GRUB_ERR_NONE;

Please drop grub_errno reset.

> +           return RETRY_DEFAULT_TIMEOUT;
> +         }
> +    }
> +  return retry;
> +}
> +
> +static grub_err_t
> +grub_ofdisk_open (const char *name, grub_disk_t disk)
> +{
> +  grub_err_t err;
> +  grub_uint64_t timeout = grub_get_time_ms () + (grub_ofdisk_disk_timeout (disk) * 1000);
> +  grub_uint16_t inc = 0;
> +
> +  do
> +    {
> +      err = grub_ofdisk_open_real (name, disk);
> +      if (grub_get_time_ms () >= timeout)
> +	break;

I think this should be moved behind "if (err == GRUB_ERR_UNKNOWN_DEVICE)"...

> +      if (err == GRUB_ERR_UNKNOWN_DEVICE)
> +        {
> +          grub_dprintf ("ofdisk", "Failed to open disk %s. Retrying...\n", name);
> +          grub_errno = GRUB_ERR_NONE;

I think you should not reset the grub_errno.

> +        }
> +      // Increase in wait time for subsequent requests

Same problem with the comment here...

> +      grub_millisleep (1000 * ++inc);

It seems to me you took Vladimir's comment partially into the account...

> +    } while (1);
> +  return err;
> +}
> +
>  static void
>  grub_ofdisk_close (grub_disk_t disk)
>  {
> @@ -568,7 +622,7 @@ grub_ofdisk_prepare (grub_disk_t disk, grub_disk_addr_t sector)
>  }
>
>  static grub_err_t
> -grub_ofdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
> +grub_ofdisk_read_real (grub_disk_t disk, grub_disk_addr_t sector,
>  		  grub_size_t size, char *buf)
>  {
>    grub_err_t err;
> @@ -587,6 +641,30 @@ grub_ofdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
>    return 0;
>  }
>
> +static grub_err_t
> +grub_ofdisk_read (grub_disk_t disk, grub_disk_addr_t sector,
> +		  grub_size_t size, char *buf)

The comments from grub_ofdisk_open() apply for grub_ofdisk_read() too...

> +{
> +  grub_err_t err;
> +  grub_uint64_t timeout = grub_get_time_ms () + (grub_ofdisk_disk_timeout (disk) * 1000);
> +  grub_uint16_t inc = 0;
> +
> +  do
> +    {
> +      err = grub_ofdisk_read_real (disk, sector, size, buf);
> +      if (grub_get_time_ms () >= timeout)
> +	break;
> +      if (err == GRUB_ERR_UNKNOWN_DEVICE)
> +        {
> +          grub_dprintf ("ofdisk", "Failed to read disk %s. Retrying...\n", (char*)disk->data);
> +          grub_errno = GRUB_ERR_NONE;
> +        }
> +      // Increase in wait time for subsequent requests
> +      grub_millisleep (1000 * ++inc);
> +    } while (1);
> +  return err;
> +}

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

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