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

2018-09-27 Thread Goffredo Baroncelli 
From: Goffredo Baroncelli 

Change the behavior of find_device(): before the patch, a read of a missed
device may trigger a rescan. However, it is never recorded that a device
is missed, so each single read of a missed device may triggers a rescan.
It is the caller who decides if a rescan is performed in case of a missed
device. And it does quite often, without considering if in the past a
devices was already found as "missed"
This behavior causes a lot of unneeded rescan, causing a huge slowdown in
case of a missed device.

After the patch, the "missed device" information is stored in the
data->devices_attached[] array (as a NULL value in the field dev). A rescan
is triggered only if no information at all is found. This means that only
the first time a read of a missed device triggers a rescan.

The change in the code is done removing "return NULL" when the disk is not
found. So it is always executed the code which stores in the
data->devices_attached[] array the value returned by grub_device_iterate():
NULL if the device is missed, or a valid data otherwise.

Signed-off-by: Goffredo Baroncelli 
---
 grub-core/fs/btrfs.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 2b1dd7dd4..c07d91657 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,10 +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, );
-  if (!ctx.dev_found)
-return NULL;
+
+  grub_device_iterate (find_device_iter, );
+
   data->n_devices_attached++;
   if (data->n_devices_attached > data->n_devices_allocated)
 {
@@ -615,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;
}
@@ -898,7 +898,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",
@@ -975,7 +975,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);
-- 
2.19.0

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

2018-09-27 Thread Daniel Kiper 
On Wed, Sep 26, 2018 at 09:55:54PM +0200, Goffredo Baroncelli wrote:
> On 25/09/2018 19.29, Daniel Kiper wrote:
> > On Wed, Sep 19, 2018 at 08:40:35PM +0200, Goffredo Baroncelli wrote:
> >> From: Goffredo Baroncelli 
> >>
> >> If a device is not found, do not return immediately but
> >> record this failure by storing NULL in data->devices_attached[].
> >
> > Still the same question: Where the store happens in the code?
> > I cannot find it in the patch below. This have to be clarified.
> >
> > Daniel
>
>
> What about the following commit description
> -
> Change the behavior of find_device(): before the patch, a read of a
> missed device might trigger a rescan. However, it is never recorded

s/might/may/

> that a device is missed, so each single read of a missed device might
> triggers a rescan.  It is the caller who decides if a rescan is
> performed in case of a missed device. And it does quite often, without
> considering if in the past a devices was already found as "missed"
> This behavior causes a lot of unneeded rescan, causing a huge slowdown
> in case of a missed device.
>
> After the patch, the "missed device" information is stored in the
> cache (as a NULL value). A rescan is triggered only if no information

What do you mean by "cache"? ctx.dev_found? If yes please use latter
instead of former. Or both together if it makes sense.

> at all is found in the cache. This means that only the first time a
> read of a missed device triggers a rescan.
>
> The change in the code is done removing "return NULL" when the disk is
> not found. So it is always executed the code which stores in the cache

cache?

> the value returned by grub_device_iterate(): NULL if the device is
> missed, or a valid data otherwise.
> -

Otherwise it is much better than earlier one.

Daniel