From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from top.free-electrons.com ([176.31.233.9] helo=mail.free-electrons.com) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1WFMMv-0002ZI-Jp for linux-mtd@lists.infradead.org; Mon, 17 Feb 2014 11:28:15 +0000 Date: Mon, 17 Feb 2014 08:27:29 -0300 From: Ezequiel Garcia To: Artem Bityutskiy Subject: Re: [PATCH v6 1/3] ubi: Introduce block devices for UBI volumes Message-ID: <20140217112728.GB21782@localhost> References: <1392581041-8099-1-git-send-email-ezequiel.garcia@free-electrons.com> <1392581041-8099-2-git-send-email-ezequiel.garcia@free-electrons.com> <1392633946.12215.150.camel@sauron.fi.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1392633946.12215.150.camel@sauron.fi.intel.com> Cc: Thomas Petazzoni , Mike Frysinger , Richard Weinberger , Michael Opdenacker , linux-mtd@lists.infradead.org, Piergiorgio Beruto , Brian Norris , David Woodhouse , Willy Tarreau List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Mon, Feb 17, 2014 at 12:45:46PM +0200, Artem Bityutskiy wrote: > On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote: > > This commit introduces block device emulation on top of ubi volumes, > > in the most featureless and skinniest way: read-only, uncached access. > > > > Given UBI takes care of wear leveling and bad block management it's possible > > to add a thin layer to enable block device access to UBI volumes. > > This allows to use a block-oriented filesystem on a flash device. > > > > As opposed to mtdblock, which uses a 1-LEB cache, we've chosen not to > > implement any caching policy. The UBI block interface is meant to be > > used in conjunction with regular block-oriented filesystems (primarily, > > squashfs). These filesystems are better prepared to do their own caching, > > rendering a block-level cache useless. > > As I explain, I do not think any additional caching is needed for the > R/O FS. Mtdblock supports writing in the most straight-forward way > (read-modify-write entire LEB), and this is why it needs the 1LEB > "cache" - it allows avoiding the read part _sometimes_ - for sequential > writes. And only for sequential writes. > > Just forget that cache. > Agreed. > > +config MTD_UBI_BLOCK > > + bool "Block device access to UBI volumes" > > bool "Read-only block devices on top of UBI volumes" > OK. > > + default n > > + help > > + Since UBI already takes care of eraseblock wear leveling > > + and bad block handling, it's possible to implement a block > > + device on top of it and therefore mount regular filesystems > > + (i.e. not flash-oriented, as ext4). > > + In other words, this is a software flash translation layer. > > I'd re-write it differently. "Since .. it is possible to implement" > looks weird. It is already implemented bu this driver. > > The customer for this text is the end user, who does not know much about > UBI and what is possible. Just give him/her facts :-) Something more > like this. > > This option enables read-only UBI block devices support. UBI block > devices will be layered on top of UBI volumes, which means that the UBI > driver will transparently handle things like bad eraseblocks and > bit-flips. You can put any file-system in on top of UBI volumes in > read-only mode (e.g., ext4), but it is probably most practical for > read-only file systems like squashfs. > Looks much better. > > + > > + This can be particularly interesting to allow mounting a read-only > > + filesystem, such as squashfs, on a NAND device. > > "This can be particularly interesting" - may be just simpler phrase > which just tells that the user can use UBI block devices with squashfs? > OK. > > + When selected, this feature will be built-in the ubi module > > Well, I'd use "UBI driver" instead of "ubi module", since we do not know > whether the users selected the module or compiled UBI into the kernel. > OK. > > + and block devices will be able to be created and removed using > > + the userspace 'ubiblkvol' tool (provided mtd-utils). > > > @@ -0,0 +1,660 @@ > > +/* > > + * Copyright (c) 2014 Ezequiel Garcia > > + * Copyright (c) 2011 Free Electrons > > If you copy-pasted _any_ code from UBI, do not forget to add original > copyrights here too. Otherwise, this is fine. > Hm... I think I took something from UBI and mtdblock. Not sure if I "copy-pasted as-is", but I guess we can safely say it was "based on". Let's re-check the code and put a comment there. > > +/* > > + * Block interface for UBI volumes > > + * > > + * A simple implementation to allow a block device interface on top > > + * of a UBI volume. The implementation is provided by creating a static > > + * 1-to-1 mapping between the block device and the UBI volume. > > .. and the LEBs of the UBI volume, may be? > Could be.. but I don't want people to get confused by that. There's no 1-1 mapping between *a* block sector and *a* LEB. Maybe the "1-1 mapping" wording is what's confusing? > > + * The addressed byte is obtained from the addressed block sector, > > + * which is mapped linearly into the corresponding LEB: > > + * > > + * leb number = addressed byte / leb size > > + * > > + * The current implementation support only read operation. Write-support > > + * addition shouldn't be too hard and could be implemented in the future. > > + * It was suggested to implement this using the already existent BLKROSET > > + * block ioctl to turn it on and off, thus avoiding undesirable writes. > > I do not think it is not hard. Decent write operation may be hard. By > decent I mean something which would give you sane random write > performance. Your implementation would basically read-erase-write entire > LEB, with CRC calculation for the entire LEB data, right? > > So I'd say just "possible", rather "shouldn't be too hard" :-) > The proposed write support used ubi_leb_change() to write. It was based on your proposal, many years ago: http://lists.infradead.org/pipermail/linux-mtd/2008-January/020381.html I think it's better to drop any mention to the write support. > > + * Contrary to the MTD block interface implementation, there's no cache > > + * involved. The reasons behind this choice is that the filesystems > > + * that would be mounted on top of UBI blocks already provide caching and > > + * are able to do it more efficiently. > > I'd not mention MTD block here at all. And it's cache. It confuses more > than gives a clue. Just tell that you do not implement any caching for > the date in your driver. > OK. > > + * We may add a cache here, but it would be desirable to register a memory > > + * shrinker to match the requirements of memory-constrained platforms. > > You could provide your vision about a possible write implementation at > the end of this comment, and there you could put your thoughts about the > cache. Because the cache is only relevant for the write support, I > believe. > I think I'll just drop the cache and write notice. > > + * This feature is compiled in the UBI core, and adds a new 'block' > > + * parameter to allow early block interface creation. Runtime > > + * block attach/detach for UBI volumes is provided through two > > + * UBI ioctls: UBI_IOCVOLATTBLK and UBI_IOCVOLDETBLK. > > + */ > > I hope more people would help reviewing the code :-) > Ah, I just noticed I forgot to add: Tested-by: Willy Tarreau Reviewed-by: Richard Weinberger who tested and reviewed previous versions of the UBI patch, and I'm sure are reading this and will test and review this one ;-) -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com