| Message ID | 3f9cef553d4f5da3fe2d76113d79ada1f0fe5224.1530169759.git-series.quentin.schulz@bootlin.com |
|---|---|
| State | Accepted |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | ubi: add possibility to skip CRC check for static UBI volumes | expand |
Am Donnerstag, 28. Juni 2018, 09:40:52 CEST schrieb Quentin Schulz: > Some users of static UBI volumes implement their own integrity check, > thus making the volume CRC check done at open time useless. For > instance, this is the case when one use the ubiblock + dm-verity + > squashfs combination, where dm-verity already checks integrity of the > block device but this time at the block granularity instead of verifying > the whole volume. > > Skipping this test drastically improves the boot-time. > > Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com> Reviewed-by: Richard Weinberger <richard@nod.at> Thanks, //richard
Hi, On Thu, 2018-06-28 at 09:40 +0200, Quentin Schulz wrote: > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c > index d4b2e87..e9e9ecb 100644 > --- a/drivers/mtd/ubi/kapi.c > +++ b/drivers/mtd/ubi/kapi.c > @@ -202,7 +202,7 @@ struct ubi_volume_desc *ubi_open_volume(int ubi_num, int vol_id, int mode) > desc->mode = mode; > > mutex_lock(&ubi->ckvol_mutex); > - if (!vol->checked) { > + if (!vol->checked && !vol->skip_check) { > /* This is the first open - check the volume */ > err = ubi_check_volume(ubi, vol_id); > if (err < 0) { Did you deliberately did not add a similar check to 'vol_cdev_write()' ? You want to skip checking on load but do have the checking after volume update ? Looks a bit inconsistent to me. At the very least deserves a comment in 'vol_cdev_write()' about why 'skip_check' flag is ignored there.
Artem, Am Montag, 2. Juli 2018, 09:30:25 CEST schrieb Artem Bityutskiy: > Hi, > > On Thu, 2018-06-28 at 09:40 +0200, Quentin Schulz wrote: > > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c > > index d4b2e87..e9e9ecb 100644 > > --- a/drivers/mtd/ubi/kapi.c > > +++ b/drivers/mtd/ubi/kapi.c > > @@ -202,7 +202,7 @@ struct ubi_volume_desc *ubi_open_volume(int ubi_num, int vol_id, int mode) > > desc->mode = mode; > > > > mutex_lock(&ubi->ckvol_mutex); > > - if (!vol->checked) { > > + if (!vol->checked && !vol->skip_check) { > > /* This is the first open - check the volume */ > > err = ubi_check_volume(ubi, vol_id); > > if (err < 0) { > > Did you deliberately did not add a similar check to 'vol_cdev_write()' ? > You want to skip checking on load but do have the checking after volume update ? > Looks a bit inconsistent to me. At the very least deserves a comment in > 'vol_cdev_write()' about why 'skip_check' flag is ignored there. Well, the idea is skipping the check, not the crc32 on the medium. That way we can later, if needed, offer a way to drop the flag but and don't have to rewrite with crc32-enabled. Thanks, //richard
On Mon, 2018-07-02 at 09:43 +0200, Richard Weinberger wrote: > Artem, > > Am Montag, 2. Juli 2018, 09:30:25 CEST schrieb Artem Bityutskiy: > > Hi, > > > > On Thu, 2018-06-28 at 09:40 +0200, Quentin Schulz wrote: > > > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c > > > index d4b2e87..e9e9ecb 100644 > > > --- a/drivers/mtd/ubi/kapi.c > > > +++ b/drivers/mtd/ubi/kapi.c > > > @@ -202,7 +202,7 @@ struct ubi_volume_desc *ubi_open_volume(int > > > ubi_num, int vol_id, int mode) > > > desc->mode = mode; > > > > > > mutex_lock(&ubi->ckvol_mutex); > > > - if (!vol->checked) { > > > + if (!vol->checked && !vol->skip_check) { > > > /* This is the first open - check the volume */ > > > err = ubi_check_volume(ubi, vol_id); > > > if (err < 0) { > > > > Did you deliberately did not add a similar check to > > 'vol_cdev_write()' ? > > You want to skip checking on load but do have the checking after > > volume update ? > > Looks a bit inconsistent to me. At the very least deserves a > > comment in > > 'vol_cdev_write()' about why 'skip_check' flag is ignored there. > > Well, the idea is skipping the check, not the crc32 on the medium. > That way we can later, if needed, offer a way to drop the flag but > and don't have to rewrite with crc32-enabled. I understand that. I am talking about cdev.c line 370. We will check the CRC after the update regardless of 'skip_check'. So I point out that this is not very consistent with what we do in 'ubi_open_volume()'. Is this deliberate or not? If this is deliberate, we should at least add an explanation comment in 'vol_cdev_write()'.
Hi Artem, On Mon, 02 Jul 2018 10:30:25 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote: > Hi, > > On Thu, 2018-06-28 at 09:40 +0200, Quentin Schulz wrote: > > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c > > index d4b2e87..e9e9ecb 100644 > > --- a/drivers/mtd/ubi/kapi.c > > +++ b/drivers/mtd/ubi/kapi.c > > @@ -202,7 +202,7 @@ struct ubi_volume_desc *ubi_open_volume(int ubi_num, int vol_id, int mode) > > desc->mode = mode; > > > > mutex_lock(&ubi->ckvol_mutex); > > - if (!vol->checked) { > > + if (!vol->checked && !vol->skip_check) { > > /* This is the first open - check the volume */ > > err = ubi_check_volume(ubi, vol_id); > > if (err < 0) { > > Did you deliberately did not add a similar check to 'vol_cdev_write()' ? > You want to skip checking on load but do have the checking after volume update ? Yep, it's on purpose, I asked Quentin to keep the test on the update volume path. > Looks a bit inconsistent to me. At the very least deserves a comment in > 'vol_cdev_write()' about why 'skip_check' flag is ignored there. Well, I thought checking the CRC just after updating the volume made sense, just to make sure things were written correctly on the medium. Let's add a comment explaining why we keep the check here, unless you see a strong reason to get rid of this check in the update path. Regards, Boris
Am Montag, 2. Juli 2018, 09:51:21 CEST schrieb Boris Brezillon: > Hi Artem, > > On Mon, 02 Jul 2018 10:30:25 +0300 > Artem Bityutskiy <dedekind1@gmail.com> wrote: > > > Hi, > > > > On Thu, 2018-06-28 at 09:40 +0200, Quentin Schulz wrote: > > > diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c > > > index d4b2e87..e9e9ecb 100644 > > > --- a/drivers/mtd/ubi/kapi.c > > > +++ b/drivers/mtd/ubi/kapi.c > > > @@ -202,7 +202,7 @@ struct ubi_volume_desc *ubi_open_volume(int ubi_num, int vol_id, int mode) > > > desc->mode = mode; > > > > > > mutex_lock(&ubi->ckvol_mutex); > > > - if (!vol->checked) { > > > + if (!vol->checked && !vol->skip_check) { > > > /* This is the first open - check the volume */ > > > err = ubi_check_volume(ubi, vol_id); > > > if (err < 0) { > > > > Did you deliberately did not add a similar check to 'vol_cdev_write()' ? > > You want to skip checking on load but do have the checking after volume update ? > > Yep, it's on purpose, I asked Quentin to keep the test on the update > volume path. > > > Looks a bit inconsistent to me. At the very least deserves a comment in > > 'vol_cdev_write()' about why 'skip_check' flag is ignored there. > > Well, I thought checking the CRC just after updating the volume made > sense, just to make sure things were written correctly on the medium. > Let's add a comment explaining why we keep the check here, unless you > see a strong reason to get rid of this check in the update path. +1 I also vote for keeping the check. vol->skip_check is really just "skip the check upon volume open". Thanks, //richard
On Mon, 2018-07-02 at 09:51 +0200, Boris Brezillon wrote: > Well, I thought checking the CRC just after updating the volume made > sense, just to make sure things were written correctly on the medium. > Let's add a comment explaining why we keep the check here, unless you > see a strong reason to get rid of this check in the update path. I am fine with this, thanks.
diff --git a/drivers/mtd/ubi/kapi.c b/drivers/mtd/ubi/kapi.c index d4b2e87..e9e9ecb 100644 --- a/drivers/mtd/ubi/kapi.c +++ b/drivers/mtd/ubi/kapi.c @@ -202,7 +202,7 @@ struct ubi_volume_desc *ubi_open_volume(int ubi_num, int vol_id, int mode) desc->mode = mode; mutex_lock(&ubi->ckvol_mutex); - if (!vol->checked) { + if (!vol->checked && !vol->skip_check) { /* This is the first open - check the volume */ err = ubi_check_volume(ubi, vol_id); if (err < 0) { diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h index 195ff8c..b5fe8f8 100644 --- a/drivers/mtd/ubi/ubi-media.h +++ b/drivers/mtd/ubi/ubi-media.h @@ -45,6 +45,11 @@ enum { * Volume flags used in the volume table record. * * @UBI_VTBL_AUTORESIZE_FLG: auto-resize this volume + * @UBI_VTBL_SKIP_CRC_CHECK_FLG: skip the CRC check done on a static volume at + * open time. Should only be set on volumes that + * are used by upper layers doing this kind of + * check. Main use-case for this flag is + * boot-time reduction * * %UBI_VTBL_AUTORESIZE_FLG flag can be set only for one volume in the volume * table. UBI automatically re-sizes the volume which has this flag and makes @@ -76,6 +81,7 @@ enum { */ enum { UBI_VTBL_AUTORESIZE_FLG = 0x01, + UBI_VTBL_SKIP_CRC_CHECK_FLG = 0x02, }; /* diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h index f5ba97c..d47b9e4 100644 --- a/drivers/mtd/ubi/ubi.h +++ b/drivers/mtd/ubi/ubi.h @@ -327,6 +327,9 @@ struct ubi_eba_leb_desc { * atomic LEB change * * @eba_tbl: EBA table of this volume (LEB->PEB mapping) + * @skip_check: %1 if CRC check of this static volume should be skipped. + * Directly reflects the presence of the + * %UBI_VTBL_SKIP_CRC_CHECK_FLG flag in the vtbl entry * @checked: %1 if this static volume was checked * @corrupted: %1 if the volume is corrupted (static volumes only) * @upd_marker: %1 if the update marker is set for this volume @@ -374,6 +377,7 @@ struct ubi_volume { void *upd_buf; struct ubi_eba_table *eba_tbl; + unsigned int skip_check:1; unsigned int checked:1; unsigned int corrupted:1; unsigned int upd_marker:1; diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c index 0be5167..e2606a4 100644 --- a/drivers/mtd/ubi/vmt.c +++ b/drivers/mtd/ubi/vmt.c @@ -299,6 +299,10 @@ int ubi_create_volume(struct ubi_device *ubi, struct ubi_mkvol_req *req) vtbl_rec.vol_type = UBI_VID_DYNAMIC; else vtbl_rec.vol_type = UBI_VID_STATIC; + + if (vol->skip_check) + vtbl_rec.flags |= UBI_VTBL_SKIP_CRC_CHECK_FLG; + memcpy(vtbl_rec.name, vol->name, vol->name_len); err = ubi_change_vtbl_record(ubi, vol_id, &vtbl_rec); @@ -733,6 +737,11 @@ static int self_check_volume(struct ubi_device *ubi, int vol_id) ubi_err(ubi, "bad used_bytes"); goto fail; } + + if (vol->skip_check) { + ubi_err(ubi, "bad skip_check"); + goto fail; + } } else { if (vol->used_ebs < 0 || vol->used_ebs > vol->reserved_pebs) { ubi_err(ubi, "bad used_ebs"); diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c index 94d7a86..2c133cd 100644 --- a/drivers/mtd/ubi/vtbl.c +++ b/drivers/mtd/ubi/vtbl.c @@ -560,6 +560,9 @@ static int init_volumes(struct ubi_device *ubi, vol->name[vol->name_len] = '\0'; vol->vol_id = i; + if (vtbl[i].flags & UBI_VTBL_SKIP_CRC_CHECK_FLG) + vol->skip_check = 1; + if (vtbl[i].flags & UBI_VTBL_AUTORESIZE_FLG) { /* Auto re-size flag may be set only for one volume */ if (ubi->autoresize_vol_id != -1) {