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

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

[Martin Kepplinger]

[-- Attachment #1: Type: text/plain, Size: 6737 bytes --]

From: Richard Weinberger <richard@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 <richard@nod.at>
Signed-off-by: Martin Kepplinger <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;
 	}
-- 
2.19.1


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3616 bytes --]

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

[Greg KH]

On Mon, Nov 26, 2018 at 11:38:42AM +0100, Martin Kepplinger wrote:
> From: Richard Weinberger <richard@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 <richard@nod.at>
> Signed-off-by: Martin Kepplinger <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.

Now queued up for 4.14.y, thanks.

greg k-h

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

[Richard Weinberger]

Greg,

Am Donnerstag, 29. November 2018, 09:09:23 CET schrieb Greg KH:
> On Mon, Nov 26, 2018 at 11:38:42AM +0100, Martin Kepplinger wrote:
> > From: Richard Weinberger <richard@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 <richard@nod.at>
> > Signed-off-by: Martin Kepplinger <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.
> 
> 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

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

[Sudip Mukherjee]

Hi Greg,

On Sun, Dec 2, 2018 at 8:51 AM Richard Weinberger <richard@nod.at> wrote:
>
> Greg,
>
> Am Donnerstag, 29. November 2018, 09:09:23 CET schrieb Greg KH:
> > On Mon, Nov 26, 2018 at 11:38:42AM +0100, Martin Kepplinger wrote:
> > > From: Richard Weinberger <richard@nod.at>
> > >
> > > [ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]
> > >
<snip>
> >
> > 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.

-- 
Regards
Sudip

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

[Sasha Levin]

On Sun, Dec 02, 2018 at 11:50:33AM +0000, Sudip Mukherjee wrote:
>Hi Greg,
>
>On Sun, Dec 2, 2018 at 8:51 AM Richard Weinberger <richard@nod.at> wrote:
>>
>> Greg,
>>
>> Am Donnerstag, 29. November 2018, 09:09:23 CET schrieb Greg KH:
>> > On Mon, Nov 26, 2018 at 11:38:42AM +0100, Martin Kepplinger wrote:
>> > > From: Richard Weinberger <richard@nod.at>
>> > >
>> > > [ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]
>> > >
><snip>
>> >
>> > 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

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

[Richard Weinberger]

Sasha,

Am Sonntag, 2. Dezember 2018, 15:35:43 CET schrieb Sasha Levin:
> On Sun, Dec 02, 2018 at 11:50:33AM +0000, Sudip Mukherjee wrote:
> >> > 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

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

[Sudip Mukherjee]

On Sun, Dec 2, 2018 at 2:35 PM Sasha Levin <sashal@kernel.org> wrote:
>
> On Sun, Dec 02, 2018 at 11:50:33AM +0000, Sudip Mukherjee wrote:
> >Hi Greg,
> >
> >On Sun, Dec 2, 2018 at 8:51 AM Richard Weinberger <richard@nod.at> wrote:
> >>
> >> Greg,
> >>
> >> Am Donnerstag, 29. November 2018, 09:09:23 CET schrieb Greg KH:
> >> > On Mon, Nov 26, 2018 at 11:38:42AM +0100, Martin Kepplinger wrote:
> >> > > From: Richard Weinberger <richard@nod.at>
> >> > >
> >> > > [ Upstream commit 34653fd8c46e771585fce5975e4243f8fd401914 ]
> >> > >
> ><snip>
> >> >
> >> > 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. :)


-- 
Regards
Sudip

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

[Sasha Levin]

On Sun, Dec 02, 2018 at 04:02:47PM +0100, Richard Weinberger wrote:
>Sasha,
>
>Am Sonntag, 2. Dezember 2018, 15:35:43 CET schrieb Sasha Levin:
>> On Sun, Dec 02, 2018 at 11:50:33AM +0000, Sudip Mukherjee wrote:
>> >> > 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

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

[Martin Kepplinger]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

On 02.12.18 16:02, Richard Weinberger wrote:
> Sasha,
> 
> Am Sonntag, 2. Dezember 2018, 15:35:43 CET schrieb Sasha Levin:
>> On Sun, Dec 02, 2018 at 11:50:33AM +0000, Sudip Mukherjee wrote:
>>>>> 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

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3616 bytes --]

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

[Greg Kroah-Hartman]

On Tue, Dec 04, 2018 at 08:39:16AM +0100, Martin Kepplinger wrote:
> On 02.12.18 16:02, Richard Weinberger wrote:
> > Sasha,
> > 
> > Am Sonntag, 2. Dezember 2018, 15:35:43 CET schrieb Sasha Levin:
> > > On Sun, Dec 02, 2018 at 11:50:33AM +0000, Sudip Mukherjee wrote:
> > > > > > 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

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

[Greg KH]

On Sun, Dec 02, 2018 at 09:51:34AM +0100, Richard Weinberger wrote:
> Greg,
> 
> Am Donnerstag, 29. November 2018, 09:09:23 CET schrieb Greg KH:
> > On Mon, Nov 26, 2018 at 11:38:42AM +0100, Martin Kepplinger wrote:
> > > From: Richard Weinberger <richard@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 <richard@nod.at>
> > > Signed-off-by: Martin Kepplinger <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.
> > 
> > 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")

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