From patchwork Sat May 26 13:22:49 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Artem Bityutskiy X-Patchwork-Id: 161481 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from merlin.infradead.org (merlin.infradead.org [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id AF914B6FA2 for ; Sat, 26 May 2012 23:24:21 +1000 (EST) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1SYGxc-0005VE-ST; Sat, 26 May 2012 13:23:12 +0000 Received: from mail-lpp01m010-f49.google.com ([209.85.215.49]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1SYGxX-0005Ut-BH for linux-mtd@lists.infradead.org; Sat, 26 May 2012 13:23:08 +0000 Received: by laap9 with SMTP id p9so1542997laa.36 for ; Sat, 26 May 2012 06:23:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:subject:from:to:cc:date:in-reply-to:references :content-type:x-mailer:mime-version; bh=FXl8cMlvYPztJxcBteFFa/8cg3mEtle5d2gh9mk3p7E=; b=ADqz9spd4bED6So/SJCcipB0LoaBlI96vWfVfqzCSQ3bjdtKa6s7svo9dLXZ7C1UrD TO+6HsWXBygpe617F345MreHPGQAXkzYVgQBgucTHuiouJqjMamq5CTF1NcDZc2/zX6g flt8I3O5MBARAhIEAxe+7T+JUZMSIZ7PcHIu56eCLVusvkzemtH5jkDztD7rLy7NiuB5 G8a9kN5G9H1tJoe9Sbryh2RAkclVCPzC2Hay9RbQqEisJEFhJx8zo8czS32ASWBFbcwK QXurB/hMuZN7SAeOb7i3FrsZxaqi84uWmlTb4Nm4IQLtea+J4a6Or40W/N2I+OComvN4 8+sQ== Received: by 10.112.85.39 with SMTP id e7mr1030816lbz.56.1338038584598; Sat, 26 May 2012 06:23:04 -0700 (PDT) Received: from [192.168.255.2] (host-94-101-1-70.igua.fi. [94.101.1.70]) by mx.google.com with ESMTPS id h9sm4825068lbi.9.2012.05.26.06.23.02 (version=SSLv3 cipher=OTHER); Sat, 26 May 2012 06:23:02 -0700 (PDT) Message-ID: <1338038569.2525.21.camel@koala> Subject: Re: [PATCH] [RFC] UBI: Implement Fastmap support From: Artem Bityutskiy To: Richard Weinberger Date: Sat, 26 May 2012 16:22:49 +0300 In-Reply-To: <1337771191-95358-2-git-send-email-richard@nod.at> References: <1337771191-95358-1-git-send-email-richard@nod.at> <1337771191-95358-2-git-send-email-richard@nod.at> X-Mailer: Evolution 3.2.3 (3.2.3-3.fc16) Mime-Version: 1.0 X-Spam-Note: CRM114 invocation failed X-Spam-Score: -2.5 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.5 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.215.49 listed in list.dnswl.org] 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (dedekind1[at]gmail.com) -0.0 SPF_PASS SPF: sender matches SPF record 0.2 FREEMAIL_ENVFROM_END_DIGIT Envelope-from freemail username ends in digit (dedekind1[at]gmail.com) -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: Heinz.Egger@linutronix.de, tglx@linutronix.de, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org, tim.bird@am.sony.com X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-mtd-bounces@lists.infradead.org Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org 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, 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 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 --- 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;