Message-ID: <1338038569.2525.21.camel@koala>
Date:	Sat, 26 May 2012 16:22:49 +0300
From:	Artem Bityutskiy <dedekind1@...il.com>
To:	Richard Weinberger <richard@....at>
Cc:	linux-mtd@...ts.infradead.org, tglx@...utronix.de,
	linux-kernel@...r.kernel.org, Heinz.Egger@...utronix.de,
	tim.bird@...sony.com
Subject: Re: [PATCH] [RFC] UBI: Implement Fastmap support

On Wed, 2012-05-23 at 13:06 +0200, Richard Weinberger wrote:
> Fastmap (aka checkpointing) allows attaching of an UBI volume in nearly
> constant time. Only a fixed number of PEBs has to be scanned.
> 
> Signed-off-by: Richard Weinberger <richard@....at>

Richard, Shmulik,

I've created 'fastmap' branch in the UBI tree. I've pushed a patch with
my TODO entries. It is also inlined in this e-mail at the end. I did not
have a lot of time to review this, but I will try to review a little bit
every day.

One thing which is apparent to me is that we have to change the way we
work - it needs improvements. The code drops are difficult to review.
And I noticed few basic errors which makes me believe the testing is not
very extensive.

So, I took an initiative to start hosting a branch where we can both
work on. Hopefully Shmulik too, may be others. I suggest you to send
incremental patches from now. They will go to that branch.

The other thing I strongly believe we must do is regression testing. We
need to start with a small set of simple tests. Then continuously make
it grow. And _every_ change has to be validated before it goes to the
fastmap branch. If something works - it must never be broken. This is
the only realistic way to quickly make fastmap production quality. The
open development in this mailing list hopefully will make more people
join.

So, I've also created a 'fastmap' branch in the mtd-utils.git repo. I've
added a 'fastmap-testing.txt' file there, empty so far. Will keep the
lists of tests we do there. Please, send patches against this branch.
Start with making fastmap pass the ubi teset from
mtd-utils.git/tests/ubi.

Thanks and hopefully this is acceptable for you.

P.S. Note, I do not intend to take over this stuff or anything like
that, I just try to involve myself a bit more, and re-use my experience
from UBIFS development where we use this "no regressions, ever growing
tests" approach. I think it was successful.

I also realize that some comments are nit-picks - we do not have to
address them immediately. We can preserve the TODOs in the code and
address later.

From bd8313d8eb6b00dc4dff3304c49bbd76cc49196a Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
Date: Sat, 26 May 2012 16:01:06 +0300
Subject: [PATCH] UBI: add a bunch of fastmap TODOs.

Well, it does not look like this code was tested well. We need to start
taking this more seriously.

We need to do the following:

1. Make sure this code passes ubi tests from mtd-utils.git
   a) with images which have fastmap
   b) with images which do not have fastmap
2. Once the they pass, make sure we never break them. Whatever we commit
   has to be tested.
3. Increasingly add more tests and make testing tougher.

I can host the tests in the mtd-utils.git repository in the 'fastmap' branch.

Please, also start sending incremental patches instead of patch-drops. This
will make review easier. I will put them to the 'fastmap' branch.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>
---
 TODO                        |   12 ++++++++++++
 drivers/mtd/ubi/attach.c    |   34 +++++++++++++++++++++++++++++++++-
 drivers/mtd/ubi/fastmap.c   |   36 ++++++++++++++++++++++++++++++++++++
 drivers/mtd/ubi/ubi-media.h |    4 ++++
 4 files changed, 85 insertions(+), 1 deletions(-)
 create mode 100644 TODO

diff --git a/TODO b/TODO
new file mode 100644
index 0000000..63a22b9
--- /dev/null
+++ b/TODO
@@ -0,0 +1,12 @@
+This is a TODO list for the fastmap feature which should be done before
+it is merged.
+
+For all or most of the items there we need to write a testcase. We can put it
+to the ubi-utils.git repository, to a separate branch at the beginning
+
+1. Test with a R/O MTD device: !(mtd->flags & MTD_WRITEABLE)
+2. Implement power-cut emulation infrastructure similar to what is in UBIFS and
+   test UBI + fastmap with it.
+3. Test the autoresize feature
+4. Test 'ubi_flush()'
+5. Test
diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 23b3d55..792808a 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1224,12 +1224,26 @@ int ubi_attach(struct ubi_device *ubi)
 	int err;
 	struct ubi_attach_info *ai = NULL;
 
+	/* TODO: Allocate ai in this fuction. And destroy it here as well */
+
 	err = ubi_scan_fastmap(ubi, &ai);
 	if (err > 0) {
+		/* TODO: in UBIFS we have a convention: every function prints
+		 * its own error messages. This makes things cleaner and easier
+		 * - the caller should not care about printing anything.
+		 * Please, move this error message to 'ubi_scan_fastmap()'. And
+		 * keep this in mind, and do similar thing globally for entire
+		 * fastmap code. */
 		if (err == UBI_BAD_FASTMAP)
-			ubi_err("Attach by fastmap failed! " \
+			ubi_err("Attach by fastmap failed! "
 				"Falling back to attach by scanning.");
 
+		/* TODO: please, remove this variable altogether, it is not
+		 * needed and it is a hack which you use to tell 'scan_peb()'
+		 * to handle fastmap volumes specially. Make this is a clean
+		 * way instead: after the scanning, go through the fastmap
+		 * volumes (if any was found) and delete them or do whatever
+		 * you need. Do not inject hacks to the scanning code. */
 		ubi->attached_by_scanning = 1;
 		ai = scan_all(ubi);
 		if (IS_ERR(ai))
@@ -1237,6 +1251,12 @@ int ubi_attach(struct ubi_device *ubi)
 	} else if (err < 0)
 		return err;
 
+	/* TODO: When you create an image with ubinize - you do not know the
+	 * amount of PEBs. So you need to initialize this field with '-1' at
+	 * ubinize time. And here you need to check for -1 and initialize it if
+	 * needed. Then store it at fastmap. This special value has to be also
+	 * documented at ubi-media.h. You also have to amend 'nused' etc.
+	 * Probably this can be done later. */
 	ubi->bad_peb_count = ai->bad_peb_count;
 	ubi->good_peb_count = ubi->peb_count - ubi->bad_peb_count;
 	ubi->corr_peb_count = ai->corr_peb_count;
@@ -1244,6 +1264,10 @@ int ubi_attach(struct ubi_device *ubi)
 	ubi->mean_ec = ai->mean_ec;
 	ubi_msg("max. sequence number:       %llu", ai->max_sqnum);
 
+	/* TODO: If you support fastmap but it was not found, you need to check
+	 * here that ai does not contain fastmap volumes. If it was corrupted,
+	 * you need to delete fastmap volumes or possible leftovers of them.
+	 * And then you have to create _new_ fastmap */
 	err = ubi_read_volume_table(ubi, ai);
 	if (err)
 		goto out_ai;
@@ -1257,6 +1281,14 @@ int ubi_attach(struct ubi_device *ubi)
 		goto out_wl;
 
 	ubi_destroy_ai(ai);
+
+	/* TODO: UBI auto formats the flash if it is empty (see ubi->is_empty).
+	 * It is currently done so that every sub-system writes initializes its
+	 * own stuff. Well, now it is only the vtbl sub-system - it creates
+	 * empty volume table. And this is why we have "early" function for
+	 * getting free PEBs. Fastmap should do the same - so I guess it is
+	 * good to do it somewhere here. Also, we need to re-create the fastmap
+	 * on-flash data-structures if they were corrupted. */
 	return 0;
 
 out_wl:
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 7757e5a..2ba2a80 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -595,6 +595,8 @@ fail:
  * ubi_find_fastmap - searches the first UBI_FM_MAX_START PEBs for the
  * fastmap super block.
  * @ubi: UBI device object
+ *
+ * TODO: clearly document return codes
  */
 static int ubi_find_fastmap(struct ubi_device *ubi, int *fm_start)
 {
@@ -614,6 +616,11 @@ static int ubi_find_fastmap(struct ubi_device *ubi, int *fm_start)
 
 		if (be32_to_cpu(vhdr->vol_id) == UBI_FM_SB_VOLUME_ID) {
 			*fm_start = i;
+			/* TODO: fix globally: all UBI prints:
+			 *  a) do not need the '\n' at the end
+			 *  b) start with a small letter, because the macros
+			 *     add several prefixes.
+			 */
 			dbg_bld("Found fastmap super block at PEB %i\n", i);
 			ret = 0;
 
@@ -623,6 +630,23 @@ static int ubi_find_fastmap(struct ubi_device *ubi, int *fm_start)
 
 	ubi_free_vid_hdr(ubi, vhdr);
 
+	/* TODO: we can return:
+	 * < 0 - error
+	 * 0 - fastmap found
+	 * 0 - also if we did not find fastmap and the last
+	 *     'ubi_io_read_vid_hdr()' call returned 0. This was not
+	 *     tested with volumes which do not contain fastmap? We need to
+	 *     test this - we need to have a testcase for this in mtd-utils. I
+	 *     can create a branch there and we should agree on which tests are
+	 *     run before anything goes into the git repo.
+	 * > 0 - ignore?
+	 *
+	 * Please, make it instead:
+	 * return 0 - no errors
+	 * return < 0 - error
+	 * If FM not found, fm_start = -1
+	 * If FM found, fm_start is set properly
+	 */
 	return ret;
 }
 
@@ -650,6 +674,13 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info **ai)
 
 		goto out;
 	}
+	/* TODO: then this will be:
+	 * if (ret)
+	 *         return ret;
+	 * if (sb_pnum == -1)
+	 *         return UBI_NO_FASTMAP;
+	 *
+	 * Much better, I think */
 
 	fmsb = kmalloc(sizeof(*fmsb), GFP_KERNEL);
 	if (!fmsb) {
@@ -661,6 +692,11 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info **ai)
 	ret = ubi_io_read(ubi, fmsb, sb_pnum, ubi->leb_start, sizeof(*fmsb));
 	if (ret) {
 		ubi_err("Unable to read fastmap super block");
+		/* TODO: please, read what 'ubi_io_read()' returns.
+		 * This code is incorrect. All return codes are carefully
+		 * documented there. And do it globally, I see you ignore the
+		 * bitflips error code and treat it as error everywhere.
+		 * Please, start testing with UBI bit-flips emulation enabled */
 		if (ret > 0)
 			ret = UBI_BAD_FASTMAP;
 
diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h
index 0e3019a..35628c3 100644
--- a/drivers/mtd/ubi/ubi-media.h
+++ b/drivers/mtd/ubi/ubi-media.h
@@ -389,6 +389,7 @@ struct ubi_vtbl_record {
 #define UBI_FM_POOL_MAGIC	0x67AF4D08
 #define UBI_FM_EBA_MAGIC	0xf0c040a8
 
+/* TODO: would be great to have short coments for the constants */
 #define UBI_FM_MAX_START	64
 #define UBI_FM_MAX_BLOCKS	32
 #define UBI_FM_MIN_POOL_SIZE	8
@@ -426,6 +427,9 @@ struct ubi_fm_sb {
  */
 struct ubi_fm_hdr {
 	__be32 magic;
+	/* TODO: would you please name these fields using the same names UBI
+	 * uses in the in-RAM data structures (bad_peb_count, good_peb_count,
+	 * etc.) See struct ubi_device. */
 	__be32 nfree;
 	__be32 nused;
 	__be32 nvol;
-- 
1.7.7.6

-- 
Best Regards,
Artem Bityutskiy

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists