Re: [PATCH 4/9] btrfs: Avoid a rescan for a device which was already not found.

[Daniel Kiper]

On Tue, Jun 19, 2018 at 08:22:19PM +0200, Goffredo Baroncelli wrote:
> Forward because this patch still doesn't reach the mailing list

Could you fix that somehow? It is confusing.

> --------------------------------------
>
> If a device is not found, record this failure by storing NULL in
> data->devices_attached[]. This way we avoid unnecessary devices rescan,
> and speedup the reads in case of a degraded array.
>
> Signed-off-by: Goffredo Baroncelli <address@hidden>
> ---
>  grub-core/fs/btrfs.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
> index 8d07e2d72..70bcb0fdc 100644
> --- a/grub-core/fs/btrfs.c
> +++ b/grub-core/fs/btrfs.c
> @@ -588,7 +588,7 @@ find_device_iter (const char *name, void *data)
>  }
>
>  static grub_device_t
> -find_device (struct grub_btrfs_data *data, grub_uint64_t id, int do_rescan)
> +find_device (struct grub_btrfs_data *data, grub_uint64_t id)
>  {
>    struct find_device_ctx ctx = {
>      .data = data,
> @@ -600,12 +600,9 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t 
> id, int do_rescan)
>    for (i = 0; i < data->n_devices_attached; i++)
>      if (id == data->devices_attached[i].id)
>        return data->devices_attached[i].dev;
> -  if (do_rescan)
> -    grub_device_iterate (find_device_iter, &ctx);
> -  if (!ctx.dev_found)
> -    {
> -      return NULL;
> -    }
> +
> +  grub_device_iterate (find_device_iter, &ctx);
> +
>    data->n_devices_attached++;
>    if (data->n_devices_attached > data->n_devices_allocated)
>      {
> @@ -617,7 +614,8 @@ find_device (struct grub_btrfs_data *data, grub_uint64_t 
> id, int do_rescan)
>                       * sizeof (data->devices_attached[0]));
>        if (!data->devices_attached)
>       {
> -       grub_device_close (ctx.dev_found);
> +       if (ctx.dev_found)
> +         grub_device_close (ctx.dev_found);
>         data->devices_attached = tmp;
>         return NULL;
>       }
> @@ -896,7 +894,7 @@ grub_btrfs_read_logical (struct grub_btrfs_data *data, 
> grub_disk_addr_t addr,
>                             " for laddr 0x%" PRIxGRUB_UINT64_T "\n", paddr,
>                             addr);
>
> -             dev = find_device (data, stripe->device_id, j);
> +             dev = find_device (data, stripe->device_id);
>               if (!dev)
>                 {
>                   grub_dprintf ("btrfs",
> @@ -973,7 +971,8 @@ grub_btrfs_unmount (struct grub_btrfs_data *data)
>    unsigned i;
>    /* The device 0 is closed one layer upper.  */
>    for (i = 1; i < data->n_devices_attached; i++)
> -    grub_device_close (data->devices_attached[i].dev);
> +    if (data->devices_attached[i].dev)
> +        grub_device_close (data->devices_attached[i].dev);
>    grub_free (data->devices_attached);
>    grub_free (data->extent);
>    grub_free (data);

The commit message or code is wrong. NULL is never stored into
data->devices_attached[]. Am I missing something?

Daniel