[PATCH] ubi: fastmap: Check each mapping only once

[Martin Kepplinger]

From: Richard Weinberger <***@nod.at>

[ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]

This commit got merged along with commit 781932375ffc
("ubi: fastmap: Correctly handle interrupted erasures in EBA") upstream but
only the latter has been applied to stable v4.14.54 as commit a23cf10d9abb.
This resulted in a performance regression. Startup on i.MX platforms is
delayed for up to a few seconds depending on the platform.
This fixes ubi fastmap to be of the same performance as it has been before
said fastmap changes.

Fixes: a23cf10d9abb ("ubi: fastmap: Correctly handle interrupted erasures in EBA")
Signed-off-by: Richard Weinberger <***@nod.at>
Signed-off-by: Martin Kepplinger <***@ginzinger.com>
---

Richard, although this fixes a major slowdown regression in -stable, do you
consider this "stable" too?

This applies and is tested only for the 4.14 stable tree. It seems to be
equally relevant for 4.9 and 4.4 though.

thanks
martin


drivers/mtd/ubi/build.c | 1 +
drivers/mtd/ubi/eba.c | 4 ++++
drivers/mtd/ubi/fastmap.c | 20 ++++++++++++++++++++
drivers/mtd/ubi/ubi.h | 11 +++++++++++
drivers/mtd/ubi/vmt.c | 1 +
drivers/mtd/ubi/vtbl.c | 16 +++++++++++++++-
6 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 18a72da759a0..6445c693d935 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -526,6 +526,7 @@ void ubi_free_internal_volumes(struct ubi_device *ubi)
for (i = ubi->vtbl_slots;
i < ubi->vtbl_slots + UBI_INT_VOL_COUNT; i++) {
ubi_eba_replace_table(ubi->volumes[i], NULL);
+ ubi_fastmap_destroy_checkmap(ubi->volumes[i]);
kfree(ubi->volumes[i]);
}
}
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index d0884bd9d955..c4d4b8f07630 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -517,6 +517,9 @@ static int check_mapping(struct ubi_device *ubi, struct ubi_volume *vol, int lnu
if (!ubi->fast_attach)
return 0;

+ if (!vol->checkmap || test_bit(lnum, vol->checkmap))
+ return 0;
+
vidb = ubi_alloc_vid_buf(ubi, GFP_NOFS);
if (!vidb)
return -ENOMEM;
@@ -551,6 +554,7 @@ static int check_mapping(struct ubi_device *ubi, struct ubi_volume *vol, int lnu
goto out_free;
}

+ set_bit(lnum, vol->checkmap);
err = 0;

out_free:
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 5a832bc79b1b..63e8527f7b65 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -1101,6 +1101,26 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
goto out;
}

+int ubi_fastmap_init_checkmap(struct ubi_volume *vol, int leb_count)
+{
+ struct ubi_device *ubi = vol->ubi;
+
+ if (!ubi->fast_attach)
+ return 0;
+
+ vol->checkmap = kcalloc(BITS_TO_LONGS(leb_count), sizeof(unsigned long),
+ GFP_KERNEL);
+ if (!vol->checkmap)
+ return -ENOMEM;
+
+ return 0;
+}
+
+void ubi_fastmap_destroy_checkmap(struct ubi_volume *vol)
+{
+ kfree(vol->checkmap);
+}
+
/**
* ubi_write_fastmap - writes a fastmap.
* @ubi: UBI device object
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 5fe62653995e..f5ba97c46160 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -334,6 +334,9 @@ struct ubi_eba_leb_desc {
* @changing_leb: %1 if the atomic LEB change ioctl command is in progress
* @direct_writes: %1 if direct writes are enabled for this volume
*
+ * @checkmap: bitmap to remember which PEB->LEB mappings got checked,
+ * protected by UBI LEB lock tree.
+ *
* The @corrupted field indicates that the volume's contents is corrupted.
* Since UBI protects only static volumes, this field is not relevant to
* dynamic volumes - it is user's responsibility to assure their data
@@ -377,6 +380,10 @@ struct ubi_volume {
unsigned int updating:1;
unsigned int changing_leb:1;
unsigned int direct_writes:1;
+
+#ifdef CONFIG_MTD_UBI_FASTMAP
+ unsigned long *checkmap;
+#endif
};

/**
@@ -965,8 +972,12 @@ size_t ubi_calc_fm_size(struct ubi_device *ubi);
int ubi_update_fastmap(struct ubi_device *ubi);
int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
struct ubi_attach_info *scan_ai);
+int ubi_fastmap_init_checkmap(struct ubi_volume *vol, int leb_count);
+void ubi_fastmap_destroy_checkmap(struct ubi_volume *vol);
#else
static inline int ubi_update_fastmap(struct ubi_device *ubi) { return 0; }
+int static inline ubi_fastmap_init_checkmap(struct ubi_volume *vol, int leb_count) { return 0; }
+static inline void ubi_fastmap_destroy_checkmap(struct ubi_volume *vol) {}
#endif

/* block.c */
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 3fd8d7ff7a02..0be516780e92 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -139,6 +139,7 @@ static void vol_release(struct device *dev)
struct ubi_volume *vol = container_of(dev, struct ubi_volume, dev);

ubi_eba_replace_table(vol, NULL);
+ ubi_fastmap_destroy_checkmap(vol);
kfree(vol);
}

diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
index 263743e7b741..94d7a865b135 100644
--- a/drivers/mtd/ubi/vtbl.c
+++ b/drivers/mtd/ubi/vtbl.c
@@ -534,7 +534,7 @@ static int init_volumes(struct ubi_device *ubi,
const struct ubi_attach_info *ai,
const struct ubi_vtbl_record *vtbl)
{
- int i, reserved_pebs = 0;
+ int i, err, reserved_pebs = 0;
struct ubi_ainf_volume *av;
struct ubi_volume *vol;

@@ -620,6 +620,16 @@ static int init_volumes(struct ubi_device *ubi,
(long long)(vol->used_ebs - 1) * vol->usable_leb_size;
vol->used_bytes += av->last_data_size;
vol->last_eb_bytes = av->last_data_size;
+
+ /*
+ * We use ubi->peb_count and not vol->reserved_pebs because
+ * we want to keep the code simple. Otherwise we'd have to
+ * resize/check the bitmap upon volume resize too.
+ * Allocating a few bytes more does not hurt.
+ */
+ err = ubi_fastmap_init_checkmap(vol, ubi->peb_count);
+ if (err)
+ return err;
}

/* And add the layout volume */
@@ -645,6 +655,9 @@ static int init_volumes(struct ubi_device *ubi,
reserved_pebs += vol->reserved_pebs;
ubi->vol_count += 1;
vol->ubi = ubi;
+ err = ubi_fastmap_init_checkmap(vol, UBI_LAYOUT_VOLUME_EBS);
+ if (err)
+ return err;

if (reserved_pebs > ubi->avail_pebs) {
ubi_err(ubi, "not enough PEBs, required %d, available %d",
@@ -849,6 +862,7 @@ int ubi_read_volume_table(struct ubi_device *ubi, struct ubi_attach_info *ai)
out_free:
vfree(ubi->vtbl);
for (i = 0; i < ubi->vtbl_slots + UBI_INT_VOL_COUNT; i++) {
+ ubi_fastmap_destroy_checkmap(ubi->volumes[i]);
kfree(ubi->volumes[i]);
ubi->volumes[i] = NULL;
}

[Greg KH]

Post by Martin Kepplinger
[ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]
This commit got merged along with commit 781932375ffc
("ubi: fastmap: Correctly handle interrupted erasures in EBA") upstream but
only the latter has been applied to stable v4.14.54 as commit a23cf10d9abb.
This resulted in a performance regression. Startup on i.MX platforms is
delayed for up to a few seconds depending on the platform.
This fixes ubi fastmap to be of the same performance as it has been before
said fastmap changes.
Fixes: a23cf10d9abb ("ubi: fastmap: Correctly handle interrupted erasures in EBA")
---
Richard, although this fixes a major slowdown regression in -stable, do you
consider this "stable" too?
This applies and is tested only for the 4.14 stable tree. It seems to be
equally relevant for 4.9 and 4.4 though.

Now queued up for 4.14.y, thanks.

greg k-h

[Richard Weinberger]

Greg,

Post by Greg KH

Post by Martin Kepplinger
[ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]
This commit got merged along with commit 781932375ffc
("ubi: fastmap: Correctly handle interrupted erasures in EBA") upstream but
only the latter has been applied to stable v4.14.54 as commit a23cf10d9abb.
This resulted in a performance regression. Startup on i.MX platforms is
delayed for up to a few seconds depending on the platform.
This fixes ubi fastmap to be of the same performance as it has been before
said fastmap changes.
Fixes: a23cf10d9abb ("ubi: fastmap: Correctly handle interrupted erasures in EBA")
---
Richard, although this fixes a major slowdown regression in -stable, do you
consider this "stable" too?
This applies and is tested only for the 4.14 stable tree. It seems to be
equally relevant for 4.9 and 4.4 though.

Now queued up for 4.14.y, thanks.

can you *please* slow a little down?

There are times (e.g. when I travel, visit customers on-site, being sick, etc...)
where I don't have the resources to monitor the mailinglists
in detail. Adding patches to stable on shout asks for trouble.

As Sudip points out, this patch needs a further fix patch:
25677478474a ("ubi: Initialize Fastmap checkmapping correctly")

Thanks,
//richard

[Sudip Mukherjee]

Hi Greg,

Post by Richard Weinberger
Greg,

Post by Greg KH

Post by Martin Kepplinger
[ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]

<snip>

Post by Richard Weinberger

Post by Greg KH
Now queued up for 4.14.y, thanks.

can you *please* slow a little down?

True. It will really help if you can have some sort of fixed schedule
for stable release, like maybe stablerc is ready on Thursday or Friday
and release the stable on Monday. Having a weekend in stablerc will be
helpful for people like me who only get the time in weekends for
upstream or stable kernel.

[Sasha Levin]

Post by Sudip Mukherjee
Hi Greg,

Post by Richard Weinberger
Greg,

Post by Greg KH

Post by Martin Kepplinger
[ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]

<snip>

Post by Richard Weinberger

Post by Greg KH
Now queued up for 4.14.y, thanks.

can you *please* slow a little down?

True. It will really help if you can have some sort of fixed schedule
for stable release, like maybe stablerc is ready on Thursday or Friday
and release the stable on Monday. Having a weekend in stablerc will be
helpful for people like me who only get the time in weekends for
upstream or stable kernel.

Any sort of schedule will never work for everyone (for example, if it's
part of your paid job - you don't necessarily want to review stuff over
the weekend).

--
Thanks,
Sasha

[Richard Weinberger]

Sasha,

Post by Sasha Levin

Post by Sudip Mukherjee

Post by Richard Weinberger

Post by Greg KH
Now queued up for 4.14.y, thanks.

can you *please* slow a little down?

True. It will really help if you can have some sort of fixed schedule
for stable release, like maybe stablerc is ready on Thursday or Friday
and release the stable on Monday. Having a weekend in stablerc will be
helpful for people like me who only get the time in weekends for
upstream or stable kernel.

Any sort of schedule will never work for everyone (for example, if it's
part of your paid job - you don't necessarily want to review stuff over
the weekend).

a schedule is not needed, but please give maintainers at least a chance
to react on stable inclusion request.
In this case Martin asked for inclusion on Monday and the patch was applied
two days later.

As you noted not everyone works full time on the kernel and gets paid for that.
So it might take a few days to react and review such a request.

Thanks,
//richard

[Sasha Levin]

Post by Richard Weinberger
Sasha,

Post by Sasha Levin

Post by Sudip Mukherjee

Post by Richard Weinberger

Post by Greg KH
Now queued up for 4.14.y, thanks.

can you *please* slow a little down?

True. It will really help if you can have some sort of fixed schedule
for stable release, like maybe stablerc is ready on Thursday or Friday
and release the stable on Monday. Having a weekend in stablerc will be
helpful for people like me who only get the time in weekends for
upstream or stable kernel.

Any sort of schedule will never work for everyone (for example, if it's
part of your paid job - you don't necessarily want to review stuff over
the weekend).

a schedule is not needed, but please give maintainers at least a chance
to react on stable inclusion request.
In this case Martin asked for inclusion on Monday and the patch was applied
two days later.
As you noted not everyone works full time on the kernel and gets paid for that.
So it might take a few days to react and review such a request.

Yes, that's fair, I wasn't arguing against that.

--
Thanks,
Sasha

[Martin Kepplinger]

Post by Richard Weinberger
Sasha,

Post by Sasha Levin

Post by Sudip Mukherjee

Post by Richard Weinberger

Post by Greg KH
Now queued up for 4.14.y, thanks.

can you *please* slow a little down?

True. It will really help if you can have some sort of fixed schedule
for stable release, like maybe stablerc is ready on Thursday or Friday
and release the stable on Monday. Having a weekend in stablerc will be
helpful for people like me who only get the time in weekends for
upstream or stable kernel.

Any sort of schedule will never work for everyone (for example, if it's
part of your paid job - you don't necessarily want to review stuff over
the weekend).

a schedule is not needed, but please give maintainers at least a chance
to react on stable inclusion request.
In this case Martin asked for inclusion on Monday and the patch was applied
two days later.

True, especially when the maintainer is asked a question as part of the
patch.

I've already had the feeling that we'd need the other patch too, but in
this case at least I should have searched for Fixes tags.

Greg, how about reminding people of Fixes tags in
Documentation/process/stable-kernel-rules.rst ?

martin

[Greg Kroah-Hartman]

Post by Martin Kepplinger

Post by Richard Weinberger
Sasha,

Post by Sasha Levin

Post by Sudip Mukherjee

Post by Richard Weinberger

Post by Greg KH
Now queued up for 4.14.y, thanks.

can you *please* slow a little down?

True. It will really help if you can have some sort of fixed schedule
for stable release, like maybe stablerc is ready on Thursday or Friday
and release the stable on Monday. Having a weekend in stablerc will be
helpful for people like me who only get the time in weekends for
upstream or stable kernel.

Any sort of schedule will never work for everyone (for example, if it's
part of your paid job - you don't necessarily want to review stuff over
the weekend).

a schedule is not needed, but please give maintainers at least a chance
to react on stable inclusion request.
In this case Martin asked for inclusion on Monday and the patch was applied
two days later.

True, especially when the maintainer is asked a question as part of the
patch.
I've already had the feeling that we'd need the other patch too, but in this
case at least I should have searched for Fixes tags.
Greg, how about reminding people of Fixes tags in
Documentation/process/stable-kernel-rules.rst ?

Reminding people how? Patches to that file are always gladly accepted :)

thanks,

greg k-h

[Sudip Mukherjee]

Post by Sasha Levin

Post by Sudip Mukherjee
Hi Greg,

Post by Richard Weinberger
Greg,

Post by Greg KH

Post by Martin Kepplinger
[ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]

<snip>

Post by Richard Weinberger

Post by Greg KH
Now queued up for 4.14.y, thanks.

can you *please* slow a little down?

True. It will really help if you can have some sort of fixed schedule
for stable release, like maybe stablerc is ready on Thursday or Friday
and release the stable on Monday. Having a weekend in stablerc will be
helpful for people like me who only get the time in weekends for
upstream or stable kernel.

Any sort of schedule will never work for everyone (for example, if it's
part of your paid job - you don't necessarily want to review stuff over
the weekend).

Yes, exactly. But like I said if the stablerc tree is pushed on
Thursday, and the stable release is released on Monday then both types
of people will get time, and hopefully everyone is happy. :)

[Greg KH]

Post by Richard Weinberger
Greg,

Post by Greg KH

Post by Martin Kepplinger
[ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]
This commit got merged along with commit 781932375ffc
("ubi: fastmap: Correctly handle interrupted erasures in EBA") upstream but
only the latter has been applied to stable v4.14.54 as commit a23cf10d9abb.
This resulted in a performance regression. Startup on i.MX platforms is
delayed for up to a few seconds depending on the platform.
This fixes ubi fastmap to be of the same performance as it has been before
said fastmap changes.
Fixes: a23cf10d9abb ("ubi: fastmap: Correctly handle interrupted erasures in EBA")
---
Richard, although this fixes a major slowdown regression in -stable, do you
consider this "stable" too?
This applies and is tested only for the 4.14 stable tree. It seems to be
equally relevant for 4.9 and 4.4 though.

Now queued up for 4.14.y, thanks.

can you *please* slow a little down?
There are times (e.g. when I travel, visit customers on-site, being sick, etc...)
where I don't have the resources to monitor the mailinglists
in detail. Adding patches to stable on shout asks for trouble.
25677478474a ("ubi: Initialize Fastmap checkmapping correctly")

This is now in 4.14.86 so all should be fine now.

As for "speed", most of the time people are complaining that I move too
slow in getting fixes backported and to their patches. Rarely am I told
I am moving too fast, that's a nice change :)

As for doing releases on a "regular" schedule, I've tried it, and it
didn't work any better/worse than what I'm doing now as everyone who
consumes these kernels have their own cadence / acceptance process and I
can never get in sync with _everyone_ let alone almost _anyone_.

And due to travel and other things (like security issues coming up),
trying to nail down a specific day-of-the-week doesn't work out at all
either.

thanks,

greg k-h