[PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue

[PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue

[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.

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)
 			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;
+				}
 			dbg_rcvry("found a master node at %d:%d", lnum, offs);
 			*mst = buf;
 			offs += sz;
 			buf  += sz;
 			len  -= sz;
-		}
+			}
 	}
+
 	/* 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;
 	*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));
 
-- 
1.9.1

Re: [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue

[Richard Weinberger]

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

RE: [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue

[Bean Huo 霍斌斌 (beanhuo)]

Dear Richard

> 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?

No, this is not one part of my path series, just a single and dedicated to 
Master node.

> > This patch has been tested on Micron MLC NAND MT29F32G08CBADAWP.
> > Issue descripted:

> > http://lists.infradead.org/pipermail/linux-mtd/2015-November/063016.ht
> > ml
> >
> > 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.
> 
For Master node block, just last page data is valid, so I think( I am limited on UBIFS knowledge, maybe I am wrong),
UBIFS should search for last page that not be programed, then search last valid master node page.
In case of there is one random data page, but last page of master block is a valid master node.
unless we can make sure that don't exist this scenario.
> >  			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.

Here should be what type of check? Can you give me some suggestions?


> >  			dbg_rcvry("found a master node at %d:%d", lnum, offs);
> >  			*mst = buf;
> >  			offs += sz;
> >  			buf  += sz;
> >  			len  -= sz;
> > -		}
> > +			}
> >  	}
> > +
> 
> Useless line break. :)
This will be improved in next version.

> >  	/* 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. :(
Yes, this check should add again, that is be more reasonable.
> >  	*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.


Currently, we get more feedbacks from our customers who are using MLC NAND,
They more like UBIFS more reliable, Even can tolerate to discard some user
Data after next power on. Means that they don't want to UBIFS mount failed just
Because of power loss, If to discard the data for the stability of the system, they
prefer to choose the latter.

For UBIFS master node on MLC NAND, I often found that one of master node block is OK,
But because of second master node block exist two pages damaged data, recovery always 
Fails. Not matter SLC or MLC, as long as there is a good master node, recovery must be 
Successful.


> Thanks,
> //richard

Re: [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue

[Richard Weinberger]

Bean,

Am 14.12.2015 um 04:55 schrieb Bean Huo 霍斌斌 (beanhuo):
> Dear Richard
> 
>> 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?
> 
> No, this is not one part of my path series, just a single and dedicated to 
> Master node.

[...]

> Currently, we get more feedbacks from our customers who are using MLC NAND,
> They more like UBIFS more reliable, Even can tolerate to discard some user
> Data after next power on. Means that they don't want to UBIFS mount failed just
> Because of power loss, If to discard the data for the stability of the system, they
> prefer to choose the latter.

MLC is currently simply not supported. If your hardware does not have a mechanism
do temper power-loss the paired page issue will damage UBI and UBIFS.
Please correct me if I'm wrong but this patch just papers over one symptom of that.

> For UBIFS master node on MLC NAND, I often found that one of master node block is OK,
> But because of second master node block exist two pages damaged data, recovery always 
> Fails. Not matter SLC or MLC, as long as there is a good master node, recovery must be 
> Successful.

This needs a much more detailed explanation.
In which scenarios on SLC NAND can you get such an unmountable UBIFS?
Maybe UBIFS is too strict and NAND behaves differently than UBIFS expects
but we need to understand it in depth.

Thanks,
//richard

RE: [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue

[Bean Huo 霍斌斌 (beanhuo)]

> Bean,
> 
> Am 14.12.2015 um 04:55 schrieb Bean Huo 霍斌斌 (beanhuo):
> > Dear Richard
> >
> >> 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?
> >
> > No, this is not one part of my path series, just a single and
> > dedicated to Master node.
> 
> [...]
> 
> > Currently, we get more feedbacks from our customers who are using MLC
> > NAND, They more like UBIFS more reliable, Even can tolerate to discard
> > some user Data after next power on. Means that they don't want to
> > UBIFS mount failed just Because of power loss, If to discard the data
> > for the stability of the system, they prefer to choose the latter.
> 
> MLC is currently simply not supported. If your hardware does not have a
> mechanism do temper power-loss the paired page issue will damage UBI and
> UBIFS.
> Please correct me if I'm wrong but this patch just papers over one symptom
> of that.
> 
> > For UBIFS master node on MLC NAND, I often found that one of master
> > node block is OK, But because of second master node block exist two
> > pages damaged data, recovery always Fails. Not matter SLC or MLC, as
> > long as there is a good master node, recovery must be Successful.
> 
> This needs a much more detailed explanation.
> In which scenarios on SLC NAND can you get such an unmountable UBIFS?


It is my mistake involved SLC NAND.
Definitely, SLC NAND does not exist two pages being damaged within one block.
I mean that master should be recovered as long as one good master block exists.
I think, at least this method is more reasonable.
My question is that why UBI doesn't recover master node for this scenario?

> Maybe UBIFS is too strict and NAND behaves differently than UBIFS expects
> but we need to understand it in depth.
For this, I think, maybe MLC NAND had not been released yet when UBI initial design.
I would like to send my version 2 patch based on your suggestion.

> Thanks,
> //richard

Re: [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue

[Richard Weinberger]

Bean,

Am 28.01.2016 um 03:42 schrieb Bean Huo 霍斌斌 (beanhuo):
>> This needs a much more detailed explanation.
>> In which scenarios on SLC NAND can you get such an unmountable UBIFS?
> 
> 
> It is my mistake involved SLC NAND.
> Definitely, SLC NAND does not exist two pages being damaged within one block.
> I mean that master should be recovered as long as one good master block exists.
> I think, at least this method is more reasonable.
> My question is that why UBI doesn't recover master node for this scenario?

UBIFS assumes that on SLC NAND already written pages must not
corrupt. It can deal with the fact that pages can get damaged while writing them (think of a power cut).
But if page X is written and UBIFS moves over to X + 1, X must never corrupt.

If this happens, something very nasty happened and UBIFS cannot operate correctly.
With your patch it may mount somehow but *will* die or lose data soon or later as the same assumptions
apply to all other UBIFS operations. It is not just about master nodes.
Master nodes are the messenger.

UBIFS' strict checks turned out to be very valuable in the past to identify driver/MTD issues.
This is why I like them so much.

>> Maybe UBIFS is too strict and NAND behaves differently than UBIFS expects
>> but we need to understand it in depth.
> For this, I think, maybe MLC NAND had not been released yet when UBI initial design.
> I would like to send my version 2 patch based on your suggestion.

If you can explain in detail why UBIFS' assumptions are wrong and how such corruptions can happen
on SLC we can talk.
But I think then we'd have to redo a lot of UBI and UBIFS code.

Thanks,
//richard

RE: [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue

[Bean Huo 霍斌斌 (beanhuo)]

Hi, Richard

> From: Richard Weinberger [mailto:richard@nod.at]
> Sent: Thursday, January 28, 2016 5:32 PM
> To: Bean Huo 霍斌斌 (beanhuo); Artem Bityutskiy; Adrian Hunter; Brian
> Norris
> Cc: linux-mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Boris
> Brezillon; Peter Pan 潘栋 (peterpandong); Karl Zhang 张双锣 (karlzhang);
> Jason Tian 田晓强 (jasontian)
> Subject: Re: [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master
> node issue
> 
> Bean,
> 
> Am 28.01.2016 um 03:42 schrieb Bean Huo 霍斌斌 (beanhuo):
> >> This needs a much more detailed explanation.
> >> In which scenarios on SLC NAND can you get such an unmountable UBIFS?
> >
> >
> > It is my mistake involved SLC NAND.
> > Definitely, SLC NAND does not exist two pages being damaged within one
> block.
> > I mean that master should be recovered as long as one good master block
> >exists.
> > I think, at least this method is more reasonable.
> > My question is that why UBI doesn't recover master node for this scenario?
> 
> UBIFS assumes that on SLC NAND already written pages must not corrupt. It
> can deal with the fact that pages can get damaged while writing them (think
> of a power cut).
> But if page X is written and UBIFS moves over to X + 1, X must never corrupt.
> 
> If this happens, something very nasty happened and UBIFS cannot operate
> correctly.

Right, for SLC NAND, X will not be damaged, because power loss happened
While programming X +1.

> With your patch it may mount somehow but *will* die or lose data soon or
> later as the same assumptions apply to all other UBIFS operations. It is not
> just about master nodes.
> Master nodes are the messenger.
>
> UBIFS' strict checks turned out to be very valuable in the past to identify
> driver/MTD issues.
> This is why I like them so much.

> >> Maybe UBIFS is too strict and NAND behaves differently than UBIFS
> >> expects but we need to understand it in depth.
> > For this, I think, maybe MLC NAND had not been released yet when UBI
> initial design.
> > I would like to send my version 2 patch based on your suggestion.
> 
> If you can explain in detail why UBIFS' assumptions are wrong and how such
> corruptions can happen on SLC we can talk.
> But I think then we'd have to redo a lot of UBI and UBIFS code.

I will hack my patch again, and double check these strict checks.
But I still insist on Master node should always be recovered by another good master,
even if two corrupted pages exist in one block. This is more reasonable and reliable.
Of course, so far, we did not meet this scenario on SLC NAND.
Current UBIFS master node recovery mechanism totally can handle with
Power loss no matter MLC or SLC, why not let UBIFS more reliable? Two master node blocks
Just for SLC NAND?
> Thanks,
> //richard

Re: [PATCH 1/1] fs:ubifs:recovery:fixup UBIFS cannot recover master node issue

[Richard Weinberger]

Bean,

Am 01.02.2016 um 08:17 schrieb Bean Huo 霍斌斌 (beanhuo):
>> If you can explain in detail why UBIFS' assumptions are wrong and how such
>> corruptions can happen on SLC we can talk.
>> But I think then we'd have to redo a lot of UBI and UBIFS code.
> 
> I will hack my patch again, and double check these strict checks.
> But I still insist on Master node should always be recovered by another good master,
> even if two corrupted pages exist in one block. This is more reasonable and reliable.
> Of course, so far, we did not meet this scenario on SLC NAND.
> Current UBIFS master node recovery mechanism totally can handle with
> Power loss no matter MLC or SLC, why not let UBIFS more reliable? Two master node blocks
> Just for SLC NAND?

Of course, I'm all for improvements. But if you talk about "more reliable" you have to define
first what the issue is.
As I said, we have this strict checks for reasons and they did a very
good service so far.
I've seen a lot UBIFS corruptions where the master node was damaged but not a single time
it was UBIFS' fault. It was always a subtle MTD driver issue.

Thanks,
//richard