diff mbox series

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

Message ID 20180919184040.22540-5-kreijack@libero.it (mailing list archive)
State New, archived
Headers show
Series [1/9] btrfs: Add support for reading a filesystem with a RAID 5 or RAID 6 profile. | expand

Commit Message

Goffredo Baroncelli Sept. 19, 2018, 6:40 p.m. UTC 
From: Goffredo Baroncelli <kreijack@inwind.it>

If a device is not found, do not return immediately but
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 <kreikack@inwind.it>
---
 grub-core/fs/btrfs.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Daniel Kiper Sept. 25, 2018, 5:29 p.m. UTC | #1 
On Wed, Sep 19, 2018 at 08:40:35PM +0200, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <kreijack@inwind.it>
>
> 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

> This way we avoid unnecessary devices rescan, and speedup the
> reads in case of a degraded array.
>
> Signed-off-by: Goffredo Baroncelli <kreikack@inwind.it>
> ---
>  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 0cdfaf7c0..6e42c33f6 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;
>  	}
> @@ -892,7 +890,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",
> @@ -969,7 +967,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
>
Goffredo Baroncelli Sept. 26, 2018, 7:55 p.m. UTC | #2 
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 <kreijack@inwind.it>
>>
>> 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 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 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 the value returned by grub_device_iterate(): NULL if the device is missed, or a valid data otherwise.
---------------------------------------------
> 
>> This way we avoid unnecessary devices rescan, and speedup the
>> reads in case of a degraded array.
>>
>> Signed-off-by: Goffredo Baroncelli <kreikack@inwind.it>
>> ---
>>  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 0cdfaf7c0..6e42c33f6 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;
>>  	}
>> @@ -892,7 +890,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",
>> @@ -969,7 +967,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
>>
>
Daniel Kiper Sept. 27, 2018, 4:03 p.m. UTC | #3 
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 <kreijack@inwind.it>
> >>
> >> 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
diff mbox series

Patch

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index 0cdfaf7c0..6e42c33f6 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;
 	}
@@ -892,7 +890,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",
@@ -969,7 +967,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);