[PATCH 2/3] ubifs: add ubifs_mount_fixup_lebs()

[PATCH 2/3] ubifs: add ubifs_mount_fixup_lebs()

[Matthew L. Creech]

This call scans all LEBs in the filesystem for those that are in-use but have
one or more empty pages at the end, then re-maps the LEBs in order to erase the
empty portions.  Afterward it removes the "leb_fixup" flag from the UBIFS
superblock.

Signed-off-by: Matthew L. Creech <mlcreech@gmail.com>
---
 fs/ubifs/sb.c    |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ubifs/ubifs.h |    1 +
 2 files changed, 137 insertions(+), 0 deletions(-)

diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index e3777d0..2afc3ad 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -652,3 +652,139 @@ out:
 	kfree(sup);
 	return err;
 }
+
+/**
+ * ubifs_fixup_leb - scan LEB for empty pages & remap if needed
+ * @c: UBIFS file-system description object
+ * @lnum: LEB number
+ * @nfree: Number of free bytes at the end of the LEB
+ *
+ * This function reads the contents of the given LEB, then remaps it to a
+ * new PEB, so that any empty pages are actually erased on flash (rather than
+ * being just all-0xff real data).
+ */
+static int ubifs_fixup_leb(struct ubifs_info *c, int lnum, int nfree)
+{
+	int err = 0, aligned_len;
+	int len = c->leb_size - nfree;
+	void *sbuf = c->sbuf;
+
+	if (unlikely(len<=0))
+		return 0;
+
+	dbg_mnt("fixup LEB %d (len %d)", lnum, len);
+
+	/* Read the existing valid data for this LEB */
+	err = ubi_read(c->ubi, lnum, sbuf, 0, len);
+	if (err && err != -EBADMSG) {
+		ubifs_err("cannot read %d bytes from LEB %d, error %d",
+			  len, lnum, err);
+		goto out;
+	}
+
+	/* Pad if necessary */
+	aligned_len = ALIGN(len, c->min_io_size);
+	if (aligned_len > len) {
+		int pad_len = aligned_len - ALIGN(len, 8);
+
+		if (pad_len > 0) {
+			void *buf = sbuf + aligned_len - pad_len;
+
+			ubifs_pad(c, buf, pad_len);
+		}
+	}
+
+	/* Atomically change this LEB's mapping */
+	err = ubi_leb_change(c->ubi, lnum, sbuf, aligned_len, UBI_UNKNOWN);
+out:
+	return err;
+}
+
+/**
+ * ubifs_fixup_all_lebs - find & remap all LEBs with trailing empty pages
+ * @c: UBIFS file-system description object
+ *
+ * This function walks through all LEBs in the filesystem and remaps those
+ * which are in-use and have trailing empty pages.
+ */
+static int ubifs_fixup_all_lebs(struct ubifs_info *c)
+{
+	int lnum, err = 0;
+	struct ubifs_lprops *lprops;
+
+	ubifs_get_lprops(c);
+
+	/*
+	 * Find all in-use LEBs and remap them to new PEBs, which will erase
+	 * any "empty" pages (they might currently exist as real 0xff data).
+	 */
+	for (lnum = 2; lnum <= c->leb_cnt; lnum++) {
+		lprops=ubifs_lpt_lookup(c, lnum);
+		if (IS_ERR(lprops)) {
+			err = PTR_ERR(lprops);
+			goto out;
+		}
+
+		if (!(lprops->flags & LPROPS_EMPTY) &&
+		    (lprops->free>=c->min_io_size)) {
+			/* Non-empty LEB with at least 1 free page */
+			err = ubifs_fixup_leb(c, lnum, lprops->free);
+			if (err)
+				goto out;
+		}
+	}
+
+out:
+	ubifs_release_lprops(c);
+	return err;
+}
+
+/**
+ * ubifs_mount_fixup_lebs - erase empty pages on first mount (for NAND)
+ * @c: UBIFS file-system description object
+ *
+ * This function fixes up LEBs on first mount, if the appropriate flag was set
+ * when the FS was created.  Each LEB with trailing "empty" (all-0xff) pages
+ * is re-written, to make sure the empty space is actually erased.  This is
+ * necessary for some NAND chips, since the 0xff data may have been programmed
+ * like real data (generating a non-0xff ECC), causing future writes to the
+ * not-really-erased pages to behave badly.  After fixup, the superblock flag
+ * is removed so that this is skipped for all future mounts.
+ */
+int ubifs_mount_fixup_lebs(struct ubifs_info *c)
+{
+	int err = 0, sup_flags = 0;
+	struct ubifs_sb_node *sup;
+
+	ubifs_assert(c->leb_fixup);
+	ubifs_assert(!c->ro_mount);
+
+	ubifs_msg("LEB fixup needed");
+
+	err = ubifs_fixup_all_lebs(c);
+	if (err)
+		goto out;
+
+	sup = ubifs_read_sb_node(c);
+	if (IS_ERR(sup)) {
+		err = PTR_ERR(sup);
+		goto out;
+	}
+
+	/* LEB fixup is no longer required */
+	c->leb_fixup = 0;
+
+	/* Set new flags, omitting LEB fixup */
+	sup_flags = 0;
+	if (c->big_lpt)
+		sup_flags |= UBIFS_FLG_BIGLPT;
+	sup->flags = cpu_to_le32(sup_flags);
+
+	err = ubifs_write_sb_node(c, sup);
+	if (err)
+		goto out;
+
+	ubifs_msg("LEB fixup complete");
+out:
+	return err;
+}
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 03e08ea..376d565 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1635,6 +1635,7 @@ int ubifs_write_master(struct ubifs_info *c);
 int ubifs_read_superblock(struct ubifs_info *c);
 struct ubifs_sb_node *ubifs_read_sb_node(struct ubifs_info *c);
 int ubifs_write_sb_node(struct ubifs_info *c, struct ubifs_sb_node *sup);
+int ubifs_mount_fixup_lebs(struct ubifs_info *c);

 /* replay.c */
 int ubifs_validate_entry(struct ubifs_info *c,
-- 
1.6.3.3


-- 
Matthew L. Creech

Re: [PATCH 2/3] ubifs: add ubifs_mount_fixup_lebs()

[Artem Bityutskiy]

On Tue, 2011-05-03 at 18:55 -0400, Matthew L. Creech wrote:
> This call scans all LEBs in the filesystem for those that are in-use but have
> one or more empty pages at the end, then re-maps the LEBs in order to erase the
> empty portions.  Afterward it removes the "leb_fixup" flag from the UBIFS
> superblock.
> 
> Signed-off-by: Matthew L. Creech <mlcreech@gmail.com>
> ---
>  fs/ubifs/sb.c    |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/ubifs/ubifs.h |    1 +
>  2 files changed, 137 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
> index e3777d0..2afc3ad 100644
> --- a/fs/ubifs/sb.c
> +++ b/fs/ubifs/sb.c
> @@ -652,3 +652,139 @@ out:
>  	kfree(sup);
>  	return err;
>  }
> +
> +/**
> + * ubifs_fixup_leb - scan LEB for empty pages & remap if needed

Nitpick, but to be 100% consistent - put a do after the first sentence
of all kerneldoc comments, because all the UBIFS code does this.

> + * @c: UBIFS file-system description object
> + * @lnum: LEB number
> + * @nfree: Number of free bytes at the end of the LEB
> + *
> + * This function reads the contents of the given LEB, then remaps it to a
> + * new PEB, so that any empty pages are actually erased on flash (rather than
> + * being just all-0xff real data).
> + */
> +static int ubifs_fixup_leb(struct ubifs_info *c, int lnum, int nfree)

Please, do not add the "ubifs_" prefix for static function. Not sure if
UBIFS is very consistent in this, but we tried to use this prefix only
for non-static (visible outside) functions to make sure we do not have
any namespace clashes.

> +{
> +	int err = 0, aligned_len;
> +	int len = c->leb_size - nfree;

We also tried to define all variables of the same type in one line, if
they fit. I mean

int a, b, c;

instead of

int a, b;
int c;

> +	void *sbuf = c->sbuf;
> +
> +	if (unlikely(len<=0))
> +		return 0;
> +
> +	dbg_mnt("fixup LEB %d (len %d)", lnum, len);
> +
> +	/* Read the existing valid data for this LEB */
> +	err = ubi_read(c->ubi, lnum, sbuf, 0, len);
> +	if (err && err != -EBADMSG) {
> +		ubifs_err("cannot read %d bytes from LEB %d, error %d",
> +			  len, lnum, err);

No need to print the error message here - UBI will do it if it fails.

> +		goto out;
> +	}
> +
> +	/* Pad if necessary */
> +	aligned_len = ALIGN(len, c->min_io_size);

Hmm, I think len returned by lprops is always min. I/O size - aligned,
so the below piece of code should not be necessary. Just ass
ubifs_assert(len % c->min_io_size == 0) here to make double sure.

> +	if (aligned_len > len) {
> +		int pad_len = aligned_len - ALIGN(len, 8);
> +
> +		if (pad_len > 0) {
> +			void *buf = sbuf + aligned_len - pad_len;
> +
> +			ubifs_pad(c, buf, pad_len);
> +		}
> +	}
> +
> +	/* Atomically change this LEB's mapping */
> +	err = ubi_leb_change(c->ubi, lnum, sbuf, aligned_len, UBI_UNKNOWN);
> +out:
> +	return err;

I'd kill this "out:" label.

> +}
> +
> +/**
> + * ubifs_fixup_all_lebs - find & remap all LEBs with trailing empty pages

dot.

> + * @c: UBIFS file-system description object
> + *
> + * This function walks through all LEBs in the filesystem and remaps those
> + * which are in-use and have trailing empty pages.
> + */
> +static int ubifs_fixup_all_lebs(struct ubifs_info *c)

So according to my suggestion this would be named:

fixup_free_space()

or something like this. Please, if you agree with my naming suggestion,
revise all comments and prints and amend them as well please.

> +{
> +	int lnum, err = 0;
> +	struct ubifs_lprops *lprops;
> +
> +	ubifs_get_lprops(c);
> +
> +	/*
> +	 * Find all in-use LEBs and remap them to new PEBs, which will erase
> +	 * any "empty" pages (they might currently exist as real 0xff data).
> +	 */
> +	for (lnum = 2; lnum <= c->leb_cnt; lnum++) {
> +		lprops=ubifs_lpt_lookup(c, lnum);
> +		if (IS_ERR(lprops)) {
> +			err = PTR_ERR(lprops);
> +			goto out;
> +		}
> +
> +		if (!(lprops->flags & LPROPS_EMPTY) &&

I think this is not quite right, because empty taken LEBs will not be
fixed up. I think you should only look at free space and nothing else.

> +		    (lprops->free>=c->min_io_size)) {
> +			/* Non-empty LEB with at least 1 free page */
> +			err = ubifs_fixup_leb(c, lnum, lprops->free)

And actually for free LEBs (lprops->free == c->leb_size) we just need to
unmap them ubi_leb_unmap() is the function name AFAIR. I mean, for
completely free LEBs we might have 2 situations:

1. Most often - it is unmapped, then unmap operation will be noop
2. Rarely it may be mapped, which means the free space there may need
fixup, so we unmap them and UBI will erase the corresponding PEB.


-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2/3] ubifs: add ubifs_mount_fixup_lebs()

[Matthew L. Creech]

All makes sense Artem, thanks for the feedback.

On Wed, May 4, 2011 at 7:13 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Tue, 2011-05-03 at 18:55 -0400, Matthew L. Creech wrote:
>> +
>> +             if (!(lprops->flags & LPROPS_EMPTY) &&
>
> I think this is not quite right, because empty taken LEBs will not be
> fixed up. I think you should only look at free space and nothing else.
>

OK, however I did find that I needed to check for (lprops->flags &
LPROPS_TAKEN) in addition to (lprops->free == c->leb_size) before
unmapping a LEB.  Failing to do so caused this:

UBIFS error (pid 1): check_lpt_type: invalid type (15) in LPT node type 1
UBIFS error (pid 1): ubifs_read_nnode: error -22 reading nnode at 8:783

so apparently it was unmapping part of the LPT, I guess?

-- 
Matthew L. Creech

Re: [PATCH 2/3] ubifs: add ubifs_mount_fixup_lebs()

[Artem Bityutskiy]

On Wed, 2011-05-04 at 18:02 -0400, Matthew L. Creech wrote:
> All makes sense Artem, thanks for the feedback.
> 
> On Wed, May 4, 2011 at 7:13 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Tue, 2011-05-03 at 18:55 -0400, Matthew L. Creech wrote:
> >> +
> >> +             if (!(lprops->flags & LPROPS_EMPTY) &&
> >
> > I think this is not quite right, because empty taken LEBs will not be
> > fixed up. I think you should only look at free space and nothing else.
> >
> 
> OK, however I did find that I needed to check for (lprops->flags &
> LPROPS_TAKEN) in addition to (lprops->free == c->leb_size) before
> unmapping a LEB.  Failing to do so caused this:
> 
> UBIFS error (pid 1): check_lpt_type: invalid type (15) in LPT node type 1
> UBIFS error (pid 1): ubifs_read_nnode: error -22 reading nnode at 8:783
> 
> so apparently it was unmapping part of the LPT, I guess?

Oh, right, the LPT subsystem contains the information about LEBs in the
main area, i.e., from LEBs number c->main_first to LEB number c->leb_cnt
- 1

The area which go _fefore_ the main are are special case.

The lprops area are LEBs from number c->lpt_first to number c->lpt_last.
For those you need to look at the ltab: 

c->ltab[lnum - c->lpt_first].free is what you need, where lnum is the
LEB number you need the amount of free space for.

For the orphans area [c->orph_first, c->orph_last] - you may to just
unmap them all, because when after mount this area does not contain any
useful data.

Master area - LEBs UBIFS_MST_LNUM and UBIFS_MST_LNUM + 1 - you know the
free space start at c->mst_offs + c->mst_node_alsz. So the amount of
used space there is c->mst_offs + c->mst_node_alsz and free space is
c->leb_size - c->mst_offs - c->mst_node_alsz; And it is the same for
both master area LEBs.

And finally the SB area (1 LEB) - you can skip this, but for
consistency, you can also fix it up. Amount of used space there is
always UBIFS_SB_NODE_SZ.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2/3] ubifs: add ubifs_mount_fixup_lebs()

[Matthew L. Creech]

On Thu, May 5, 2011 at 7:49 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>
> The area which go _fefore_ the main are are special case.
>
> The lprops area are LEBs from number c->lpt_first to number c->lpt_last.
> For those you need to look at the ltab:
>
> c->ltab[lnum - c->lpt_first].free is what you need, where lnum is the
> LEB number you need the amount of free space for.
>
> For the orphans area [c->orph_first, c->orph_last] - you may to just
> unmap them all, because when after mount this area does not contain any
> useful data.
>
> Master area - LEBs UBIFS_MST_LNUM and UBIFS_MST_LNUM + 1 - you know the
> free space start at c->mst_offs + c->mst_node_alsz. So the amount of
> used space there is c->mst_offs + c->mst_node_alsz and free space is
> c->leb_size - c->mst_offs - c->mst_node_alsz; And it is the same for
> both master area LEBs.
>
> And finally the SB area (1 LEB) - you can skip this, but for
> consistency, you can also fix it up. Amount of used space there is
> always UBIFS_SB_NODE_SZ.
>

OK, I've separated these out in the v3 patch.  I also ignored the log
LEBs, since you didn't mention those, and I wasn't sure how to handle
them - are they OK to leave alone?  Or could they cause similar
problems if not cleaned up?

-- 
Matthew L. Creech

Re: [PATCH 2/3] ubifs: add ubifs_mount_fixup_lebs()

[Artem Bityutskiy]

On Thu, 2011-05-05 at 16:36 -0400, Matthew L. Creech wrote:
> On Thu, May 5, 2011 at 7:49 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >
> > The area which go _fefore_ the main are are special case.
> >
> > The lprops area are LEBs from number c->lpt_first to number c->lpt_last.
> > For those you need to look at the ltab:
> >
> > c->ltab[lnum - c->lpt_first].free is what you need, where lnum is the
> > LEB number you need the amount of free space for.
> >
> > For the orphans area [c->orph_first, c->orph_last] - you may to just
> > unmap them all, because when after mount this area does not contain any
> > useful data.
> >
> > Master area - LEBs UBIFS_MST_LNUM and UBIFS_MST_LNUM + 1 - you know the
> > free space start at c->mst_offs + c->mst_node_alsz. So the amount of
> > used space there is c->mst_offs + c->mst_node_alsz and free space is
> > c->leb_size - c->mst_offs - c->mst_node_alsz; And it is the same for
> > both master area LEBs.
> >
> > And finally the SB area (1 LEB) - you can skip this, but for
> > consistency, you can also fix it up. Amount of used space there is
> > always UBIFS_SB_NODE_SZ.
> >
> 
> OK, I've separated these out in the v3 patch.  I also ignored the log
> LEBs, since you didn't mention those, and I wasn't sure how to handle
> them - are they OK to leave alone?  Or could they cause similar
> problems if not cleaned up?

Actually the log LEBs shoule be processed as well. I think:

1. all LEBs which are _not_ between the log tail and head should be
unmapped.
2. log head should be fixed up.

I'll send some code to you soon.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)