| Message ID | 1329250006-22944-8-git-send-email-rw@linutronix.de |
|---|---|
| State | RFC |
| Headers | show |
On Tue, 14 Feb 2012 21:06:46 +0100 Richard Weinberger <rw@linutronix.de> wrote: > +#ifdef CONFIG_MTD_UBI_CHECKPOINT > +static int attach_by_checkpointing(struct ubi_device *ubi) > +{ > + int cp_start, err; > + struct ubi_scan_info *si; > + > + cp_start = ubi_find_checkpoint(ubi); > + if (cp_start < 0) > + return -ENOENT; > + > + si = ubi_read_checkpoint(ubi, cp_start); > + if (IS_ERR(si)) > + return PTR_ERR(si); > + > + ubi->bad_peb_count = 0; > + ubi->good_peb_count = ubi->peb_count; Zero reported bad PEBs when checkpointing. Seems that checkpointing does not remember number/location of bad PEBs. Are we fine with that? > +#ifdef CONFIG_MTD_UBI_CHECKPOINT > + err = attach_by_checkpointing(ubi); > + > + if (err) { > + if (err != -ENOENT) > + ubi_msg("falling back to attach by scanning mode!\n"); > + > + err = attach_by_scanning(ubi); > + } Code does not fit error message. Message states "falling back to scanning" only if "err != -ENOENT". However code calls 'attach_by_scanning' regardless 'err'. Was it your intention? Regards, Shmulik
Am 19.02.2012 14:57, schrieb Shmulik Ladkani: > On Tue, 14 Feb 2012 21:06:46 +0100 Richard Weinberger<rw@linutronix.de> wrote: >> +#ifdef CONFIG_MTD_UBI_CHECKPOINT >> +static int attach_by_checkpointing(struct ubi_device *ubi) >> +{ >> + int cp_start, err; >> + struct ubi_scan_info *si; >> + >> + cp_start = ubi_find_checkpoint(ubi); >> + if (cp_start< 0) >> + return -ENOENT; >> + >> + si = ubi_read_checkpoint(ubi, cp_start); >> + if (IS_ERR(si)) >> + return PTR_ERR(si); >> + >> + ubi->bad_peb_count = 0; >> + ubi->good_peb_count = ubi->peb_count; > > Zero reported bad PEBs when checkpointing. > Seems that checkpointing does not remember number/location of bad PEBs. Currently checkpointing cares only about used and free PEBs. Bad PEBs are no longer visible to UBI after recovering from a checkpoint. > Are we fine with that? This patch is a RFC. :-) >> +#ifdef CONFIG_MTD_UBI_CHECKPOINT >> + err = attach_by_checkpointing(ubi); >> + >> + if (err) { >> + if (err != -ENOENT) >> + ubi_msg("falling back to attach by scanning mode!\n"); >> + >> + err = attach_by_scanning(ubi); >> + } > > Code does not fit error message. > Message states "falling back to scanning" only if "err != -ENOENT". > However code calls 'attach_by_scanning' regardless 'err'. > Was it your intention? Yes. If recovering from a checkpoint fails the corresponding code prints a human readable error message in any case. Thanks, //richard
On Sun, 19 Feb 2012 15:08:44 +0100 Richard Weinberger <rw@linutronix.de> wrote: > Am 19.02.2012 14:57, schrieb Shmulik Ladkani: > > On Tue, 14 Feb 2012 21:06:46 +0100 Richard Weinberger<rw@linutronix.de> wrote: > >> +#ifdef CONFIG_MTD_UBI_CHECKPOINT > >> +static int attach_by_checkpointing(struct ubi_device *ubi) > >> +{ > >> + int cp_start, err; > >> + struct ubi_scan_info *si; > >> + > >> + cp_start = ubi_find_checkpoint(ubi); > >> + if (cp_start< 0) > >> + return -ENOENT; > >> + > >> + si = ubi_read_checkpoint(ubi, cp_start); > >> + if (IS_ERR(si)) > >> + return PTR_ERR(si); > >> + > >> + ubi->bad_peb_count = 0; > >> + ubi->good_peb_count = ubi->peb_count; > > > > Zero reported bad PEBs when checkpointing. > > Seems that checkpointing does not remember number/location of bad PEBs. > > Currently checkpointing cares only about used and free PEBs. > Bad PEBs are no longer visible to UBI after recovering from a checkpoint. Ok. However it is still reported to the log in 'ubi_attach_mtd_dev' and as a sysfs attribute. BTW, the counter is still incremented by WL subsystem, though. Hence, reported value will be bad PEBs encountered since last attach (where formerly, it was absolute total bad PEBs in the ubi device). Maybe remove 'bad_peb_count' altogether. Also, "ubi->good_peb_count = ubi->peb_count" results in different 'beb_rsvd_level' caculation, see 'ubi_calculate_reserved'. > > Are we fine with that? > > This patch is a RFC. :-) So I noticed :-) Just trying to point out things which cause ubi system to behave differently. > Thanks, > //richard Wellcome. Will try to review the big stuff later on. Regards, Shmulik
Am 19.02.2012 15:40, schrieb Shmulik Ladkani: > On Sun, 19 Feb 2012 15:08:44 +0100 Richard Weinberger<rw@linutronix.de> wrote: >> Am 19.02.2012 14:57, schrieb Shmulik Ladkani: >>> On Tue, 14 Feb 2012 21:06:46 +0100 Richard Weinberger<rw@linutronix.de> wrote: >>>> +#ifdef CONFIG_MTD_UBI_CHECKPOINT >>>> +static int attach_by_checkpointing(struct ubi_device *ubi) >>>> +{ >>>> + int cp_start, err; >>>> + struct ubi_scan_info *si; >>>> + >>>> + cp_start = ubi_find_checkpoint(ubi); >>>> + if (cp_start< 0) >>>> + return -ENOENT; >>>> + >>>> + si = ubi_read_checkpoint(ubi, cp_start); >>>> + if (IS_ERR(si)) >>>> + return PTR_ERR(si); >>>> + >>>> + ubi->bad_peb_count = 0; >>>> + ubi->good_peb_count = ubi->peb_count; >>> >>> Zero reported bad PEBs when checkpointing. >>> Seems that checkpointing does not remember number/location of bad PEBs. >> >> Currently checkpointing cares only about used and free PEBs. >> Bad PEBs are no longer visible to UBI after recovering from a checkpoint. > > Ok. > However it is still reported to the log in 'ubi_attach_mtd_dev' > and as a sysfs attribute. > BTW, the counter is still incremented by WL subsystem, though. > Hence, reported value will be bad PEBs encountered since last attach > (where formerly, it was absolute total bad PEBs in the ubi device). > Maybe remove 'bad_peb_count' altogether. > > Also, "ubi->good_peb_count = ubi->peb_count" results in different > 'beb_rsvd_level' caculation, see 'ubi_calculate_reserved'. I can add these counters into the checkpoint. >>> Are we fine with that? >> >> This patch is a RFC. :-) > > So I noticed :-) > Just trying to point out things which cause ubi system to behave > differently. > Thanks a lot for reviewing this feature! //richard
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index 115749f..06c5c77 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -148,6 +148,17 @@ int ubi_volume_notify(struct ubi_device *ubi, struct ubi_volume *vol, int ntype) ubi_do_get_device_info(ubi, &nt.di); ubi_do_get_volume_info(ubi, vol, &nt.vi); + +#ifdef CONFIG_MTD_UBI_CHECKPOINT + switch (ntype) { + case UBI_VOLUME_ADDED: + case UBI_VOLUME_REMOVED: + case UBI_VOLUME_RESIZED: + case UBI_VOLUME_RENAMED: + if (ubi_update_checkpoint(ubi)) + ubi_err("Unable to update checkpoint!"); + } +#endif return blocking_notifier_call_chain(&ubi_notifiers, ntype, &nt); } @@ -852,6 +863,61 @@ static int autoresize(struct ubi_device *ubi, int vol_id) return 0; } +#ifdef CONFIG_MTD_UBI_CHECKPOINT +static int attach_by_checkpointing(struct ubi_device *ubi) +{ + int cp_start, err; + struct ubi_scan_info *si; + + cp_start = ubi_find_checkpoint(ubi); + if (cp_start < 0) + return -ENOENT; + + si = ubi_read_checkpoint(ubi, cp_start); + if (IS_ERR(si)) + return PTR_ERR(si); + + ubi->bad_peb_count = 0; + ubi->good_peb_count = ubi->peb_count; + ubi->corr_peb_count = 0; + ubi->max_ec = si->max_ec; + ubi->mean_ec = si->mean_ec; + ubi_msg("max. sequence number: %llu", si->max_sqnum); + + err = ubi_read_volume_table(ubi, si); + if (err) { + ubi_err("ubi_read_volume_table failed"); + goto out_si; + } + + err = ubi_wl_init_scan(ubi, si); + if (err) { + ubi_err("ubi_wl_init_scan failed!"); + goto out_vtbl; + } + + err = ubi_eba_init_scan(ubi, si); + if (err) { + ubi_err("ubi_eba_init_scan failed!"); + goto out_wl; + } + + ubi_msg("successfully recovered from checkpoint!"); + ubi_scan_destroy_si(si); + return 0; + +out_wl: + ubi_wl_close(ubi); +out_vtbl: + free_internal_volumes(ubi); + vfree(ubi->vtbl); +out_si: + ubi_scan_destroy_si(si); + + return err; +} +#endif + /** * ubi_attach_mtd_dev - attach an MTD device. * @mtd: MTD device description object @@ -931,6 +997,12 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset) ubi->vid_hdr_offset = vid_hdr_offset; ubi->autoresize_vol_id = -1; +#ifdef CONFIG_MTD_UBI_CHECKPOINT + ubi->long_pool.used = ubi->long_pool.size = ubi->long_pool.max_size = ARRAY_SIZE(ubi->long_pool.pebs); + ubi->short_pool.used = ubi->short_pool.size = ubi->short_pool.max_size = ARRAY_SIZE(ubi->short_pool.pebs); + ubi->unk_pool.used = ubi->unk_pool.size = ubi->unk_pool.max_size = ARRAY_SIZE(ubi->unk_pool.pebs); +#endif + mutex_init(&ubi->buf_mutex); mutex_init(&ubi->ckvol_mutex); mutex_init(&ubi->device_mutex); @@ -957,7 +1029,18 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset) if (err) goto out_free; +#ifdef CONFIG_MTD_UBI_CHECKPOINT + err = attach_by_checkpointing(ubi); + + if (err) { + if (err != -ENOENT) + ubi_msg("falling back to attach by scanning mode!\n"); + + err = attach_by_scanning(ubi); + } +#else err = attach_by_scanning(ubi); +#endif if (err) { dbg_err("failed to attach by scanning, error %d", err); goto out_debugging;
Signed-off-by: Richard Weinberger <rw@linutronix.de> --- drivers/mtd/ubi/build.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 83 insertions(+), 0 deletions(-)