| Message ID | 20180927183504.18078-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 |
On Thu, Sep 27, 2018 at 08:34:59PM +0200, Goffredo Baroncelli wrote: > From: Goffredo Baroncelli <kreijack@inwind.it> > > 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 <kreikack@inwind.it> Commit message still begs for improvement. I will send you a proposal shortly. Daniel
On Tue, Oct 09, 2018 at 07:56:43PM +0200, Daniel Kiper wrote: > On Thu, Sep 27, 2018 at 08:34:59PM +0200, Goffredo Baroncelli wrote: > > From: Goffredo Baroncelli <kreijack@inwind.it> > > > > 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 <kreikack@inwind.it> > > Commit message still begs for improvement. I will send you a proposal shortly. Below you can find updated commit message. Please verify it is OK. Currently read from missing device triggers rescan. However, it is never recorded that the device is missing. So, each read of a missing device triggers rescan again and again. This behavior causes a lot of unneeded rescans leading to huge slowdowns. This patch fixes above mentioned issue. Information about missing devices is stored in the data->devices_attached[] array as NULL value in dev member. Rescan is triggered only if no information is found for a given device. This means that only first time read triggers rescan. The patch drops premature return. This way data->devices_attached[] is filled even when a given device is missing. Daniel
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, &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) { @@ -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);