| Message ID | 6551534f88b3aa77cc377c4270c4ddf7fe92cdf4.1524214122.git-series.quentin.schulz@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | [1/2] ubi: move constants for ubi vol parsing from kernel param to ubi.h | expand |
Quentin, Am Freitag, 20. April 2018, 10:52:41 CEST schrieb Quentin Schulz: > There's already ECC on NAND pages so there may be no need for one to > check the CRC of a UBI volume. > > Let's introduce a ubi.nocheck parameter that let one skip the CRC check > when attaching a UBI volume. > > This also drastically speeds kernel boot by removing a potentially > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board > for a ~20MB UBI volume used as rootfs. You mean at *open* time of a *static* UBI volume? So I guess the use case is having a read-only filesystem on top of ubiblock? > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > --- > drivers/mtd/ubi/kapi.c | 70 +++++++++++++++++++++++++++++++++++++++++++- > drivers/mtd/ubi/ubi.h | 6 ++++- > drivers/mtd/ubi/vtbl.c | 24 ++++++++++++++- > 3 files changed, 99 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c > index d4b2e87..d604cd5 100644 > --- a/drivers/mtd/ubi/kapi.c > +++ b/drivers/mtd/ubi/kapi.c > @@ -28,6 +28,9 @@ > #include <asm/div64.h> > #include "ubi.h" > > +struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES]; > +int ubinocheck_devs; > + > /** > * ubi_do_get_device_info - get information about UBI device. > * @ubi: UBI device description object > @@ -865,3 +868,70 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb) > return blocking_notifier_chain_unregister(&ubi_notifiers, nb); > } > EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier); > + > +static int __init ubinocheckvol_set_param(const char *val, > + const struct kernel_param *kp) > +{ > + int i, ret; > + size_t len; > + struct ubivol_param *param; > + char buf[UBIVOL_PARAM_LEN]; > + char *pbuf = &buf[0]; > + char *tokens[UBIVOL_PARAM_COUNT]; > + > + if (!val) > + return -EINVAL; > + > + len = strnlen(val, UBIVOL_PARAM_LEN); > + if (len == 0) { > + pr_warn("UBI: nocheck: empty 'nocheck=' parameter - ignored\n"); > + return 0; > + } > + > + if (len == UBIVOL_PARAM_LEN) { > + pr_err("UBI: nocheck: parameter \"%s\" is too long, max. is %d\n", > + val, UBIVOL_PARAM_LEN); > + return -EINVAL; > + } > + > + strcpy(buf, val); > + > + /* Get rid of the final newline */ > + if (buf[len - 1] == '\n') > + buf[len - 1] = '\0'; > + > + for (i = 0; i < UBIVOL_PARAM_COUNT; i++) > + tokens[i] = strsep(&pbuf, ","); > + > + param = &ubinocheck_param[ubinocheck_devs]; > + if (tokens[1]) { > + /* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */ > + ret = kstrtoint(tokens[0], 10, ¶m->ubi_num); > + if (ret < 0) > + return -EINVAL; > + > + /* Second param can be a number or a name */ > + ret = kstrtoint(tokens[1], 10, ¶m->vol_id); > + if (ret < 0) { > + param->vol_id = -1; > + strcpy(param->name, tokens[1]); > + } > + } > Do we really need this per volume? If your flash is trustworthy, it should not matter. Having it per UBI instance instead of volume would make the code less complicated. > + ubinocheck_devs++; > + > + return 0; > +} > + > +static const struct kernel_param_ops ubinocheckvol_param_ops = { > + .set = ubinocheckvol_set_param, > +}; > +module_param_cb(nocheck, &ubinocheckvol_param_ops, NULL, 0); > +MODULE_PARM_DESC(nocheck, > + "Disable CRC check for UBI volumes. Parameter format: nocheck=dev,[num|name]>.\n" > + "Multiple \"nocheck\" parameters may be specified.\n" > + "Examples\n" > + "Using the UBI device, and the volume name:\n" > + "ubi.nocheck=0,rootfs\n" > + "Using both UBI device number and UBI volume number:\n" > + "ubi.nocheck=0,0\n"); > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h > index 4cc6ec9..2bd89b4 100644 > --- a/drivers/mtd/ubi/ubi.h > +++ b/drivers/mtd/ubi/ubi.h > @@ -825,6 +825,12 @@ struct ubivol_param { > char name[UBIVOL_PARAM_LEN + 1]; > }; > > +/* Numbers of elements set in the @ubinocheck_param array */ > +extern int ubinocheck_devs; > + > +/* MTD devices specification parameters */ > +extern struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES]; > + > #include "debug.h" > > extern struct kmem_cache *ubi_wl_entry_slab; > diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c > index 263743e..06b6cfd 100644 > --- a/drivers/mtd/ubi/vtbl.c > +++ b/drivers/mtd/ubi/vtbl.c > @@ -534,9 +534,10 @@ 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, j, reserved_pebs = 0; > struct ubi_ainf_volume *av; > struct ubi_volume *vol; > + struct ubivol_param *param; > > for (i = 0; i < ubi->vtbl_slots; i++) { > cond_resched(); > @@ -620,6 +621,27 @@ 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; > + > + for (j = 0; j < ubinocheck_devs; j++) { > + param = &ubinocheck_param[j]; > + > + if (vol->ubi->ubi_num != param->ubi_num) > + continue; > + if (vol->vol_id == param->vol_id) { > + ubi_msg(vol->ubi, > + "skipping CRC check for volume %d", > + vol->vol_id); > + vol->checked = true; Please don't abuse the checked flag. A new one a la "skip_check" does not hurt. But again, I don't think we need it per volume. Also don't forget to add the nocheck parameter to the ioctl() interface, such that you can specify it also with ubiattach... Thanks, //richard
Hi Richard, On Fri, Apr 20, 2018 at 11:37:07AM +0200, Richard Weinberger wrote: > Quentin, > > Am Freitag, 20. April 2018, 10:52:41 CEST schrieb Quentin Schulz: > > There's already ECC on NAND pages so there may be no need for one to > > check the CRC of a UBI volume. > > > > Let's introduce a ubi.nocheck parameter that let one skip the CRC check > > when attaching a UBI volume. > > > > This also drastically speeds kernel boot by removing a potentially > > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board > > for a ~20MB UBI volume used as rootfs. > > You mean at *open* time of a *static* UBI volume? > So I guess the use case is having a read-only filesystem on top of ubiblock? > Turns out I forgot some important information and poorly worded my commit log :) Yes sorry, I have a squashfs (read-only by design) as the image of one static UBI volume. And yes, I meant when opening a static UBI volume. Quentin > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > --- > > drivers/mtd/ubi/kapi.c | 70 +++++++++++++++++++++++++++++++++++++++++++- > > drivers/mtd/ubi/ubi.h | 6 ++++- > > drivers/mtd/ubi/vtbl.c | 24 ++++++++++++++- > > 3 files changed, 99 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c > > index d4b2e87..d604cd5 100644 > > --- a/drivers/mtd/ubi/kapi.c > > +++ b/drivers/mtd/ubi/kapi.c > > @@ -28,6 +28,9 @@ > > #include <asm/div64.h> > > #include "ubi.h" > > > > +struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES]; > > +int ubinocheck_devs; > > + > > /** > > * ubi_do_get_device_info - get information about UBI device. > > * @ubi: UBI device description object > > @@ -865,3 +868,70 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb) > > return blocking_notifier_chain_unregister(&ubi_notifiers, nb); > > } > > EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier); > > + > > +static int __init ubinocheckvol_set_param(const char *val, > > + const struct kernel_param *kp) > > +{ > > + int i, ret; > > + size_t len; > > + struct ubivol_param *param; > > + char buf[UBIVOL_PARAM_LEN]; > > + char *pbuf = &buf[0]; > > + char *tokens[UBIVOL_PARAM_COUNT]; > > + > > + if (!val) > > + return -EINVAL; > > + > > + len = strnlen(val, UBIVOL_PARAM_LEN); > > + if (len == 0) { > > + pr_warn("UBI: nocheck: empty 'nocheck=' parameter - ignored\n"); > > + return 0; > > + } > > + > > + if (len == UBIVOL_PARAM_LEN) { > > + pr_err("UBI: nocheck: parameter \"%s\" is too long, max. is %d\n", > > + val, UBIVOL_PARAM_LEN); > > + return -EINVAL; > > + } > > + > > + strcpy(buf, val); > > + > > + /* Get rid of the final newline */ > > + if (buf[len - 1] == '\n') > > + buf[len - 1] = '\0'; > > + > > + for (i = 0; i < UBIVOL_PARAM_COUNT; i++) > > + tokens[i] = strsep(&pbuf, ","); > > + > > + param = &ubinocheck_param[ubinocheck_devs]; > > + if (tokens[1]) { > > + /* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */ > > + ret = kstrtoint(tokens[0], 10, ¶m->ubi_num); > > + if (ret < 0) > > + return -EINVAL; > > + > > + /* Second param can be a number or a name */ > > + ret = kstrtoint(tokens[1], 10, ¶m->vol_id); > > + if (ret < 0) { > > + param->vol_id = -1; > > + strcpy(param->name, tokens[1]); > > + } > > + } > > > > Do we really need this per volume? If your flash is trustworthy, it should not matter. > Having it per UBI instance instead of volume would make the code less complicated. > > > + ubinocheck_devs++; > > + > > + return 0; > > +} > > + > > +static const struct kernel_param_ops ubinocheckvol_param_ops = { > > + .set = ubinocheckvol_set_param, > > +}; > > +module_param_cb(nocheck, &ubinocheckvol_param_ops, NULL, 0); > > +MODULE_PARM_DESC(nocheck, > > + "Disable CRC check for UBI volumes. Parameter format: nocheck=dev,[num|name]>.\n" > > + "Multiple \"nocheck\" parameters may be specified.\n" > > + "Examples\n" > > + "Using the UBI device, and the volume name:\n" > > + "ubi.nocheck=0,rootfs\n" > > + "Using both UBI device number and UBI volume number:\n" > > + "ubi.nocheck=0,0\n"); > > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h > > index 4cc6ec9..2bd89b4 100644 > > --- a/drivers/mtd/ubi/ubi.h > > +++ b/drivers/mtd/ubi/ubi.h > > @@ -825,6 +825,12 @@ struct ubivol_param { > > char name[UBIVOL_PARAM_LEN + 1]; > > }; > > > > +/* Numbers of elements set in the @ubinocheck_param array */ > > +extern int ubinocheck_devs; > > + > > +/* MTD devices specification parameters */ > > +extern struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES]; > > + > > #include "debug.h" > > > > extern struct kmem_cache *ubi_wl_entry_slab; > > diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c > > index 263743e..06b6cfd 100644 > > --- a/drivers/mtd/ubi/vtbl.c > > +++ b/drivers/mtd/ubi/vtbl.c > > @@ -534,9 +534,10 @@ 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, j, reserved_pebs = 0; > > struct ubi_ainf_volume *av; > > struct ubi_volume *vol; > > + struct ubivol_param *param; > > > > for (i = 0; i < ubi->vtbl_slots; i++) { > > cond_resched(); > > @@ -620,6 +621,27 @@ 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; > > + > > + for (j = 0; j < ubinocheck_devs; j++) { > > + param = &ubinocheck_param[j]; > > + > > + if (vol->ubi->ubi_num != param->ubi_num) > > + continue; > > + if (vol->vol_id == param->vol_id) { > > + ubi_msg(vol->ubi, > > + "skipping CRC check for volume %d", > > + vol->vol_id); > > + vol->checked = true; > > Please don't abuse the checked flag. > A new one a la "skip_check" does not hurt. > But again, I don't think we need it per volume. > > Also don't forget to add the nocheck parameter to the ioctl() interface, such that > you can specify it also with ubiattach... > > Thanks, > //richard
On Fri, 20 Apr 2018 10:52:41 +0200 Quentin Schulz <quentin.schulz@bootlin.com> wrote: > There's already ECC on NAND pages so there may be no need for one to > check the CRC of a UBI volume. That's true that ECC can help detecting corruptions, but I don't think this is the actual reason for disabling CRC check at volume open time. The actual reason for doing that is when the UBI volume user (in our case the squashfs FS) is also checking data consistency on its own (which is the case for squashfs). > > Let's introduce a ubi.nocheck parameter that let one skip the CRC check > when attaching a UBI volume. > > This also drastically speeds kernel boot by removing a potentially > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board > for a ~20MB UBI volume used as rootfs. Can you give the old and new open/mount time instead of telling how much you gained on the whole boot-time?
Hi Boris, On Fri, Apr 20, 2018 at 07:19:24PM +0200, Boris Brezillon wrote: > On Fri, 20 Apr 2018 10:52:41 +0200 > Quentin Schulz <quentin.schulz@bootlin.com> wrote: > > > There's already ECC on NAND pages so there may be no need for one to > > check the CRC of a UBI volume. > > That's true that ECC can help detecting corruptions, but I don't think > this is the actual reason for disabling CRC check at volume open time. > The actual reason for doing that is when the UBI volume user (in our > case the squashfs FS) is also checking data consistency on its own > (which is the case for squashfs). > > > > > Let's introduce a ubi.nocheck parameter that let one skip the CRC check > > when attaching a UBI volume. > > > > This also drastically speeds kernel boot by removing a potentially > > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board > > for a ~20MB UBI volume used as rootfs. > > Can you give the old and new open/mount time instead of telling how > much you gained on the whole boot-time? # grabserial -v -d /dev/ttyUSB0 -t "Starting kernel" Kernel command line is: rootfstype=squashfs root=/dev/ubiblock0_0 ubi.mtd=ubi ubi.block=0,rootfs mtdparts=nand:92M@36M(ubi) Current behaviour (CRC checking): [2.335558 0.017867] ubi0: attaching mtd7 [2.504021 0.168463] ubi0: scanning is finished [2.519824 0.015803] ubi0: attached mtd7 (name "ubi", size 92 MiB) [2.523987 0.004163] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes [2.530447 0.006460] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 512 [2.535836 0.005389] ubi0: VID header offset: 512 (aligned 512), data offset: 2048 [2.541601 0.005765] ubi0: good PEBs: 736, bad PEBs: 0, corrupted PEBs: 0 [2.547923 0.006322] ubi0: user volume: 3, internal volumes: 1, max. volumes count: 128 [2.554223 0.006300] ubi0: max/mean erase counter: 1/0, WL threshold: 4096, image sequence number: 463835917 [2.562460 0.008237] ubi0: available PEBs: 0, total reserved PEBs: 736, PEBs reserved for bad PEB handling: 20 [2.569779 0.007319] ubi0: background thread "ubi_bgt0d" started, PID 546 [4.550142 1.980363] block ubiblock0_0: created from ubi0:0(rootfs) Skipping CRC check (ubi.nocheck=0,rootfs): [2.349903 0.019126] ubi0: attaching mtd7 [2.516995 0.167092] ubi0: scanning is finished [2.527101 0.010106] ubi0: skipping CRC check for volume rootfs [2.536719 0.009618] ubi0: attached mtd7 (name "ubi", size 92 MiB) [2.541859 0.005140] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes [2.547744 0.005885] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 512 [2.552990 0.005246] ubi0: VID header offset: 512 (aligned 512), data offset: 2048 [2.560497 0.007507] ubi0: good PEBs: 736, bad PEBs: 0, corrupted PEBs: 0 [2.568010 0.007513] ubi0: user volume: 3, internal volumes: 1, max. volumes count: 128 [2.574723 0.006713] ubi0: max/mean erase counter: 1/0, WL threshold: 4096, image sequence number: 463835917 [2.586223 0.011500] ubi0: available PEBs: 0, total reserved PEBs: 736, PEBs reserved for bad PEB handling: 20 [2.592365 0.006142] ubi0: background thread "ubi_bgt0d" started, PID 545 [2.595960 0.003595] block ubiblock0_0: created from ubi0:0(rootfs) It seems like it's more in the magnitude of 2s rather than 3, but still. This is retrieved from the boot log. Thanks, Quentin
On Mon, 23 Apr 2018 11:40:09 +0200 Quentin Schulz <quentin.schulz@bootlin.com> wrote: > Hi Boris, > > On Fri, Apr 20, 2018 at 07:19:24PM +0200, Boris Brezillon wrote: > > On Fri, 20 Apr 2018 10:52:41 +0200 > > Quentin Schulz <quentin.schulz@bootlin.com> wrote: > > > > > There's already ECC on NAND pages so there may be no need for one to > > > check the CRC of a UBI volume. > > > > That's true that ECC can help detecting corruptions, but I don't think > > this is the actual reason for disabling CRC check at volume open time. > > The actual reason for doing that is when the UBI volume user (in our > > case the squashfs FS) is also checking data consistency on its own > > (which is the case for squashfs). > > > > > > > > Let's introduce a ubi.nocheck parameter that let one skip the CRC check > > > when attaching a UBI volume. > > > > > > This also drastically speeds kernel boot by removing a potentially > > > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board > > > for a ~20MB UBI volume used as rootfs. > > > > Can you give the old and new open/mount time instead of telling how > > much you gained on the whole boot-time? > > # grabserial -v -d /dev/ttyUSB0 -t "Starting kernel" > > Kernel command line is: > rootfstype=squashfs root=/dev/ubiblock0_0 ubi.mtd=ubi ubi.block=0,rootfs mtdparts=nand:92M@36M(ubi) > > Current behaviour (CRC checking): > [2.335558 0.017867] ubi0: attaching mtd7 > [2.504021 0.168463] ubi0: scanning is finished > [2.519824 0.015803] ubi0: attached mtd7 (name "ubi", size 92 MiB) > [2.523987 0.004163] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes > [2.530447 0.006460] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 512 > [2.535836 0.005389] ubi0: VID header offset: 512 (aligned 512), data offset: 2048 > [2.541601 0.005765] ubi0: good PEBs: 736, bad PEBs: 0, corrupted PEBs: 0 > [2.547923 0.006322] ubi0: user volume: 3, internal volumes: 1, max. volumes count: 128 > [2.554223 0.006300] ubi0: max/mean erase counter: 1/0, WL threshold: 4096, image sequence number: 463835917 > [2.562460 0.008237] ubi0: available PEBs: 0, total reserved PEBs: 736, PEBs reserved for bad PEB handling: 20 > [2.569779 0.007319] ubi0: background thread "ubi_bgt0d" started, PID 546 > [4.550142 1.980363] block ubiblock0_0: created from ubi0:0(rootfs) > > Skipping CRC check (ubi.nocheck=0,rootfs): > [2.349903 0.019126] ubi0: attaching mtd7 > [2.516995 0.167092] ubi0: scanning is finished > [2.527101 0.010106] ubi0: skipping CRC check for volume rootfs > [2.536719 0.009618] ubi0: attached mtd7 (name "ubi", size 92 MiB) > [2.541859 0.005140] ubi0: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes > [2.547744 0.005885] ubi0: min./max. I/O unit sizes: 2048/2048, sub-page size 512 > [2.552990 0.005246] ubi0: VID header offset: 512 (aligned 512), data offset: 2048 > [2.560497 0.007507] ubi0: good PEBs: 736, bad PEBs: 0, corrupted PEBs: 0 > [2.568010 0.007513] ubi0: user volume: 3, internal volumes: 1, max. volumes count: 128 > [2.574723 0.006713] ubi0: max/mean erase counter: 1/0, WL threshold: 4096, image sequence number: 463835917 > [2.586223 0.011500] ubi0: available PEBs: 0, total reserved PEBs: 736, PEBs reserved for bad PEB handling: 20 > [2.592365 0.006142] ubi0: background thread "ubi_bgt0d" started, PID 545 > [2.595960 0.003595] block ubiblock0_0: created from ubi0:0(rootfs) > > It seems like it's more in the magnitude of 2s rather than 3, but still. No, I meant add this information to the commit message since telling how many seconds are saved on the absolute boot-time does not give any information on the before vs after ratio.
Hi Richard, On Fri, Apr 20, 2018 at 11:37:07AM +0200, Richard Weinberger wrote: > Quentin, > > Am Freitag, 20. April 2018, 10:52:41 CEST schrieb Quentin Schulz: > > There's already ECC on NAND pages so there may be no need for one to > > check the CRC of a UBI volume. > > > > Let's introduce a ubi.nocheck parameter that let one skip the CRC check > > when attaching a UBI volume. > > > > This also drastically speeds kernel boot by removing a potentially > > useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board > > for a ~20MB UBI volume used as rootfs. > > You mean at *open* time of a *static* UBI volume? > So I guess the use case is having a read-only filesystem on top of ubiblock? > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > --- > > drivers/mtd/ubi/kapi.c | 70 +++++++++++++++++++++++++++++++++++++++++++- > > drivers/mtd/ubi/ubi.h | 6 ++++- > > drivers/mtd/ubi/vtbl.c | 24 ++++++++++++++- > > 3 files changed, 99 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c > > index d4b2e87..d604cd5 100644 > > --- a/drivers/mtd/ubi/kapi.c > > +++ b/drivers/mtd/ubi/kapi.c > > @@ -28,6 +28,9 @@ > > #include <asm/div64.h> > > #include "ubi.h" > > > > +struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES]; > > +int ubinocheck_devs; > > + > > /** > > * ubi_do_get_device_info - get information about UBI device. > > * @ubi: UBI device description object > > @@ -865,3 +868,70 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb) > > return blocking_notifier_chain_unregister(&ubi_notifiers, nb); > > } > > EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier); > > + > > +static int __init ubinocheckvol_set_param(const char *val, > > + const struct kernel_param *kp) > > +{ > > + int i, ret; > > + size_t len; > > + struct ubivol_param *param; > > + char buf[UBIVOL_PARAM_LEN]; > > + char *pbuf = &buf[0]; > > + char *tokens[UBIVOL_PARAM_COUNT]; > > + > > + if (!val) > > + return -EINVAL; > > + > > + len = strnlen(val, UBIVOL_PARAM_LEN); > > + if (len == 0) { > > + pr_warn("UBI: nocheck: empty 'nocheck=' parameter - ignored\n"); > > + return 0; > > + } > > + > > + if (len == UBIVOL_PARAM_LEN) { > > + pr_err("UBI: nocheck: parameter \"%s\" is too long, max. is %d\n", > > + val, UBIVOL_PARAM_LEN); > > + return -EINVAL; > > + } > > + > > + strcpy(buf, val); > > + > > + /* Get rid of the final newline */ > > + if (buf[len - 1] == '\n') > > + buf[len - 1] = '\0'; > > + > > + for (i = 0; i < UBIVOL_PARAM_COUNT; i++) > > + tokens[i] = strsep(&pbuf, ","); > > + > > + param = &ubinocheck_param[ubinocheck_devs]; > > + if (tokens[1]) { > > + /* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */ > > + ret = kstrtoint(tokens[0], 10, ¶m->ubi_num); > > + if (ret < 0) > > + return -EINVAL; > > + > > + /* Second param can be a number or a name */ > > + ret = kstrtoint(tokens[1], 10, ¶m->vol_id); > > + if (ret < 0) { > > + param->vol_id = -1; > > + strcpy(param->name, tokens[1]); > > + } > > + } > > > > Do we really need this per volume? If your flash is trustworthy, it should not matter. > Having it per UBI instance instead of volume would make the code less complicated. > In my opinion, yes. Here is my use case: I have one "big" UBI image with different volumes and filesystems. One of those is a squashfs that is mounted/checked by dm-verity. The hash check done by dm-verity is heavy and makes the CRC check done by UBI useless. I would like to still be able to check the CRC of the other UBI volumes but also have one or more volumes for which I bypass the CRC check done by UBI since it's useless and time consuming. By putting a flag per UBI image would require me to complexify the layout of my storage medium to have one UBI image for the volumes that don't need CRC check and one for the volumes that do. It's cumbersome and can be avoided IMHO. > > + ubinocheck_devs++; > > + > > + return 0; > > +} > > + > > +static const struct kernel_param_ops ubinocheckvol_param_ops = { > > + .set = ubinocheckvol_set_param, > > +}; > > +module_param_cb(nocheck, &ubinocheckvol_param_ops, NULL, 0); > > +MODULE_PARM_DESC(nocheck, > > + "Disable CRC check for UBI volumes. Parameter format: nocheck=dev,[num|name]>.\n" > > + "Multiple \"nocheck\" parameters may be specified.\n" > > + "Examples\n" > > + "Using the UBI device, and the volume name:\n" > > + "ubi.nocheck=0,rootfs\n" > > + "Using both UBI device number and UBI volume number:\n" > > + "ubi.nocheck=0,0\n"); > > diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h > > index 4cc6ec9..2bd89b4 100644 > > --- a/drivers/mtd/ubi/ubi.h > > +++ b/drivers/mtd/ubi/ubi.h > > @@ -825,6 +825,12 @@ struct ubivol_param { > > char name[UBIVOL_PARAM_LEN + 1]; > > }; > > > > +/* Numbers of elements set in the @ubinocheck_param array */ > > +extern int ubinocheck_devs; > > + > > +/* MTD devices specification parameters */ > > +extern struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES]; > > + > > #include "debug.h" > > > > extern struct kmem_cache *ubi_wl_entry_slab; > > diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c > > index 263743e..06b6cfd 100644 > > --- a/drivers/mtd/ubi/vtbl.c > > +++ b/drivers/mtd/ubi/vtbl.c > > @@ -534,9 +534,10 @@ 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, j, reserved_pebs = 0; > > struct ubi_ainf_volume *av; > > struct ubi_volume *vol; > > + struct ubivol_param *param; > > > > for (i = 0; i < ubi->vtbl_slots; i++) { > > cond_resched(); > > @@ -620,6 +621,27 @@ 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; > > + > > + for (j = 0; j < ubinocheck_devs; j++) { > > + param = &ubinocheck_param[j]; > > + > > + if (vol->ubi->ubi_num != param->ubi_num) > > + continue; > > + if (vol->vol_id == param->vol_id) { > > + ubi_msg(vol->ubi, > > + "skipping CRC check for volume %d", > > + vol->vol_id); > > + vol->checked = true; > > Please don't abuse the checked flag. > A new one a la "skip_check" does not hurt. I'll do that. > But again, I don't think we need it per volume. > > Also don't forget to add the nocheck parameter to the ioctl() interface, such that > you can specify it also with ubiattach... > If we go for a per-image flag, adding nocheck to the ioctl makes sense, otherwise we have to find a way to select only one or more volumes for which the nocheck flag should be set. That being said, I'm not sure a per-image flag is the way we want to go. What are your thoughts on this? Thanks, Quentin
Quentin, Am Montag, 11. Juni 2018, 12:20:37 CEST schrieb Quentin Schulz: > If we go for a per-image flag, adding nocheck to the ioctl makes sense, > otherwise we have to find a way to select only one or more volumes for > which the nocheck flag should be set. That being said, I'm not sure a > per-image flag is the way we want to go. > > What are your thoughts on this? I like that approach a lot more, please send a patch. :-) Having a volume flag will reduce complexity a lot. Thanks, //richard
On Thu, 14 Jun 2018 09:29:42 +0200 Richard Weinberger <richard@nod.at> wrote: > Quentin, > > Am Montag, 11. Juni 2018, 12:20:37 CEST schrieb Quentin Schulz: > > If we go for a per-image flag, adding nocheck to the ioctl makes sense, > > otherwise we have to find a way to select only one or more volumes for > > which the nocheck flag should be set. That being said, I'm not sure a > > per-image flag is the way we want to go. > > > > What are your thoughts on this? > > I like that approach a lot more, please send a patch. :-) I don't think the approach you're talking about has been mentioned by Quentin in this email :-). To clarify the discussion, we're talking about adding a new flag to VTBL entries telling whether the CRC should be skipped or not. > Having a volume flag will reduce complexity a lot. That's my opinion as well.
Am Donnerstag, 14. Juni 2018, 10:04:36 CEST schrieb Boris Brezillon: > On Thu, 14 Jun 2018 09:29:42 +0200 > Richard Weinberger <richard@nod.at> wrote: > > > Quentin, > > > > Am Montag, 11. Juni 2018, 12:20:37 CEST schrieb Quentin Schulz: > > > If we go for a per-image flag, adding nocheck to the ioctl makes sense, > > > otherwise we have to find a way to select only one or more volumes for > > > which the nocheck flag should be set. That being said, I'm not sure a > > > per-image flag is the way we want to go. > > > > > > What are your thoughts on this? > > > > I like that approach a lot more, please send a patch. :-) > > I don't think the approach you're talking about has been mentioned by > Quentin in this email :-). To clarify the discussion, we're talking > about adding a new flag to VTBL entries telling whether the CRC should > be skipped or not. Hehe yes. Too many discussions on and off-list in parallel. ;) Thanks, //richard
diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c index d4b2e87..d604cd5 100644 --- a/drivers/mtd/ubi/kapi.c +++ b/drivers/mtd/ubi/kapi.c @@ -28,6 +28,9 @@ #include <asm/div64.h> #include "ubi.h" +struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES]; +int ubinocheck_devs; + /** * ubi_do_get_device_info - get information about UBI device. * @ubi: UBI device description object @@ -865,3 +868,70 @@ int ubi_unregister_volume_notifier(struct notifier_block *nb) return blocking_notifier_chain_unregister(&ubi_notifiers, nb); } EXPORT_SYMBOL_GPL(ubi_unregister_volume_notifier); + +static int __init ubinocheckvol_set_param(const char *val, + const struct kernel_param *kp) +{ + int i, ret; + size_t len; + struct ubivol_param *param; + char buf[UBIVOL_PARAM_LEN]; + char *pbuf = &buf[0]; + char *tokens[UBIVOL_PARAM_COUNT]; + + if (!val) + return -EINVAL; + + len = strnlen(val, UBIVOL_PARAM_LEN); + if (len == 0) { + pr_warn("UBI: nocheck: empty 'nocheck=' parameter - ignored\n"); + return 0; + } + + if (len == UBIVOL_PARAM_LEN) { + pr_err("UBI: nocheck: parameter \"%s\" is too long, max. is %d\n", + val, UBIVOL_PARAM_LEN); + return -EINVAL; + } + + strcpy(buf, val); + + /* Get rid of the final newline */ + if (buf[len - 1] == '\n') + buf[len - 1] = '\0'; + + for (i = 0; i < UBIVOL_PARAM_COUNT; i++) + tokens[i] = strsep(&pbuf, ","); + + param = &ubinocheck_param[ubinocheck_devs]; + if (tokens[1]) { + /* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */ + ret = kstrtoint(tokens[0], 10, ¶m->ubi_num); + if (ret < 0) + return -EINVAL; + + /* Second param can be a number or a name */ + ret = kstrtoint(tokens[1], 10, ¶m->vol_id); + if (ret < 0) { + param->vol_id = -1; + strcpy(param->name, tokens[1]); + } + } + + ubinocheck_devs++; + + return 0; +} + +static const struct kernel_param_ops ubinocheckvol_param_ops = { + .set = ubinocheckvol_set_param, +}; +module_param_cb(nocheck, &ubinocheckvol_param_ops, NULL, 0); +MODULE_PARM_DESC(nocheck, + "Disable CRC check for UBI volumes. Parameter format: nocheck=dev,[num|name]>.\n" + "Multiple \"nocheck\" parameters may be specified.\n" + "Examples\n" + "Using the UBI device, and the volume name:\n" + "ubi.nocheck=0,rootfs\n" + "Using both UBI device number and UBI volume number:\n" + "ubi.nocheck=0,0\n"); diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h index 4cc6ec9..2bd89b4 100644 --- a/drivers/mtd/ubi/ubi.h +++ b/drivers/mtd/ubi/ubi.h @@ -825,6 +825,12 @@ struct ubivol_param { char name[UBIVOL_PARAM_LEN + 1]; }; +/* Numbers of elements set in the @ubinocheck_param array */ +extern int ubinocheck_devs; + +/* MTD devices specification parameters */ +extern struct ubivol_param ubinocheck_param[UBIVOL_MAX_DEVICES]; + #include "debug.h" extern struct kmem_cache *ubi_wl_entry_slab; diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c index 263743e..06b6cfd 100644 --- a/drivers/mtd/ubi/vtbl.c +++ b/drivers/mtd/ubi/vtbl.c @@ -534,9 +534,10 @@ 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, j, reserved_pebs = 0; struct ubi_ainf_volume *av; struct ubi_volume *vol; + struct ubivol_param *param; for (i = 0; i < ubi->vtbl_slots; i++) { cond_resched(); @@ -620,6 +621,27 @@ 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; + + for (j = 0; j < ubinocheck_devs; j++) { + param = &ubinocheck_param[j]; + + if (vol->ubi->ubi_num != param->ubi_num) + continue; + if (vol->vol_id == param->vol_id) { + ubi_msg(vol->ubi, + "skipping CRC check for volume %d", + vol->vol_id); + vol->checked = true; + break; + } + if (!strcmp(vol->name, param->name)) { + ubi_msg(vol->ubi, + "skipping CRC check for volume %s", + param->name); + vol->checked = true; + break; + } + } } /* And add the layout volume */
There's already ECC on NAND pages so there may be no need for one to check the CRC of a UBI volume. Let's introduce a ubi.nocheck parameter that let one skip the CRC check when attaching a UBI volume. This also drastically speeds kernel boot by removing a potentially useless check, e.g. I gained 3.2s on boot time of a SPEAr600-based board for a ~20MB UBI volume used as rootfs. Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> --- drivers/mtd/ubi/kapi.c | 70 +++++++++++++++++++++++++++++++++++++++++++- drivers/mtd/ubi/ubi.h | 6 ++++- drivers/mtd/ubi/vtbl.c | 24 ++++++++++++++- 3 files changed, 99 insertions(+), 1 deletion(-)