Re: [PATCH 2/2] ubi: introduce ubi.nocheck parameter to skip CRC check when attaching ubi vol

[Quentin Schulz <quentin.schulz@bootlin.com>]

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

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, &param->ubi_num);
> > +		if (ret < 0)
> > +			return -EINVAL;
> > +
> > +		/* Second param can be a number or a name */
> > +		ret = kstrtoint(tokens[1], 10, &param->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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]