[PATCH v6 0/3] ubi: Add block interface

[PATCH v6 0/3] ubi: Add block interface

[Ezequiel Garcia]

Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Michael Opdenacker <michael.opdenacker@free-electrons.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
Cc: Willy Tarreau <w@1wt.eu>
Cc: Richard Weinberger <richard.weinberger@gmail.com>

This patch adds block device access on top of UBI volumes, which allows
for a ligthweigth solution to mount block-oriented filesystems on top of
flash devices.

The main target of this patch is supporting lighter, read-only filesystem,
by putting a squashfs on top of ubiblock. And since squashfs already provides
a file cache (which is even configurable), a first uncached implementation
it's good enough for now.

You can read this discussion if you are want more historical details:

  http://lkml.indiana.edu/hypermail/linux/kernel/1211.2/02556.html

In particular, it's important to note this already has users:

  1. http://permalink.gmane.org/gmane.linux.drivers.mtd/46221

  2. http://permalink.gmane.org/gmane.linux.drivers.mtd/49385

  3. Myself (Yes, I am now taking my own medicine).

The current implementation support only read operation. Write-support
addition shouldn't be too hard and could be implemented in the future.
Artem has suggested to implement this using the already existent BLKROSET
block ioctl to turn it on and off, thus avoiding undesirable writes.

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.

We may add a cache here, but it would be desirable to register a memory
shrinker to match the requirements of memory-constrained platforms.
As noted below, and despite being integrated into the UBI module,
the block layer is very losely coupled to the rest of the UBI code.
This is important as it should mean it has a low impact on UBI's stability.

These are great ideas [1] and I'll take a look at them as soon as I find
some more spare time. However, given the patch has been floating around
since quite some time and a already has a few users, I'd rather have it
merged sooner than later, at least with some minimal functionality.

How does this approach sound?

[1] ... so thanks for them! I know I've complained in the past about the
feedback received asking to remove the write and cache compile-time options.
However, the dynamic alternatives do look much smarter, so the feedback
is much appreciated.

* Changes from v5

  * Remove stupid comment about code aesthetic.

  * Rework comments inside the driver and added user documentation to the
    mtd website.

  * Included the mtd-utils patch, adding the ubiblkvol user-space tool.

* Changes from v4

  * Dropped write-support

  * Dropped cached access

* Changes from v3

  * Replaced the 2-LEB (read and write) for a 1-LEB cache. This was done
    to prevent any cache-coherency issues and to avoid over-design.
    It's not clear having a separate read and write cache has a dramatical
    impact on wear leveling or performance.

  * Made the 1-LEB cache optional. Based on a suggestion by Piergiorgio
    Beruto, caching at the block level would be redundant and ineffective.
    The filesystem has a better chance of caching.

    Some very simple tests on squashfs (ubiblock's primary target) showed
    that squashfs does a great job on caching its file accesses.

    Given a LEB can be quite big on some devices O(~100KiB), it can be
    interesting to remove the cache altogether on memory-constrained platforms.

  * Make UBI_IOCVOLDETBLK return an error if the detaching wasn't successful,
    as suggested by Mike Frysinger.

  * Fixed a bunch of typos and suggestions made by Mike Frysinger.

* Changes from v2

  * On this 3rd version of the patch I've re-worked the patch considerably,
    mostly integrating it with UBI, instead of making it a separate module.

    Despite this integration, the ubiblock code is still completely independent
    of the UBI core and it can be completely compiled-out through configuration.

  * Making ubiblock part of UBI allows to add ioctl access to attach/detach
    block devices to/from UBI volumes in a very simple way.

  * A new tool 'ubiblkvol' is added to ubi-utils to access these two new ioctls.

  * To address the 'write or not to write' discussion, the write support is
    enabled with a kernel option and there's a big fat 'DANGEROUS' label on it
    to prevent misusage.

Ezequiel Garcia (1):
  ubi: Introduce block devices for UBI volumes

 drivers/mtd/ubi/Kconfig     |  19 ++
 drivers/mtd/ubi/Makefile    |   1 +
 drivers/mtd/ubi/block.c     | 660 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/ubi/build.c     |   5 +
 drivers/mtd/ubi/cdev.c      |  20 ++
 drivers/mtd/ubi/ubi.h       |  14 +
 include/uapi/mtd/ubi-user.h |  11 +
 7 files changed, 730 insertions(+)
 create mode 100644 drivers/mtd/ubi/block.c

-- 
1.8.1.5

[PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Ezequiel Garcia]

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.

Write support has not been implemented. This might be added in the
future, using BLKROSET block ioctl to enable/disable it dynamically,
thus providing the means to protect the device from unwanted writes.

Block devices are created upon user request through new ioctls:
UBI_IOCVOLATTBLK to attach and UBI_IOCVOLDETBLK to detach.
Also, a new UBI module parameter is added 'ubi.block'. This parameter is
needed in order to attach a block device on boot-up time, allowing to
mount the rootfs on a ubiblock device.
For instance, you could have these kernel parameters:

  ubi.mtd=5 ubi.block=0,0 root=/dev/ubiblock0_0

Or, if you compile ubi as a module:

  $ modprobe ubi mtd=/dev/mtd5 block=/dev/ubi0_0

Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: Michael Opdenacker <michael.opdenacker@free-electrons.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
Cc: Willy Tarreau <w@1wt.eu>
Cc: Richard Weinberger <richard.weinberger@gmail.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/ubi/Kconfig     |  19 ++
 drivers/mtd/ubi/Makefile    |   1 +
 drivers/mtd/ubi/block.c     | 660 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/ubi/build.c     |   5 +
 drivers/mtd/ubi/cdev.c      |  20 ++
 drivers/mtd/ubi/ubi.h       |  14 +
 include/uapi/mtd/ubi-user.h |  11 +
 7 files changed, 730 insertions(+)
 create mode 100644 drivers/mtd/ubi/block.c

diff --git a/drivers/mtd/ubi/Kconfig b/drivers/mtd/ubi/Kconfig
index 36663af..4350bd5 100644
--- a/drivers/mtd/ubi/Kconfig
+++ b/drivers/mtd/ubi/Kconfig
@@ -87,4 +87,23 @@ config MTD_UBI_GLUEBI
 	   work on top of UBI. Do not enable this unless you use legacy
 	   software.
 
+config MTD_UBI_BLOCK
+	bool "Block device access to UBI volumes"
+	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.
+
+	   This can be particularly interesting to allow mounting a read-only
+	   filesystem, such as squashfs, on a NAND device.
+
+	   When selected, this feature will be built-in the ubi module
+	   and block devices will be able to be created and removed using
+	   the userspace 'ubiblkvol' tool (provided mtd-utils).
+
+	   If in doubt, say "N".
+
 endif # MTD_UBI
diff --git a/drivers/mtd/ubi/Makefile b/drivers/mtd/ubi/Makefile
index b46b0c97..4e3c3d7 100644
--- a/drivers/mtd/ubi/Makefile
+++ b/drivers/mtd/ubi/Makefile
@@ -3,5 +3,6 @@ obj-$(CONFIG_MTD_UBI) += ubi.o
 ubi-y += vtbl.o vmt.o upd.o build.o cdev.o kapi.o eba.o io.o wl.o attach.o
 ubi-y += misc.o debug.o
 ubi-$(CONFIG_MTD_UBI_FASTMAP) += fastmap.o
+ubi-$(CONFIG_MTD_UBI_BLOCK) += block.o
 
 obj-$(CONFIG_MTD_UBI_GLUEBI) += gluebi.o
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
new file mode 100644
index 0000000..02bb117
--- /dev/null
+++ b/drivers/mtd/ubi/block.c
@@ -0,0 +1,660 @@
+/*
+ * Copyright (c) 2014 Ezequiel Garcia
+ * Copyright (c) 2011 Free Electrons
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ */
+
+/*
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * We may add a cache here, but it would be desirable to register a memory
+ * shrinker to match the requirements of memory-constrained platforms.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+#include <linux/mtd/ubi.h>
+#include <linux/workqueue.h>
+#include <linux/blkdev.h>
+#include <linux/hdreg.h>
+
+#include "ubi-media.h"
+#include "ubi.h"
+
+/* Maximum number of supported devices */
+#define UBIBLOCK_MAX_DEVICES 32
+
+/* Maximum length of the 'block=' parameter */
+#define UBIBLOCK_PARAM_LEN 63
+
+/* Maximum number of comma-separated items in the 'block=' parameter */
+#define UBIBLOCK_PARAM_COUNT 2
+
+struct ubiblock_param {
+	int ubi_num;
+	int vol_id;
+	char name[UBIBLOCK_PARAM_LEN+1];
+};
+
+/* Numbers of elements set in the @ubiblock_param array */
+static int ubiblock_devs __initdata;
+
+/* MTD devices specification parameters */
+static struct ubiblock_param ubiblock_param[UBIBLOCK_MAX_DEVICES] __initdata;
+
+struct ubiblock {
+	struct ubi_volume_desc *desc;
+	struct ubi_volume_info *vi;
+	int ubi_num;
+	int vol_id;
+	int refcnt;
+	int leb_size;
+
+	struct gendisk *gd;
+	struct request_queue *rq;
+
+	struct workqueue_struct *wq;
+	struct work_struct work;
+
+	struct mutex vol_mutex;
+	spinlock_t queue_lock;
+	struct list_head list;
+};
+
+/* Linked list of all ubiblock instances */
+static LIST_HEAD(ubiblock_devices);
+static DEFINE_MUTEX(devices_mutex);
+static int ubiblock_major;
+
+static int __init ubiblock_set_param(const char *val,
+				     const struct kernel_param *kp)
+{
+	int i, ret;
+	size_t len;
+	struct ubiblock_param *param;
+	char buf[UBIBLOCK_PARAM_LEN];
+	char *pbuf = &buf[0];
+	char *tokens[UBIBLOCK_PARAM_COUNT];
+
+	if (!val)
+		return -EINVAL;
+
+	len = strnlen(val, UBIBLOCK_PARAM_LEN);
+	if (len == 0) {
+		ubi_warn("block: empty 'block=' parameter - ignored\n");
+		return 0;
+	}
+
+	if (len == UBIBLOCK_PARAM_LEN) {
+		ubi_err("block: parameter \"%s\" is too long, max. is %d\n",
+			val, UBIBLOCK_PARAM_LEN);
+		return -EINVAL;
+	}
+
+	strcpy(buf, val);
+
+	/* Get rid of the final newline */
+	if (buf[len - 1] == '\n')
+		buf[len - 1] = '\0';
+
+	for (i = 0; i < UBIBLOCK_PARAM_COUNT; i++)
+		tokens[i] = strsep(&pbuf, ",");
+
+	param = &ubiblock_param[ubiblock_devs];
+	if (tokens[1]) {
+		/* Two parameters: can be 'ubi, vol_id' or 'ubi, vol_name' */
+		ret = kstrtoint(tokens[0], 10, &param->ubi_num);
+		if (ret < 0)
+			return -EINVAL;
+
+		/* Second param can be a number or a name */
+		ret = kstrtoint(tokens[1], 10, &param->vol_id);
+		if (ret < 0) {
+			param->vol_id = -1;
+			strcpy(param->name, tokens[1]);
+		}
+
+	} else {
+		/* One parameter: must be device path */
+		strcpy(param->name, tokens[0]);
+		param->ubi_num = -1;
+		param->vol_id = -1;
+	}
+
+	ubiblock_devs++;
+
+	return 0;
+}
+
+static const struct kernel_param_ops ubiblock_param_ops = {
+	.set    = ubiblock_set_param,
+};
+module_param_cb(block, &ubiblock_param_ops, NULL, 0);
+MODULE_PARM_DESC(block, "Attach block devices to UBI volumes. Parameter format: block=<path|dev,num|dev,name>.\n"
+			"Multiple \"block\" parameters may be specified.\n"
+			"UBI volumes may be specified by their number, name, or path to the device node.\n"
+			"Examples\n"
+			"Using the UBI volume path:\n"
+			"ubi.block=/dev/ubi0_0\n"
+			"Using the UBI device, and the volume name:\n"
+			"ubi.block=0,rootfs\n"
+			"Using both UBI device number and UBI volume number:\n"
+			"ubi.block=0,0\n");
+
+static struct ubiblock *find_dev_nolock(int ubi_num, int vol_id)
+{
+	struct ubiblock *dev;
+
+	list_for_each_entry(dev, &ubiblock_devices, list)
+		if (dev->ubi_num == ubi_num && dev->vol_id == vol_id)
+			return dev;
+	return NULL;
+}
+
+static int ubiblock_read_to_buf(struct ubiblock *dev, char *buffer,
+				int leb, int offset, int len)
+{
+	int ret;
+
+	ret = ubi_read(dev->desc, leb, buffer, offset, len);
+	if (ret) {
+		ubi_err("%s ubi_read error %d",
+			dev->gd->disk_name, ret);
+		return ret;
+	}
+	return 0;
+}
+
+static int ubiblock_read(struct ubiblock *dev, char *buffer, int pos, int len)
+{
+	int leb, offset, ret;
+	int bytes_left = len;
+	int to_read = len;
+
+	/* Get leb:offset address to read from */
+	leb = pos / dev->leb_size;
+	offset = pos % dev->leb_size;
+
+	while (bytes_left) {
+
+		/*
+		 * We can only read one leb at a time.
+		 * Therefore if the read length is larger than
+		 * one leb size, we split the operation.
+		 */
+		if (offset + to_read > dev->leb_size)
+			to_read = dev->leb_size - offset;
+
+		ret = ubiblock_read_to_buf(dev, buffer, leb, offset, to_read);
+		if (ret)
+			return ret;
+
+		buffer += to_read;
+		bytes_left -= to_read;
+		to_read = bytes_left;
+		leb++;
+		offset = 0;
+	}
+	return 0;
+}
+
+static int do_ubiblock_request(struct ubiblock *dev, struct request *req)
+{
+	int pos, len, ret;
+
+	if (req->cmd_type != REQ_TYPE_FS)
+		return -EIO;
+
+	if (blk_rq_pos(req) + blk_rq_cur_sectors(req) >
+	    get_capacity(req->rq_disk))
+		return -EIO;
+
+	if (rq_data_dir(req) != READ)
+		return -ENOSYS; /* Write not implemented */
+
+	pos = blk_rq_pos(req) << 9;
+	len = blk_rq_cur_bytes(req);
+
+	/*
+	 * Let's prevent the device from being removed while we're doing I/O
+	 * work. Notice that this means we serialize all the I/O operations,
+	 * but it's probably of no impact given the NAND core serializes
+	 * flash acceses anywafy.
+	 */
+	mutex_lock(&dev->vol_mutex);
+	ret = ubiblock_read(dev, req->buffer, pos, len);
+	mutex_unlock(&dev->vol_mutex);
+
+	return ret;
+}
+
+static void ubiblock_do_work(struct work_struct *work)
+{
+	struct ubiblock *dev =
+		container_of(work, struct ubiblock, work);
+	struct request_queue *rq = dev->rq;
+	struct request *req;
+	int res;
+
+	spin_lock_irq(rq->queue_lock);
+
+	req = blk_fetch_request(rq);
+	while (req) {
+
+		spin_unlock_irq(rq->queue_lock);
+		res = do_ubiblock_request(dev, req);
+		spin_lock_irq(rq->queue_lock);
+
+		/*
+		 * If we're done with this request,
+		 * we need to fetch a new one
+		 */
+		if (!__blk_end_request_cur(req, res))
+			req = blk_fetch_request(rq);
+	}
+
+	spin_unlock_irq(rq->queue_lock);
+}
+
+static void ubiblock_request(struct request_queue *rq)
+{
+	struct ubiblock *dev;
+	struct request *req;
+
+	dev = rq->queuedata;
+
+	if (!dev)
+		while ((req = blk_fetch_request(rq)) != NULL)
+			__blk_end_request_all(req, -ENODEV);
+	else
+		queue_work(dev->wq, &dev->work);
+}
+
+static int ubiblock_open(struct block_device *bdev, fmode_t mode)
+{
+	struct ubiblock *dev = bdev->bd_disk->private_data;
+	int ret;
+
+	mutex_lock(&dev->vol_mutex);
+	if (dev->refcnt > 0) {
+		/*
+		 * The volume is already open, just increase the reference
+		 * counter.
+		 */
+		goto out_done;
+	}
+
+	/*
+	 * We want users to be aware they should only mount us as read-only.
+	 * It's just a paranoid check, as write requests will get rejected
+	 * in any case.
+	 */
+	if (mode & FMODE_WRITE) {
+		ret = -EPERM;
+		goto out_unlock;
+	}
+
+	dev->desc = ubi_open_volume(dev->ubi_num, dev->vol_id, UBI_READONLY);
+	if (IS_ERR(dev->desc)) {
+		ubi_err("%s failed to open ubi volume %d_%d",
+			dev->gd->disk_name, dev->ubi_num, dev->vol_id);
+		ret = PTR_ERR(dev->desc);
+		dev->desc = NULL;
+		goto out_unlock;
+	}
+
+	dev->vi = kzalloc(sizeof(struct ubi_volume_info), GFP_KERNEL);
+	if (!dev->vi) {
+		ret = -ENOMEM;
+		goto out_close;
+	}
+	ubi_get_volume_info(dev->desc, dev->vi);
+	dev->leb_size = dev->vi->usable_leb_size;
+
+out_done:
+	dev->refcnt++;
+	mutex_unlock(&dev->vol_mutex);
+	return 0;
+
+out_close:
+	ubi_close_volume(dev->desc);
+	dev->desc = NULL;
+out_unlock:
+	mutex_unlock(&dev->vol_mutex);
+	return ret;
+}
+
+static void ubiblock_release(struct gendisk *gd, fmode_t mode)
+{
+	struct ubiblock *dev = gd->private_data;
+
+	mutex_lock(&dev->vol_mutex);
+
+	dev->refcnt--;
+	if (dev->refcnt == 0) {
+		kfree(dev->vi);
+		ubi_close_volume(dev->desc);
+
+		dev->vi = NULL;
+		dev->desc = NULL;
+	}
+
+	mutex_unlock(&dev->vol_mutex);
+}
+
+static int ubiblock_getgeo(struct block_device *bdev, struct hd_geometry *geo)
+{
+	/* Some tools might require this information */
+	geo->heads = 1;
+	geo->cylinders = 1;
+	geo->sectors = get_capacity(bdev->bd_disk);
+	geo->start = 0;
+	return 0;
+}
+
+static const struct block_device_operations ubiblock_ops = {
+	.owner = THIS_MODULE,
+	.open = ubiblock_open,
+	.release = ubiblock_release,
+	.getgeo	= ubiblock_getgeo,
+};
+
+int ubiblock_add(struct ubi_volume_info *vi)
+{
+	struct ubiblock *dev;
+	struct gendisk *gd;
+	int disk_capacity;
+	int ret;
+
+	/* Check that the volume isn't already handled */
+	mutex_lock(&devices_mutex);
+	if (find_dev_nolock(vi->ubi_num, vi->vol_id)) {
+		mutex_unlock(&devices_mutex);
+		return -EEXIST;
+	}
+	mutex_unlock(&devices_mutex);
+
+	dev = kzalloc(sizeof(struct ubiblock), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	mutex_init(&dev->vol_mutex);
+
+	dev->ubi_num = vi->ubi_num;
+	dev->vol_id = vi->vol_id;
+
+	/* Initialize the gendisk of this ubiblock device */
+	gd = alloc_disk(1);
+	if (!gd) {
+		ubi_err("block: alloc_disk failed");
+		ret = -ENODEV;
+		goto out_free_dev;
+	}
+
+	gd->fops = &ubiblock_ops;
+	gd->major = ubiblock_major;
+	gd->first_minor = dev->ubi_num * UBI_MAX_VOLUMES + dev->vol_id;
+	gd->private_data = dev;
+	sprintf(gd->disk_name, "ubiblock%d_%d", dev->ubi_num, dev->vol_id);
+	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
+	set_capacity(gd, disk_capacity);
+	dev->gd = gd;
+
+	spin_lock_init(&dev->queue_lock);
+	dev->rq = blk_init_queue(ubiblock_request, &dev->queue_lock);
+	if (!dev->rq) {
+		ubi_err("block: blk_init_queue failed");
+		ret = -ENODEV;
+		goto out_put_disk;
+	}
+
+	dev->rq->queuedata = dev;
+	dev->gd->queue = dev->rq;
+
+	/*
+	 * Create one workqueue per volume (per registered block device).
+	 * Rembember workqueues are cheap, they're not threads.
+	 */
+	dev->wq = alloc_workqueue(gd->disk_name, 0, 0);
+	if (!dev->wq)
+		goto out_free_queue;
+	INIT_WORK(&dev->work, ubiblock_do_work);
+
+	mutex_lock(&devices_mutex);
+	list_add_tail(&dev->list, &ubiblock_devices);
+	mutex_unlock(&devices_mutex);
+
+	/* Must be the last step: anyone can call file ops from now on */
+	add_disk(dev->gd);
+
+	ubi_msg("%s created from ubi%d:%d(%s)",
+		dev->gd->disk_name, dev->ubi_num, dev->vol_id, vi->name);
+
+	return 0;
+
+out_free_queue:
+	blk_cleanup_queue(dev->rq);
+out_put_disk:
+	put_disk(dev->gd);
+out_free_dev:
+	kfree(dev);
+
+	return ret;
+}
+
+static void ubiblock_cleanup(struct ubiblock *dev)
+{
+	del_gendisk(dev->gd);
+	blk_cleanup_queue(dev->rq);
+	ubi_msg("%s released", dev->gd->disk_name);
+	put_disk(dev->gd);
+}
+
+int ubiblock_del(struct ubi_volume_info *vi)
+{
+	struct ubiblock *dev;
+
+	mutex_lock(&devices_mutex);
+	dev = find_dev_nolock(vi->ubi_num, vi->vol_id);
+	if (!dev) {
+		mutex_unlock(&devices_mutex);
+		return -ENODEV;
+	}
+
+	/* Found a device, let's lock it so we can check if it's busy */
+	mutex_lock(&dev->vol_mutex);
+
+	if (dev->refcnt > 0) {
+		mutex_unlock(&dev->vol_mutex);
+		mutex_unlock(&devices_mutex);
+		return -EBUSY;
+	}
+
+	/* Remove from device list */
+	list_del(&dev->list);
+	mutex_unlock(&devices_mutex);
+
+	/* Flush pending work and stop this workqueue */
+	destroy_workqueue(dev->wq);
+
+	ubiblock_cleanup(dev);
+	mutex_unlock(&dev->vol_mutex);
+	kfree(dev);
+	return 0;
+}
+
+static void ubiblock_resize(struct ubi_volume_info *vi)
+{
+	struct ubiblock *dev;
+	int disk_capacity;
+
+	/*
+	 * Need to lock the device list until we stop using the device,
+	 * otherwise the device struct might get released in ubiblock_del().
+	 */
+	mutex_lock(&devices_mutex);
+	dev = find_dev_nolock(vi->ubi_num, vi->vol_id);
+	if (!dev) {
+		mutex_unlock(&devices_mutex);
+		return;
+	}
+
+	mutex_lock(&dev->vol_mutex);
+	disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
+	set_capacity(dev->gd, disk_capacity);
+	ubi_msg("%s resized to %d LEBs", dev->gd->disk_name, vi->size);
+	mutex_unlock(&dev->vol_mutex);
+	mutex_unlock(&devices_mutex);
+}
+
+static int ubiblock_notify(struct notifier_block *nb,
+			 unsigned long notification_type, void *ns_ptr)
+{
+	struct ubi_notification *nt = ns_ptr;
+
+	switch (notification_type) {
+	case UBI_VOLUME_ADDED:
+		/*
+		 * We want to enforce explicit block device attaching for
+		 * volumes; so when a volume is added we do nothing.
+		 */
+		break;
+	case UBI_VOLUME_REMOVED:
+		ubiblock_del(&nt->vi);
+		break;
+	case UBI_VOLUME_RESIZED:
+		ubiblock_resize(&nt->vi);
+		break;
+	default:
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block ubiblock_notifier = {
+	.notifier_call = ubiblock_notify,
+};
+
+static struct ubi_volume_desc * __init
+open_volume_desc(const char *name, int ubi_num, int vol_id)
+{
+	if (ubi_num == -1)
+		/* No ubi num, name must be a vol device path */
+		return ubi_open_volume_path(name, UBI_READONLY);
+	else if (vol_id == -1)
+		/* No vol_id, must be vol_name */
+		return ubi_open_volume_nm(ubi_num, name, UBI_READONLY);
+	else
+		return ubi_open_volume(ubi_num, vol_id, UBI_READONLY);
+}
+
+static void __init ubiblock_add_from_param(void)
+{
+	int i, ret;
+	struct ubiblock_param *p;
+	struct ubi_volume_desc *desc;
+	struct ubi_volume_info vi;
+
+	for (i = 0; i < ubiblock_devs; i++) {
+		p = &ubiblock_param[i];
+
+		desc = open_volume_desc(p->name, p->ubi_num, p->vol_id);
+		if (IS_ERR(desc)) {
+			ubi_warn("block: can't open volume, err=%ld\n",
+				 PTR_ERR(desc));
+			continue;
+		}
+
+		ubi_get_volume_info(desc, &vi);
+		ret = ubiblock_add(&vi);
+		if (ret)
+			ubi_warn("block: can't add '%s' volume, err=%d\n",
+				 vi.name, ret);
+		ubi_close_volume(desc);
+	}
+}
+
+int __init ubiblock_init(void)
+{
+	int ret;
+
+	ubiblock_major = register_blkdev(0, "ubiblock");
+	if (ubiblock_major < 0)
+		return ubiblock_major;
+
+	/* Attach block devices from 'block=' module param */
+	ubiblock_add_from_param();
+
+	/*
+	 * Block devices needs to be attached to volumes explicitly
+	 * upon user request. So we ignore existing volumes.
+	 */
+	ret = ubi_register_volume_notifier(&ubiblock_notifier, 1);
+	if (ret < 0)
+		unregister_blkdev(ubiblock_major, "ubiblock");
+	return ret;
+}
+
+void __exit ubiblock_exit(void)
+{
+	struct ubiblock *next;
+	struct ubiblock *dev;
+
+	ubi_unregister_volume_notifier(&ubiblock_notifier);
+
+	list_for_each_entry_safe(dev, next, &ubiblock_devices, list) {
+
+		/* Flush pending work and stop workqueue */
+		destroy_workqueue(dev->wq);
+
+		/* The module is being forcefully removed */
+		WARN_ON(dev->desc);
+
+		/* Remove from device list */
+		list_del(&dev->list);
+
+		ubiblock_cleanup(dev);
+
+		kfree(dev);
+	}
+
+	unregister_blkdev(ubiblock_major, "ubiblock");
+}
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 57deae9..a775603 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1298,6 +1298,9 @@ static int __init ubi_init(void)
 		}
 	}
 
+	if (ubiblock_init() < 0)
+		ubi_warn("cannot init block device access");
+
 	return 0;
 
 out_detach:
@@ -1326,6 +1329,8 @@ static void __exit ubi_exit(void)
 {
 	int i;
 
+	ubiblock_exit();
+
 	for (i = 0; i < UBI_MAX_DEVICES; i++)
 		if (ubi_devices[i]) {
 			mutex_lock(&ubi_devices_mutex);
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 8ca49f2..39d3774 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -561,6 +561,26 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
 		break;
 	}
 
+	/* Attach a block device to an UBI volume */
+	case UBI_IOCVOLATTBLK:
+	{
+		struct ubi_volume_info vi;
+
+		ubi_get_volume_info(desc, &vi);
+		err = ubiblock_add(&vi);
+		break;
+	}
+
+	/* Dettach a block device from an UBI volume */
+	case UBI_IOCVOLDETBLK:
+	{
+		struct ubi_volume_info vi;
+
+		ubi_get_volume_info(desc, &vi);
+		err = ubiblock_del(&vi);
+		break;
+	}
+
 	default:
 		err = -ENOTTY;
 		break;
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 8ea6297..e76ff98 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -864,6 +864,20 @@ int ubi_update_fastmap(struct ubi_device *ubi);
 int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		     int fm_anchor);
 
+/* block.c */
+#ifdef CONFIG_MTD_UBI_BLOCK
+int ubiblock_init(void);
+void ubiblock_exit(void);
+int ubiblock_add(struct ubi_volume_info *vi);
+int ubiblock_del(struct ubi_volume_info *vi);
+#else
+static inline int ubiblock_init(void) { return 0; }
+static inline void ubiblock_exit(void) {}
+static inline int ubiblock_add(struct ubi_volume_info *vi) { return -ENOTTY; }
+static inline int ubiblock_del(struct ubi_volume_info *vi) { return -ENOTTY; }
+#endif
+
+
 /*
  * ubi_rb_for_each_entry - walk an RB-tree.
  * @rb: a pointer to type 'struct rb_node' to use as a loop counter
diff --git a/include/uapi/mtd/ubi-user.h b/include/uapi/mtd/ubi-user.h
index 723c324..1080762 100644
--- a/include/uapi/mtd/ubi-user.h
+++ b/include/uapi/mtd/ubi-user.h
@@ -134,6 +134,13 @@
  * used. A pointer to a &struct ubi_set_vol_prop_req object is expected to be
  * passed. The object describes which property should be set, and to which value
  * it should be set.
+ *
+ * Block device access to UBI volumes
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * To attach or detach a block device from an UBI volume the %UBI_IOCVOLATTBLK
+ * and %UBI_IOCVOLDETBLK ioctl commands should be used, respectively.
+ * These commands take no arguments.
  */
 
 /*
@@ -191,6 +198,10 @@
 /* Set an UBI volume property */
 #define UBI_IOCSETVOLPROP _IOW(UBI_VOL_IOC_MAGIC, 6, \
 			       struct ubi_set_vol_prop_req)
+/* Attach a block device to an UBI volume */
+#define UBI_IOCVOLATTBLK _IO(UBI_VOL_IOC_MAGIC, 7)
+/* Detach a block device from an UBI volume */
+#define UBI_IOCVOLDETBLK _IO(UBI_VOL_IOC_MAGIC, 8)
 
 /* Maximum MTD device name length supported by UBI */
 #define MAX_UBI_MTD_NAME_LEN 127
-- 
1.8.1.5

[PATCH v6 2/3] ubi-utils: Add ubiblkvol tool

[Ezequiel Garcia]

With the addition of block device access to UBI volumes, we now
add a simple userspace tool to access the new ioctls.

Since the ioctls have no arguments, usage of this tool is as simple
as it gets:

  $ ubiblkvol --attach /dev/ubi0_0

will make a new device /dev/ubiblock0_0 available, and

  $ ubiblkvol --detach /dev/ubi0_0

will remove the device.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
Applies on mtd-utils git repo.

 Makefile                   |   2 +-
 include/mtd/ubi-user.h     |   4 ++
 ubi-utils/.gitignore       |   1 +
 ubi-utils/include/libubi.h |  16 +++++
 ubi-utils/libubi.c         |  10 +++
 ubi-utils/ubiblkvol.c      | 176 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 208 insertions(+), 1 deletion(-)
 create mode 100644 ubi-utils/ubiblkvol.c

diff --git a/Makefile b/Makefile
index 4ff8a49..52a35bc 100644
--- a/Makefile
+++ b/Makefile
@@ -28,7 +28,7 @@ MTD_BINS = \
 	sumtool jffs2reader
 UBI_BINS = \
 	ubiupdatevol ubimkvol ubirmvol ubicrc32 ubinfo ubiattach \
-	ubidetach ubinize ubiformat ubirename mtdinfo ubirsvol
+	ubidetach ubinize ubiformat ubirename mtdinfo ubirsvol ubiblkvol
 
 BINS = $(MTD_BINS)
 BINS += mkfs.ubifs/mkfs.ubifs
diff --git a/include/mtd/ubi-user.h b/include/mtd/ubi-user.h
index 1c06d88..1889249 100644
--- a/include/mtd/ubi-user.h
+++ b/include/mtd/ubi-user.h
@@ -186,6 +186,10 @@
 /* Set an UBI volume property */
 #define UBI_IOCSETVOLPROP _IOW(UBI_VOL_IOC_MAGIC, 6, \
 			       struct ubi_set_vol_prop_req)
+/* Attach a block device to an UBI volume */
+#define UBI_IOCVOLATTBLK _IO(UBI_VOL_IOC_MAGIC, 7)
+/* Detach a block device from an UBI volume */
+#define UBI_IOCVOLDETBLK _IO(UBI_VOL_IOC_MAGIC, 8)
 
 /* Maximum MTD device name length supported by UBI */
 #define MAX_UBI_MTD_NAME_LEN 127
diff --git a/ubi-utils/.gitignore b/ubi-utils/.gitignore
index c035c97..5c9cbd9 100644
--- a/ubi-utils/.gitignore
+++ b/ubi-utils/.gitignore
@@ -9,4 +9,5 @@
 /ubirmvol
 /ubiupdatevol
 /ubirsvol
+/ubiblkvol
 /mtdinfo
diff --git a/ubi-utils/include/libubi.h b/ubi-utils/include/libubi.h
index 47f40e2..3759aa8 100644
--- a/ubi-utils/include/libubi.h
+++ b/ubi-utils/include/libubi.h
@@ -400,6 +400,22 @@ int ubi_get_vol_info1_nm(libubi_t desc, int dev_num, const char *name,
 			 struct ubi_vol_info *info);
 
 /**
+ * ubi_vol_block_add - attach a block device to an UBI volume.
+ * @fd: volume character device file descriptor
+ *
+ * Returns %0 in case of success and %-1 in case of failure.
+ */
+int ubi_vol_block_add(int fd);
+
+/**
+ * ubi_vol_block_del - detach a block device from an UBI volume.
+ * @fd: volume character device file descriptor
+ *
+ * Returns %0 in case of success and %-1 in case of failure.
+ */
+int ubi_vol_block_del(int fd);
+
+/**
  * ubi_update_start - start UBI volume update.
  * @desc: UBI library descriptor
  * @fd: volume character device file descriptor
diff --git a/ubi-utils/libubi.c b/ubi-utils/libubi.c
index a7463e8..cb631f9 100644
--- a/ubi-utils/libubi.c
+++ b/ubi-utils/libubi.c
@@ -1109,6 +1109,16 @@ int ubi_rsvol(libubi_t desc, const char *node, int vol_id, long long bytes)
 	return ret;
 }
 
+int ubi_vol_block_add(int fd)
+{
+	return ioctl(fd, UBI_IOCVOLATTBLK);
+}
+
+int ubi_vol_block_del(int fd)
+{
+	return ioctl(fd, UBI_IOCVOLDETBLK);
+}
+
 int ubi_update_start(libubi_t desc, int fd, long long bytes)
 {
 	desc = desc;
diff --git a/ubi-utils/ubiblkvol.c b/ubi-utils/ubiblkvol.c
new file mode 100644
index 0000000..4d023cc
--- /dev/null
+++ b/ubi-utils/ubiblkvol.c
@@ -0,0 +1,176 @@
+/*
+ * ubiblkvol:
+ *   An utility to attach block devices to UBI volumes.
+ *
+ * Copyright (c) Ezequiel Garcia, 2013
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ * Usage
+ * -----
+ * Since the ioctls have no arguments, usage of this tool is as simple
+ * as it gets:
+ *
+ *   $ ubiblkvol --attach /dev/ubi0_0
+ *
+ * will make a new device /dev/ubiblock0_0 available, and
+ *
+ *   $ ubiblkvol --detach /dev/ubi0_0
+ *
+ * will remove the device.
+ */
+
+#define PROGRAM_NAME    "ubiblkvol"
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <getopt.h>
+#include <stdarg.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+
+#include <libubi.h>
+#include "common.h"
+
+struct args {
+	const char *node;
+	int attach;
+};
+
+static struct args args;
+
+static const char doc[] = PROGRAM_NAME " version " VERSION
+			 " - a tool to add/remove block device interface from UBI volumes.";
+
+static const char optionsstr[] =
+"-a, --attach               attach block to volume\n"
+"-d, --detach               detach block from volume\n"
+"-h, --help                 print help message\n"
+"-V, --version              print program version";
+
+static const char usage[] =
+"Usage: " PROGRAM_NAME " [-a,-d] <UBI volume node file name>\n"
+"Example: " PROGRAM_NAME " --attach /dev/ubi0_0";
+
+static const struct option long_options[] = {
+	{ .name = "attach",   .has_arg = 1, .flag = NULL, .val = 'a' },
+	{ .name = "detach",   .has_arg = 1, .flag = NULL, .val = 'd' },
+	{ .name = "help",     .has_arg = 0, .flag = NULL, .val = 'h' },
+	{ .name = "version",  .has_arg = 0, .flag = NULL, .val = 'V' },
+	{ NULL, 0, NULL, 0}
+};
+
+static int parse_opt(int argc, char * const argv[])
+{
+	while (1) {
+		int key;
+
+		key = getopt_long(argc, argv, "a:d:h?V", long_options, NULL);
+		if (key == -1)
+			break;
+
+		switch (key) {
+		case 'a':
+			args.attach = 1;
+		case 'd':
+			args.node = optarg;
+			break;
+		case 'h':
+		case '?':
+			printf("%s\n\n", doc);
+			printf("%s\n\n", usage);
+			printf("%s\n", optionsstr);
+			exit(EXIT_SUCCESS);
+
+		case 'V':
+			common_print_version();
+			exit(EXIT_SUCCESS);
+
+		default:
+			fprintf(stderr, "Use -h for help\n");
+			return -1;
+		}
+	}
+
+	if (!args.node)
+		return errmsg("invalid arguments (use -h for help)");
+
+	return 0;
+}
+
+int main(int argc, char * const argv[])
+{
+	int err, fd;
+	libubi_t libubi;
+
+	err = parse_opt(argc, argv);
+	if (err)
+		return -1;
+
+	libubi = libubi_open();
+	if (!libubi) {
+		if (errno == 0)
+			errmsg("UBI is not present in the system");
+		else
+			sys_errmsg("cannot open libubi");
+		goto out_libubi;
+	}
+
+	err = ubi_probe_node(libubi, args.node);
+	if (err == 1) {
+		errmsg("\"%s\" is an UBI device node, not an UBI volume node",
+		       args.node);
+		goto out_libubi;
+	} else if (err < 0) {
+		if (errno == ENODEV)
+			errmsg("\"%s\" is not an UBI volume node", args.node);
+		else
+			sys_errmsg("error while probing \"%s\"", args.node);
+		goto out_libubi;
+	}
+
+	fd = open(args.node, O_RDWR);
+	if (fd == -1) {
+		sys_errmsg("cannot open UBI volume \"%s\"", args.node);
+		goto out_libubi;
+	}
+
+	if (args.attach) {
+		err = ubi_vol_block_add(fd);
+		if (err) {
+			sys_errmsg("cannot attach block device");
+			goto out_close;
+		}
+	} else {
+		err = ubi_vol_block_del(fd);
+		if (err) {
+			sys_errmsg("cannot detach block device");
+			goto out_close;
+		}
+	}
+
+	close(fd);
+	libubi_close(libubi);
+	return 0;
+
+out_close:
+	close(fd);
+out_libubi:
+	libubi_close(libubi);
+	return -1;
+}
-- 
1.8.1.5

[PATCH v6 3/3] UBI: Add block interface documentation

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
Applies on top of mtd-www git repo.

 doc/general.xml |  4 ++++
 doc/ubi.xml     | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 faq/ubi.xml     | 16 +++++++-------
 3 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/doc/general.xml b/doc/general.xml
index 31ea243..d5af815 100644
--- a/doc/general.xml
+++ b/doc/general.xml
@@ -182,6 +182,10 @@ use block-based file systems on top of bare flashes using
 In other words, please, <b>do not use</b> <code>mtdblock</code> unless you
 know exactly what you are doing.</p>
 
+<p>Instead, you may find the block interface provided by <code>UBI</code>
+useful. Please refer to the <a href="ubi.html#L_block">UBI section</a> for more
+details.</p>
+
 <p>There is also a read-only version of this driver which doesn't have the
 capacity to do the caching and erase/writeback, mainly for use with uCLinux
 where the extra RAM requirement was considered too large.</p>
diff --git a/doc/ubi.xml b/doc/ubi.xml
index f55c04a..dd06927 100644
--- a/doc/ubi.xml
+++ b/doc/ubi.xml
@@ -47,6 +47,7 @@
 		</ol>
 	</li>
 	<li><a href="ubi.html#L_fastmap">Fastmap</a></li>
+	<li><a href="ubi.html#L_block">Block interface</a></li>
 	<li><a href="ubi.html#L_ubidoc">More documentation</a></li>
 </ol>
 
@@ -173,6 +174,11 @@ because:</p>
 	mechanisms).</li>
 </ul>
 
+<p>UBI also provides a block interface that allows regular, block-oriented
+filesystems to be mounted on top of an UBI volume. This is possible because UBI
+handles bad-blocks transparently. This block interface does not peform any
+wear-leveling, and therefore write support hasn't been implemented.</p>
+
 <p>There is an additional driver called <code>gluebi</code> which emulates MTD
 devices on top of UBI volumes. This looks a little strange, because UBI works
 on top of an MTD device, then <code>gluebi</code> emulates other MTD devices
@@ -236,6 +242,8 @@ sub-directory.</p>
 	devices (the opposite to what <code>ubiattach</code> does);</li>
 	<li><code>ubimkvol</code> - creates UBI volumes on UBI devices;</li>
 	<li><code>ubirmvol</code> - removes UBI volumes from UBI devices;</li>
+	<li><code>ubiblkvol</code> - manages block interfaces for UBI volumes.
+	See <a href="ubi.html#L_block">here</a> for more information;</li>
 	<li><code>ubiupdatevol</code> - updates UBI volumes; this tool uses the
 	<a href="ubi.html#L_volupdate">UBI volume update feature</a> which
 	leaves the volume in "corrupted" state if the update was interrupted;
@@ -1261,6 +1269,65 @@ second. Therefore, fastmap makes only sense on fast and large flash devices
 where a full scan takes too long. E.g. On 4GiB NAND chips a full scan takes
 several seconds whereas a fast attach needs less than one second.</p>
 
+<h2><a name="L_block">Block interface</a></h2>
+
+<p>UBI allows to create a block device on top of each UBI volume. The block
+interface implementation currently has the following limitations:
+
+<ul>
+	<li>Read-only operation.</li>
+	<li>1-to-1 mapping between block sectors and logical eraseblocks.
+	In other words, no wear-leveling is provided.</li>
+	<li>Serialized and uncached operation.</li>
+</ul></p>
+
+<p>Write-support is still under discussion and might be implemented in a
+near future.</p>
+
+<p>Regarding the serialized, uncached operation, keep in mind the NAND driver core
+already serializes all I/O anyway. In addition, filesystems usually provides
+a much better caching. For these reasons, it's expected that these limitations
+won't impact performance heavily.</p>
+
+<p>Despite these limitations, a block interface is still very useful to mount
+read-only, regular filesystems on top of UBI volumes. This is the case
+of squashfs, which can be used as a lightweigth read-only rootfs on a NAND
+device.</p>
+
+<h4><a name="L_block_usage">Usage</a></h4>
+<p>Attaching and detaching a block interface on a UBI volume is fairly easy.
+You can do this either by using the <code>block</code> UBI module parameter
+or with the <code>ubiblkvol</code> user-space tool.</p>
+
+<p>In order to attach a block device on bootup time (e.g. to mount the rootfs
+on such a block device) you can specify the <code>block</code> parameter as
+a kernel boot arguments:</p>
+
+<p><code>ubi.mtd=5 ubi.block=0,0 root=/dev/ubiblock0_0</code></p>
+
+<p>There are several ways if specifying a volume:</p>
+<ul>
+	<li><p>Using the UBI volume path:</p>
+	<code>ubi.block=/dev/ubi0_0</code></li>
+
+	<li><p>Using the UBI device, and the volume name:</p>
+	<code>ubi.block=0,rootfs</code></li>
+
+	<li><p>Using both UBI device number and UBI volume number:</p>
+	<code>ubi.block=0,0</code></li>
+</ul>
+
+<p>If you've built UBI as a module you can use this parameter at module insertion
+time:</p>
+
+<code>$ modprobe ubi mtd=/dev/mtd5 block=/dev/ubi0_0</code>
+
+<p>A block interface can also be attached/detached dynamically at runtime, using
+the <code>ubiblkvol</code> user-space tool:</p>
+
+<p><code>$ ubiblkvol --attach /dev/ubi0_0</code></p>
+<p><code>$ ubiblkvol --detach /dev/ubi0_0</code></p>
+
 <h2><a name="L_ubidoc">More documentation</a></h2>
 
 <p>Unfortunately, there are no thorough and strict UBI documents. But there is
diff --git a/faq/ubi.xml b/faq/ubi.xml
index 2832928..69f5495 100644
--- a/faq/ubi.xml
+++ b/faq/ubi.xml
@@ -18,6 +18,7 @@
 	<li><a href="ubi.html#L_mkvol">How do I create/delete UBI volumes?</a></li>
 	<li><a href="ubi.html#L_run_jffs2">How do I run JFFS2 on top of an UBI volume?</a></li>
 	<li><a href="ubi.html#L_ext2_over_ubi">Can I run ext2 on top of UBI?</a></li>
+	<li><a href="ubi.html#L_squashfs_over_ubi">Can I run squashfs on top of UBI?</a></li>
 	<li><a href="ubi.html#L_format_mtd">Do I have to format my empty flash before running UBI on top of it?</a></li>
 	<li><a href="ubi.html#L_ubierase">How do I erase flash and preserve erase counters?</a></li>
 	<li><a href="ubi.html#L_ubi_mkimg">How do I create UBI images?</a></li>
@@ -159,16 +160,15 @@ Please, read the <a href="../doc/ubi.html#L_rednote">big red note</a>
 and <a href="../doc/ubi.html#L_overview">overview</a> documentation sections to
 realize why.</p>
 
-<p>But it is much easier to implement FTL on top of UBI than on top of MTD,
-because UBI takes care about many flash complexities and makes it
-possible to concentrate on on upper-level issues rather then solving flash
-media problems like wear-leveling, bit-flips, bad eraseblocks, etc.</p>
-
-<p><a
-href="http://lists.infradead.org/pipermail/linux-mtd/2008-January/020381.html">
-This</a> e-mail describes an idea of a simple FTL layer on top of UBI.</p>
+<p>However, given UBI takes care of many flash complexities, it provides a
+bad-block-free block interface on top of UBI volumes. This feature is useful to
+mount read-only filesystems.</p>
 
+<h2><a name="L_squashfs_over_ubi">Can I run squashfs on top of UBI?</a></h2>
 
+<p>Yes. UBI allows to create a read-only block interface on top of a UBI volume
+which is suitable for read-only block-oriented filesystems, such as squashfs.
+See the <a href="..doc/ubi.html#L_block">UBI block interface</a> section for more details.</p>
 
 <h2><a name="L_format_mtd">
 	Do I have to format my empty flash before running UBI on top of it?
-- 
1.8.1.5

Re: [PATCH v6 0/3] ubi: Add block interface

[Artem Bityutskiy]

Thanks!

On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote:
> The main target of this patch is supporting lighter, read-only
> filesystem,
> by putting a squashfs on top of ubiblock. And since squashfs already
> provides
> a file cache (which is even configurable), a first uncached
> implementation
> it's good enough for now.

Doesn't Linux provide a fake inode for _every_ block device, and uses
page-cache to cache pages?

There is even entire special-purpose file-system for managing these fake
block device inodes, see fs/block_dev.c

static struct file_system_type bd_type = {
        .name           = "bdev",
        .mount          = bd_mount,
        .kill_sb        = kill_anon_super,
};

I am really not sure which other cache do we need. The only thing coming
to my mind is about emulating a HW disk cache in software. But I would
not go that far without a really good reason.

If what I say is correct, I guess we can just drop the "cache" subject
completely now.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Artem Bityutskiy]

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.

If we were adding a write support, we'd need a smarter cache, which
would probably cache multiple LEBs, and had a shrinker, and you'd need
to support the barrier operation, and so on. But there is not write
support at all now, let's not even talk about the cache.

Let me try to mostly review user-visible parts :=) Some nit-picks below.

> +config MTD_UBI_BLOCK
> +	bool "Block device access to UBI volumes"

bool "Read-only block devices on top of UBI volumes"

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

Feel free to rewrite it, I just wanted to give an idea what I suggest.

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

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

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

> +/*
> + * 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?

> + * 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" :-)

> + * 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.

> + * 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.

> + * 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 :-)

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v6 0/3] ubi: Add block interface

[Ezequiel Garcia]

On Mon, Feb 17, 2014 at 12:19:52PM +0200, Artem Bityutskiy wrote:
> Thanks!
> 
> On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote:
> > The main target of this patch is supporting lighter, read-only
> > filesystem,
> > by putting a squashfs on top of ubiblock. And since squashfs already
> > provides
> > a file cache (which is even configurable), a first uncached
> > implementation
> > it's good enough for now.
> 
> Doesn't Linux provide a fake inode for _every_ block device, and uses
> page-cache to cache pages?
> 
> There is even entire special-purpose file-system for managing these fake
> block device inodes, see fs/block_dev.c
> 
> static struct file_system_type bd_type = {
>         .name           = "bdev",
>         .mount          = bd_mount,
>         .kill_sb        = kill_anon_super,
> };
> 
> I am really not sure which other cache do we need. The only thing coming
> to my mind is about emulating a HW disk cache in software. But I would
> not go that far without a really good reason.
> 

If we ever re-add the write support, it would be natural to implement it
using a 1-LEB cache. Block sectors are much much smaller than LEB's
(around three orders of magnitude), so we would want to read an entire
LEB into a 1-LEB buffer, update the buffer, and then write it back.

In this scheme, it's almost straightforward to keep this buffer around
as a cache, and only write-back to disk when the new read/write requests
asks for a different LEB.

IIRC, this is the main reason why we had the 1-LEB cache.

Does it sound sane?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v6 0/3] ubi: Add block interface

[Artem Bityutskiy]

On Mon, 2014-02-17 at 07:48 -0300, Ezequiel Garcia wrote:
> On Mon, Feb 17, 2014 at 12:19:52PM +0200, Artem Bityutskiy wrote:
> > Thanks!
> > 
> > On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote:
> > > The main target of this patch is supporting lighter, read-only
> > > filesystem,
> > > by putting a squashfs on top of ubiblock. And since squashfs already
> > > provides
> > > a file cache (which is even configurable), a first uncached
> > > implementation
> > > it's good enough for now.
> > 
> > Doesn't Linux provide a fake inode for _every_ block device, and uses
> > page-cache to cache pages?
> > 
> > There is even entire special-purpose file-system for managing these fake
> > block device inodes, see fs/block_dev.c
> > 
> > static struct file_system_type bd_type = {
> >         .name           = "bdev",
> >         .mount          = bd_mount,
> >         .kill_sb        = kill_anon_super,
> > };
> > 
> > I am really not sure which other cache do we need. The only thing coming
> > to my mind is about emulating a HW disk cache in software. But I would
> > not go that far without a really good reason.
> > 
> 
> If we ever re-add the write support, it would be natural to implement it
> using a 1-LEB cache. Block sectors are much much smaller than LEB's
> (around three orders of magnitude), so we would want to read an entire
> LEB into a 1-LEB buffer, update the buffer, and then write it back.
> 
> In this scheme, it's almost straightforward to keep this buffer around
> as a cache, and only write-back to disk when the new read/write requests
> asks for a different LEB.
> 
> IIRC, this is the main reason why we had the 1-LEB cache.

May be. But you are submitting R/O driver. And yet talk about caches and
write support in several places. Caches are only relevant to write
support. Write support is absent. Let's focus on what is present.

Feel free to put all the thoughts about a possible write support
implementation to a single big comment in the code. That's fine. I do
not object. I just want to focus on what is present.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v6 0/3] ubi: Add block interface

[Ezequiel Garcia]

On Mon, Feb 17, 2014 at 12:53:40PM +0200, Artem Bityutskiy wrote:
> On Mon, 2014-02-17 at 07:48 -0300, Ezequiel Garcia wrote:
> > On Mon, Feb 17, 2014 at 12:19:52PM +0200, Artem Bityutskiy wrote:
> > > Thanks!
> > > 
> > > On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote:
> > > > The main target of this patch is supporting lighter, read-only
> > > > filesystem,
> > > > by putting a squashfs on top of ubiblock. And since squashfs already
> > > > provides
> > > > a file cache (which is even configurable), a first uncached
> > > > implementation
> > > > it's good enough for now.
> > > 
> > > Doesn't Linux provide a fake inode for _every_ block device, and uses
> > > page-cache to cache pages?
> > > 
> > > There is even entire special-purpose file-system for managing these fake
> > > block device inodes, see fs/block_dev.c
> > > 
> > > static struct file_system_type bd_type = {
> > >         .name           = "bdev",
> > >         .mount          = bd_mount,
> > >         .kill_sb        = kill_anon_super,
> > > };
> > > 
> > > I am really not sure which other cache do we need. The only thing coming
> > > to my mind is about emulating a HW disk cache in software. But I would
> > > not go that far without a really good reason.
> > > 
> > 
> > If we ever re-add the write support, it would be natural to implement it
> > using a 1-LEB cache. Block sectors are much much smaller than LEB's
> > (around three orders of magnitude), so we would want to read an entire
> > LEB into a 1-LEB buffer, update the buffer, and then write it back.
> > 
> > In this scheme, it's almost straightforward to keep this buffer around
> > as a cache, and only write-back to disk when the new read/write requests
> > asks for a different LEB.
> > 
> > IIRC, this is the main reason why we had the 1-LEB cache.
> 
> May be. But you are submitting R/O driver. And yet talk about caches and
> write support in several places. Caches are only relevant to write
> support. Write support is absent. Let's focus on what is present.
> 
> Feel free to put all the thoughts about a possible write support
> implementation to a single big comment in the code. That's fine. I do
> not object. I just want to focus on what is present.
> 

Totally agreed. Those comments were just a product of my own inertia.

If at all possible, take a look at the website doc and I'll push a new
round soon.

Thanks for the review,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Ezequiel Garcia]

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 <w@1wt.eu>
  Reviewed-by: Richard Weinberger <richard.weinberger@gmail.com>

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

Re: [PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Willy Tarreau]

On Mon, Feb 17, 2014 at 08:27:29AM -0300, Ezequiel Garcia wrote:
> Ah, I just noticed I forgot to add:
> 
>   Tested-by: Willy Tarreau <w@1wt.eu>
>   Reviewed-by: Richard Weinberger <richard.weinberger@gmail.com>
> 
> who tested and reviewed previous versions of the UBI patch, and
> I'm sure are reading this and will test and review this one ;-)

Well, I'll be able to run a few tests with it when time permits, but
as explained, my hopes of something useful vanished with the removal
of the write support, so in practice I'll stick to the initial patch
:-(

Willy

Re: [PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Artem Bityutskiy]

On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote:
> +static void __init ubiblock_add_from_param(void)
> +{
> +       int i, ret;
> +       struct ubiblock_param *p;
> +       struct ubi_volume_desc *desc;
> +       struct ubi_volume_info vi;
> +
> +       for (i = 0; i < ubiblock_devs; i++) {
> +               p = &ubiblock_param[i];
> +
> +               desc = open_volume_desc(p->name, p->ubi_num, p->vol_id);
> +               if (IS_ERR(desc)) {
> +                       ubi_warn("block: can't open volume, err=%ld\n",
> +                                PTR_ERR(desc));
> +                       continue;
> +               }

Should we be consistent here with how UBI behaves when attaches MTD
devices? UBI will error out if it cannot attach any. And for me it makes
sense. Indeed, if, the user, say asked to attach 2 UBI volumes via the
module parameter, surely the user expects to see 2 block device when
module loading finishes without errors?

What I read from this code means that even if loading finishes without
errors, I may see zero or 1 block devices, depending on how many of them
failed.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Artem Bityutskiy]

On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote:
> +static int ubiblock_read(struct ubiblock *dev, char *buffer, int pos, int len)
> +{
> +	int leb, offset, ret;
> +	int bytes_left = len;
> +	int to_read = len;

"pos" sounds like byte offset, which cannot be true because 'int' would
be too short type for it. 

....

> +static int do_ubiblock_request(struct ubiblock *dev, struct request *req)
> +{
> +	int pos, len, ret;
> +
> +	if (req->cmd_type != REQ_TYPE_FS)
> +		return -EIO;
> +
> +	if (blk_rq_pos(req) + blk_rq_cur_sectors(req) >
> +	    get_capacity(req->rq_disk))
> +		return -EIO;
> +
> +	if (rq_data_dir(req) != READ)
> +		return -ENOSYS; /* Write not implemented */
> +
> +	pos = blk_rq_pos(req) << 9;

So 'pos' is actually the 512-byte sector number? Would you please better
name then, something self-documenting like 'sec' or 'sector' ?

> +
> +	/*
> +	 * Let's prevent the device from being removed while we're doing I/O
> +	 * work. Notice that this means we serialize all the I/O operations,
> +	 * but it's probably of no impact given the NAND core serializes
> +	 * flash acceses anywafy.
> +	 */
> +	mutex_lock(&dev->vol_mutex);
> +	ret = ubiblock_read(dev, req->buffer, pos, len);
> +	mutex_unlock(&dev->vol_mutex);

Would you please a better name for 'vol_mutex'. Just makes me confused
because this is what we use in UBI to lock the entire volume. And here
it is different mutex. Let's express the code in clearer terms and use
something like just 'device_lock' or something which would suggest that
this is locks the entire ubiblock device.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Ezequiel Garcia]

On Mon, Feb 17, 2014 at 05:10:14PM +0200, Artem Bityutskiy wrote:
> On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote:
> > +static void __init ubiblock_add_from_param(void)
> > +{
> > +       int i, ret;
> > +       struct ubiblock_param *p;
> > +       struct ubi_volume_desc *desc;
> > +       struct ubi_volume_info vi;
> > +
> > +       for (i = 0; i < ubiblock_devs; i++) {
> > +               p = &ubiblock_param[i];
> > +
> > +               desc = open_volume_desc(p->name, p->ubi_num, p->vol_id);
> > +               if (IS_ERR(desc)) {
> > +                       ubi_warn("block: can't open volume, err=%ld\n",
> > +                                PTR_ERR(desc));
> > +                       continue;
> > +               }
> 
> Should we be consistent here with how UBI behaves when attaches MTD
> devices? UBI will error out if it cannot attach any. And for me it makes
> sense. Indeed, if, the user, say asked to attach 2 UBI volumes via the
> module parameter, surely the user expects to see 2 block device when
> module loading finishes without errors?
> 
> What I read from this code means that even if loading finishes without
> errors, I may see zero or 1 block devices, depending on how many of them
> failed.
> 

Good catch! This is wrong.

Thanks,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v6 2/3] ubi-utils: Add ubiblkvol tool

[Artem Bityutskiy]

On Sun, 2014-02-16 at 17:04 -0300, Ezequiel Garcia wrote:
> With the addition of block device access to UBI volumes, we now
> add a simple userspace tool to access the new ioctls.

I noticed you call this driver "block device access to UBI volumes" or
even "block device interface of UBI volumes". I am not sure we picked
the best terminology. Could we please discuss this a bit. Here are my
thoughts.

Essentially, what you have implemented is a R/O block driver. This
driver works on top of UBI volumes. It uses simple "1-to-1" mapping,
which is basically its media format.

Someone, in theory, may implement a more sophisticated block driver
which would be an R/W driver with good random I/O performance. This
driver would not use 1-to-1 mapping. It would have internal block
mapping tables, and garbage-collector.

There may be multiple drivers like this implemented.

These multiple drivers would have the same user interface - the block
API. But very different implementation, and different incompatible media
format.

What would be our terminology then, I wonder. What do yo think:

* If your driver is ubiblock, how would those be called, any idea?
* How would we distinguish between them when doing 'modprobe' ?
* How would we specify which one to use when using "ubiblkvol" ?

BTW, I am not sure "ubiblkvol" is a good name, may be you have other
ideas? Absent better ideas, may be calling it 'ubiblock' would be
better, just like the name of the driver? This would at least use less
of the "namespace".

Thoughts?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v6 2/3] ubi-utils: Add ubiblkvol tool

[Ezequiel Garcia]

On Tue, Feb 18, 2014 at 09:54:40AM +0200, Artem Bityutskiy wrote:
> On Sun, 2014-02-16 at 17:04 -0300, Ezequiel Garcia wrote:
> > With the addition of block device access to UBI volumes, we now
> > add a simple userspace tool to access the new ioctls.
> 
> I noticed you call this driver "block device access to UBI volumes" or
> even "block device interface of UBI volumes". I am not sure we picked
> the best terminology.

Yes, given we've shaped this more like a new UBI feature, than a standalone
driver, I tried to move away from the "ubiblock driver" wording.

> Could we please discuss this a bit. Here are my
> thoughts.
> 

Sure.

> Essentially, what you have implemented is a R/O block driver. This
> driver works on top of UBI volumes. It uses simple "1-to-1" mapping,
> which is basically its media format.
> 

So far, so good.

> Someone, in theory, may implement a more sophisticated block driver
> which would be an R/W driver with good random I/O performance. This
> driver would not use 1-to-1 mapping. It would have internal block
> mapping tables, and garbage-collector.
> 

Indeed. I've been playing with that idea, but (as we already discussed)
this simpler implementation is already interesting enough.

> There may be multiple drivers like this implemented.
> 

Well, I thought the above as future improvements of the current proposal.
But I guess we can't know in advance, how are they going to be like.

> These multiple drivers would have the same user interface - the block
> API. But very different implementation, and different incompatible media
> format.
> 
> What would be our terminology then, I wonder. What do yo think:
> 
> * If your driver is ubiblock, how would those be called, any idea?
> * How would we distinguish between them when doing 'modprobe' ?o

I'm not sure about any of these.

Are you thinking in having multiple block emulation implementations living
together using this same design; i.e. all being part of the UBI driver?

I'm not sure that would be sane. I'm more inclined towards improvements of
the current code. Is this idea too narrow-minded?

> * How would we specify which one to use when using "ubiblkvol" ?
>

Supposing we have several options, I guess a parameter will do.

> BTW, I am not sure "ubiblkvol" is a good name, may be you have other
> ideas? Absent better ideas, may be calling it 'ubiblock' would be
> better, just like the name of the driver? This would at least use less
> of the "namespace".
> 

Yes, could be. "ubiblock" looks a bit cleaner.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Ezequiel Garcia]

On Mon, Feb 17, 2014 at 05:22:31PM +0200, Artem Bityutskiy wrote:
> On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote:
> > +static int ubiblock_read(struct ubiblock *dev, char *buffer, int pos, int len)
> > +{
> > +	int leb, offset, ret;
> > +	int bytes_left = len;
> > +	int to_read = len;
> 
> "pos" sounds like byte offset, which cannot be true because 'int' would
> be too short type for it. 
> 

[..]
> > +
> > +	pos = blk_rq_pos(req) << 9;
> 
> So 'pos' is actually the 512-byte sector number? Would you please better
> name then, something self-documenting like 'sec' or 'sector' ?
> 

No, 'pos' is the byte offset. See the << 9, which is the custom
translation from 512-byte sector into byte offset.

I'm completely sure which type to use, size_t ? loff_t ? off_t ?

> > +
> > +	/*
> > +	 * Let's prevent the device from being removed while we're doing I/O
> > +	 * work. Notice that this means we serialize all the I/O operations,
> > +	 * but it's probably of no impact given the NAND core serializes
> > +	 * flash acceses anywafy.
> > +	 */
> > +	mutex_lock(&dev->vol_mutex);
> > +	ret = ubiblock_read(dev, req->buffer, pos, len);
> > +	mutex_unlock(&dev->vol_mutex);
> 
> Would you please a better name for 'vol_mutex'. Just makes me confused
> because this is what we use in UBI to lock the entire volume. And here
> it is different mutex. Let's express the code in clearer terms and use
> something like just 'device_lock' or something which would suggest that
> this is locks the entire ubiblock device.
> 

Sure, no problem.

Thanks for the review!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Ezequiel Garcia]

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:
> 
[..]
> > @@ -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... after re-reading the code in detail, it's obvious I took the
parameter handling code from mtd/ubi/build.c. So I'll push a v8 with
this additional copyright notice:

"""
  Driver parameter handling strongly based on drivers/mtd/ubi/build.c

    Copyright (c) International Business Machines Corp., 2006
    Copyright (c) Nokia Corporation, 2007
    Authors: Artem Bityutskiy (Битюцкий Артём), Frank Haverkamp
"""
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com