Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1289309 > unrolled thread
| Started by | Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> |
|---|---|
| First post | 2015-12-11 09:30 +0100 |
| Last post | 2015-12-11 10:20 +0100 |
| Articles | 2 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> - 2015-12-11 09:30 +0100
Re: [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue Richard Weinberger <richard@nod.at> - 2015-12-11 10:20 +0100
| From | Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> |
|---|---|
| Date | 2015-12-11 09:30 +0100 |
| Subject | [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue |
| Message-ID | <qErfs-1Fq-13@gated-at.bofh.it> |
Rm9yIE1MQyBOQU5ELCBwYWlyZWQgcGFnZSBpc3N1ZSBpcyBub3cgYSBjb21tb24ga25vd24gaXNz dWUuDQpUaGlzIHBhdGNoIGlzIGp1c3QgZm9yIG1hc3RlciBub2RlIGNhbm5vdCBiZSByZWNvdmVy ZWQgd2hpbGUNCnRoZXJlIHdpbGwgdHdvIHBhZ2VzIGJlIGRhbWFnZWQgaW4gb25lIHNpbmdsZSBt YXN0ZXIgbm9kZSBibG9jay4NCkFzIGZvciB0aGlzIHBhdGNoLCBpZiB0aGVyZSBhcmUgbW9yZSB0 aGFuIG9uZSBwYWdlIGRhdGEgaW4NCm1hc3RlciBub2RlIGJsb2NrIGJlaW5nIGRhbWFnZWQsIGFu ZCBhcyBsb25nIGFzIGV4aXN0IG9uZQ0KdW5jb3JydXB0ZWQgbWFzdGVyIG5vZGUgYmxvY2ssIG1h c3RlciBub2RlIHdpbGwgYmUgcmVjb3ZlcmVkLg0KDQpUaGlzIHBhdGNoIGhhcyBiZWVuIHRlc3Rl ZCBvbiBNaWNyb24gTUxDIE5BTkQgTVQyOUYzMkcwOENCQURBV1AuDQpJc3N1ZSBkZXNjcmlwdGVk Og0KaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvcGlwZXJtYWlsL2xpbnV4LW10ZC8yMDE1LU5v dmVtYmVyLzA2MzAxNi5odG1sDQoNClNpZ25lZC1vZmYtYnk6IEJlYW4gSHVvIDxiZWFuaHVvQG1p Y3Jvbi5jb20+DQotLS0NCiBmcy91Ymlmcy9yZWNvdmVyeS5jIHwgNzUgKysrKysrKysrKysrKysr KysrKysrKysrKysrKysrKysrKy0tLS0tLS0tLS0tLS0tLS0tLS0NCiAxIGZpbGUgY2hhbmdlZCwg NDkgaW5zZXJ0aW9ucygrKSwgMjYgZGVsZXRpb25zKC0pDQoNCmRpZmYgLS1naXQgYS9mcy91Ymlm cy9yZWNvdmVyeS5jIGIvZnMvdWJpZnMvcmVjb3ZlcnkuYw0KaW5kZXggNjk1ZmM3MS4uZTMxNTRl NiAxMDA2NDQNCi0tLSBhL2ZzL3ViaWZzL3JlY292ZXJ5LmMNCisrKyBiL2ZzL3ViaWZzL3JlY292 ZXJ5LmMNCkBAIC0xMjgsNyArMTI4LDcgQEAgc3RhdGljIGludCBnZXRfbWFzdGVyX25vZGUoY29u c3Qgc3RydWN0IHViaWZzX2luZm8gKmMsIGludCBsbnVtLCB2b2lkICoqcGJ1ZiwNCiAJd2hpbGUg KG9mZnMgKyBVQklGU19NU1RfTk9ERV9TWiA8PSBjLT5sZWJfc2l6ZSkgew0KIAkJc3RydWN0IHVi aWZzX2NoICpjaCA9IGJ1ZjsNCiANCi0JCWlmIChsZTMyX3RvX2NwdShjaC0+bWFnaWMpICE9IFVC SUZTX05PREVfTUFHSUMpDQorCQlpZiAobGUzMl90b19jcHUoY2gtPm1hZ2ljKSA9PSAweEZGRkZG RkZGKQ0KIAkJCWJyZWFrOw0KIAkJb2ZmcyArPSBzejsNCiAJCWJ1ZiAgKz0gc3o7DQpAQCAtMTM3 LDM3ICsxMzcsNDAgQEAgc3RhdGljIGludCBnZXRfbWFzdGVyX25vZGUoY29uc3Qgc3RydWN0IHVi aWZzX2luZm8gKmMsIGludCBsbnVtLCB2b2lkICoqcGJ1ZiwNCiAJLyogU2VlIGlmIHRoZXJlIHdh cyBhIHZhbGlkIG1hc3RlciBub2RlIGJlZm9yZSB0aGF0ICovDQogCWlmIChvZmZzKSB7DQogCQlp bnQgcmV0Ow0KLQ0KK3JldHJ5Og0KIAkJb2ZmcyAtPSBzejsNCiAJCWJ1ZiAgLT0gc3o7DQogCQls ZW4gICs9IHN6Ow0KIAkJcmV0ID0gdWJpZnNfc2Nhbl9hX25vZGUoYywgYnVmLCBsZW4sIGxudW0s IG9mZnMsIDEpOw0KLQkJaWYgKHJldCAhPSBTQ0FOTkVEX0FfTk9ERSAmJiBvZmZzKSB7DQotCQkJ LyogQ291bGQgaGF2ZSBiZWVuIGNvcnJ1cHRpb24gc28gY2hlY2sgb25lIHBsYWNlIGJhY2sgKi8N Ci0JCQlvZmZzIC09IHN6Ow0KLQkJCWJ1ZiAgLT0gc3o7DQotCQkJbGVuICArPSBzejsNCi0JCQly ZXQgPSB1Ymlmc19zY2FuX2Ffbm9kZShjLCBidWYsIGxlbiwgbG51bSwgb2ZmcywgMSk7DQotCQkJ aWYgKHJldCAhPSBTQ0FOTkVEX0FfTk9ERSkNCi0JCQkJLyoNCi0JCQkJICogV2UgYWNjZXB0IG9u bHkgb25lIGFyZWEgb2YgY29ycnVwdGlvbiBiZWNhdXNlDQotCQkJCSAqIHdlIGFyZSBhc3N1bWlu ZyB0aGF0IGl0IHdhcyBjYXVzZWQgd2hpbGUNCi0JCQkJICogdHJ5aW5nIHRvIHdyaXRlIGEgbWFz dGVyIG5vZGUuDQotCQkJCSAqLw0KLQkJCQlnb3RvIG91dF9lcnI7DQotCQl9DQotCQlpZiAocmV0 ID09IFNDQU5ORURfQV9OT0RFKSB7DQotCQkJc3RydWN0IHViaWZzX2NoICpjaCA9IGJ1ZjsNCi0N Ci0JCQlpZiAoY2gtPm5vZGVfdHlwZSAhPSBVQklGU19NU1RfTk9ERSkNCisJCWlmIChyZXQgIT0g U0NBTk5FRF9BX05PREUpIHsNCisJCQkvKiBDb3VsZCBoYXZlIGJlZW4gY29ycnVwdGlvbiBzbyBj aGVjayBtb3JlDQorCQkJICogcGxhY2UgYmFjay4gV2UgYWNjZXB0IHR3byBhcmVhcyBvZiBjb3Jy dXB0aW9uDQorCQkJICogYmVjYXVzZSB3ZSBhcmUgYXNzdW1pbmcgdGhhdCBmb3IgTUxDIE5BTkQs aXQNCisJCQkgKiB3YXMgY2F1c2VkIHdoaWxlIHRyeWluZyB0byB3cml0ZSBhIGxvd2VyDQorCQkJ ICogcGFnZSwgdXBwZXIgcGFnZSBiZWluZyBkYW1hZ2VkLg0KKwkJCSAqLw0KKwkJCWlmIChvZmZz ID4gMCkNCisJCQkJZ290byByZXRyeTsNCisJCQllbHNlDQogCQkJCWdvdG8gb3V0X2VycjsNCisJ CQl9DQorCQkJaWYgKHJldCA9PSBTQ0FOTkVEX0FfTk9ERSkgew0KKwkJCQlzdHJ1Y3QgdWJpZnNf Y2ggKmNoID0gYnVmOw0KKw0KKwkJCQlpZiAoY2gtPm5vZGVfdHlwZSAhPSBVQklGU19NU1RfTk9E RSkgew0KKwkJCQkJaWYgKG9mZnMpDQorCQkJCQkJZ290byByZXRyeTsNCisJCQkJCWVsc2UNCisJ CQkJCQlnb3RvIG91dF9lcnI7DQorCQkJCX0NCiAJCQlkYmdfcmN2cnkoImZvdW5kIGEgbWFzdGVy IG5vZGUgYXQgJWQ6JWQiLCBsbnVtLCBvZmZzKTsNCiAJCQkqbXN0ID0gYnVmOw0KIAkJCW9mZnMg Kz0gc3o7DQogCQkJYnVmICArPSBzejsNCiAJCQlsZW4gIC09IHN6Ow0KLQkJfQ0KKwkJCX0NCiAJ fQ0KKw0KIAkvKiBDaGVjayBmb3IgY29ycnVwdGlvbiAqLw0KIAlpZiAob2ZmcyA8IGMtPmxlYl9z aXplKSB7DQogCQlpZiAoIWlzX2VtcHR5KGJ1ZiwgbWluX3QoaW50LCBsZW4sIHN6KSkpIHsNCkBA IC0xNzgsMTAgKzE4MSw2IEBAIHN0YXRpYyBpbnQgZ2V0X21hc3Rlcl9ub2RlKGNvbnN0IHN0cnVj dCB1Ymlmc19pbmZvICpjLCBpbnQgbG51bSwgdm9pZCAqKnBidWYsDQogCQlidWYgICs9IHN6Ow0K IAkJbGVuICAtPSBzejsNCiAJfQ0KLQkvKiBDaGVjayByZW1haW5pbmcgZW1wdHkgc3BhY2UgKi8N Ci0JaWYgKG9mZnMgPCBjLT5sZWJfc2l6ZSkNCi0JCWlmICghaXNfZW1wdHkoYnVmLCBsZW4pKQ0K LQkJCWdvdG8gb3V0X2VycjsNCiAJKnBidWYgPSBzYnVmOw0KIAlyZXR1cm4gMDsNCiANCkBAIC0y MzYsNyArMjM1LDcgQEAgb3V0Og0KIGludCB1Ymlmc19yZWNvdmVyX21hc3Rlcl9ub2RlKHN0cnVj dCB1Ymlmc19pbmZvICpjKQ0KIHsNCiAJdm9pZCAqYnVmMSA9IE5VTEwsICpidWYyID0gTlVMTCwg KmNvcjEgPSBOVUxMLCAqY29yMiA9IE5VTEw7DQotCXN0cnVjdCB1Ymlmc19tc3Rfbm9kZSAqbXN0 MSA9IE5VTEwsICptc3QyID0gTlVMTCwgKm1zdDsNCisJc3RydWN0IHViaWZzX21zdF9ub2RlICpt c3QxID0gTlVMTCwgKm1zdDIgPSBOVUxMLCAqbXN0ID0gTlVMTDsNCiAJY29uc3QgaW50IHN6ID0g Yy0+bXN0X25vZGVfYWxzejsNCiAJaW50IGVyciwgb2ZmczEsIG9mZnMyOw0KIA0KQEAgLTI4MCw2 ICsyNzksMjggQEAgaW50IHViaWZzX3JlY292ZXJfbWFzdGVyX25vZGUoc3RydWN0IHViaWZzX2lu Zm8gKmMpDQogCQkJCWlmIChjb3IxKQ0KIAkJCQkJZ290byBvdXRfZXJyOw0KIAkJCQltc3QgPSBt c3QxOw0KKwkJCX0gZWxzZSBpZiAob2ZmczIgKyBzeiAhPSBvZmZzMSkgew0KKwkJCQlpZiAobGUz Ml90b19jcHUobXN0MS0+Y2guc3FudW0pID4NCisJCQkJCWxlMzJfdG9fY3B1KG1zdDItPmNoLnNx bnVtKSkgew0KKwkJCQkJLyoNCisJCQkJCSAqIDFzdCBMRUIgd3JpdHRlbiwgb2NjdXJyZWQgcG93 ZXINCisJCQkJCSAqIGxvc3Mgd2hpbGUgd3JpdGluZyAybmQgTEVCLg0KKwkJCQkJICovDQorCQkJ CQlpZiAoY29yMSkNCisJCQkJCQlnb3RvIG91dF9lcnI7DQorCQkJCQltc3QgPSBtc3QxOw0KKwkJ CQl9IGVsc2UgaWYgKGxlMzJfdG9fY3B1KG1zdDEtPmNoLnNxbnVtKSA8DQorCQkJCQlsZTMyX3Rv X2NwdShtc3QyLT5jaC5zcW51bSkpIHsNCisJCQkJLyogV2hpbGUgd3JpdGluZyAxc3QgTEVCLCBv Y2N1cnJlZCBwb3dlciBsb3NzICovDQorCQkJCQlpZiAoIWNvcjIpIHsNCisJCQkJCQlpZiAobXN0 Mi0+ZmxhZ3MgJg0KKwkJCQkJCSAgIGNwdV90b19sZTMyKFVCSUZTX01TVF9ESVJUWSkpDQorCQkJ CQkJCW1zdCA9IG1zdDI7DQorCQkJCQkJZWxzZQ0KKwkJCQkJCQlnb3RvIG91dF9lcnI7DQorCQkJ CQl9IGVsc2UNCisJCQkJCWdvdG8gb3V0X2VycjsNCisJCQkJfQ0KIAkJCX0gZWxzZQ0KIAkJCQln b3RvIG91dF9lcnI7DQogCQl9IGVsc2Ugew0KQEAgLTMwNSw2ICszMjYsOCBAQCBpbnQgdWJpZnNf cmVjb3Zlcl9tYXN0ZXJfbm9kZShzdHJ1Y3QgdWJpZnNfaW5mbyAqYykNCiAJCW1zdCA9IG1zdDI7 DQogCX0NCiANCisJaWYgKG1zdCA9PSBOVUxMKQ0KKwkJZ290byBvdXRfZXJyOw0KIAl1Ymlmc19t c2coYywgInJlY292ZXJlZCBtYXN0ZXIgbm9kZSBmcm9tIExFQiAlZCIsDQogCQkgIChtc3QgPT0g bXN0MSA/IFVCSUZTX01TVF9MTlVNIDogVUJJRlNfTVNUX0xOVU0gKyAxKSk7DQogDQotLSANCjEu OS4xDQo= -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Richard Weinberger <richard@nod.at> |
|---|---|
| Date | 2015-12-11 10:20 +0100 |
| Message-ID | <qEs1R-2d9-21@gated-at.bofh.it> |
| In reply to | #1289309 |
Bean,
Am 11.12.2015 um 09:26 schrieb Bean Huo 霍斌斌 (beanhuo):
> For MLC NAND, paired page issue is now a common known issue.
> This patch is just for master node cannot be recovered while
> there will two pages be damaged in one single master node block.
> As for this patch, if there are more than one page data in
> master node block being damaged, and as long as exist one
> uncorrupted master node block, master node will be recovered.
So, this patch is part if a larger patch series to make UBIFS MLC aware?
> This patch has been tested on Micron MLC NAND MT29F32G08CBADAWP.
> Issue descripted:
> http://lists.infradead.org/pipermail/linux-mtd/2015-November/063016.html
>
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
> fs/ubifs/recovery.c | 75 ++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 49 insertions(+), 26 deletions(-)
>
> diff --git a/fs/ubifs/recovery.c b/fs/ubifs/recovery.c
> index 695fc71..e3154e6 100644
> --- a/fs/ubifs/recovery.c
> +++ b/fs/ubifs/recovery.c
> @@ -128,7 +128,7 @@ static int get_master_node(const struct ubifs_info *c, int lnum, void **pbuf,
> while (offs + UBIFS_MST_NODE_SZ <= c->leb_size) {
> struct ubifs_ch *ch = buf;
>
> - if (le32_to_cpu(ch->magic) != UBIFS_NODE_MAGIC)
> + if (le32_to_cpu(ch->magic) == 0xFFFFFFFF)
The purpose of this check was to trigger upon garbage data (including 0xFF).
Now you only check for 0xFF bytes.
> break;
> offs += sz;
> buf += sz;
> @@ -137,37 +137,40 @@ static int get_master_node(const struct ubifs_info *c, int lnum, void **pbuf,
> /* See if there was a valid master node before that */
> if (offs) {
> int ret;
> -
> +retry:
> offs -= sz;
> buf -= sz;
> len += sz;
> ret = ubifs_scan_a_node(c, buf, len, lnum, offs, 1);
> - if (ret != SCANNED_A_NODE && offs) {
> - /* Could have been corruption so check one place back */
> - offs -= sz;
> - buf -= sz;
> - len += sz;
> - ret = ubifs_scan_a_node(c, buf, len, lnum, offs, 1);
> - if (ret != SCANNED_A_NODE)
> - /*
> - * We accept only one area of corruption because
> - * we are assuming that it was caused while
> - * trying to write a master node.
> - */
> - goto out_err;
> - }
> - if (ret == SCANNED_A_NODE) {
> - struct ubifs_ch *ch = buf;
> -
> - if (ch->node_type != UBIFS_MST_NODE)
> + if (ret != SCANNED_A_NODE) {
> + /* Could have been corruption so check more
> + * place back. We accept two areas of corruption
> + * because we are assuming that for MLC NAND,it
> + * was caused while trying to write a lower
> + * page, upper page being damaged.
> + */
> + if (offs > 0)
> + goto retry;
> + else
> goto out_err;
> + }
> + if (ret == SCANNED_A_NODE) {
> + struct ubifs_ch *ch = buf;
> +
> + if (ch->node_type != UBIFS_MST_NODE) {
> + if (offs)
> + goto retry;
> + else
> + goto out_err;
> + }
Here you kill another sanity check.
> dbg_rcvry("found a master node at %d:%d", lnum, offs);
> *mst = buf;
> offs += sz;
> buf += sz;
> len -= sz;
> - }
> + }
> }
> +
Useless line break. :)
> /* Check for corruption */
> if (offs < c->leb_size) {
> if (!is_empty(buf, min_t(int, len, sz))) {
> @@ -178,10 +181,6 @@ static int get_master_node(const struct ubifs_info *c, int lnum, void **pbuf,
> buf += sz;
> len -= sz;
> }
> - /* Check remaining empty space */
> - if (offs < c->leb_size)
> - if (!is_empty(buf, len))
> - goto out_err;
Another check gone. :(
> *pbuf = sbuf;
> return 0;
>
> @@ -236,7 +235,7 @@ out:
> int ubifs_recover_master_node(struct ubifs_info *c)
> {
> void *buf1 = NULL, *buf2 = NULL, *cor1 = NULL, *cor2 = NULL;
> - struct ubifs_mst_node *mst1 = NULL, *mst2 = NULL, *mst;
> + struct ubifs_mst_node *mst1 = NULL, *mst2 = NULL, *mst = NULL;
> const int sz = c->mst_node_alsz;
> int err, offs1, offs2;
>
> @@ -280,6 +279,28 @@ int ubifs_recover_master_node(struct ubifs_info *c)
> if (cor1)
> goto out_err;
> mst = mst1;
> + } else if (offs2 + sz != offs1) {
> + if (le32_to_cpu(mst1->ch.sqnum) >
> + le32_to_cpu(mst2->ch.sqnum)) {
> + /*
> + * 1st LEB written, occurred power
> + * loss while writing 2nd LEB.
> + */
> + if (cor1)
> + goto out_err;
> + mst = mst1;
> + } else if (le32_to_cpu(mst1->ch.sqnum) <
> + le32_to_cpu(mst2->ch.sqnum)) {
> + /* While writing 1st LEB, occurred power loss */
> + if (!cor2) {
> + if (mst2->flags &
> + cpu_to_le32(UBIFS_MST_DIRTY))
> + mst = mst2;
> + else
> + goto out_err;
> + } else
> + goto out_err;
> + }
> } else
> goto out_err;
> } else {
> @@ -305,6 +326,8 @@ int ubifs_recover_master_node(struct ubifs_info *c)
> mst = mst2;
> }
>
> + if (mst == NULL)
> + goto out_err;
> ubifs_msg(c, "recovered master node from LEB %d",
> (mst == mst1 ? UBIFS_MST_LNUM : UBIFS_MST_LNUM + 1));
That said, please explain your patch in more detail. i.e. Why do you remove these checks?
Why is this correct to do so?
To me it looks like an ad-hoc solution to make UBIFS not
abort on your MLC by removing well-established checks.
I agree that UBIFS's master node checks are very picky but for SLC they are correct and make a lot of sense.
Adding MLC support must not hurt UBIFS's SLC robustness.
Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web