Re: [PATCHv9] UBI: new module ubiblk: block layer on top of UBI

From: Artem Bityutskiy
Date: Sat Oct 01 2011 - 10:05:52 EST

On Mon, 2011-09-26 at 16:40 +0200, David Wagner wrote:
> ubiblk is a read-only block layer on top of UBI. It presents UBI volumes as
> read-only block devices (named ubiblkX_Y, where X is the UBI device number
> and Y the Volume ID).
>
> It is used by putting a block filesystem image on a UBI volume, creating the
> corresponding ubiblk device and then mounting it.
>
> It uses the UBI API to register to UBI notifications and to read from the
> volumes. It also creates a ubiblk_ctrl device node that simply receives ioctl
> from a userspace tool for creating/removing ubiblk devices.
>
> Some code is taken from mtd_blkdevs and gluebi. Some code for the ioctl part is
> also inspired from ubi's core.
>
> Advantages of ubiblk over gluebi+mtdblock_ro:

I do not have enough time to nicely answer with comments, so here is
just some patch with my cosmetic changes plus I added "TODO:" items here
and there. Please, apply it and resolve the TODO items, if you can, ok?

diff --git a/drivers/mtd/ubi/ubiblk.c b/drivers/mtd/ubi/ubiblk.c
index ccb22de..2da46fe 100644
--- a/drivers/mtd/ubi/ubiblk.c
+++ b/drivers/mtd/ubi/ubiblk.c
@@ -40,7 +40,7 @@
#define BLK_SIZE 512

/**
- * struct ubiblk_dev - represents a ubiblk device, proxying a UBI volume
+ * struct ubiblk_dev - represents a ubiblk device, proxying a UBI volume.
* @desc: open UBI volume descriptor
* @vi: UBI volume information
* @ubi_num: UBI device number
@@ -49,25 +49,25 @@
* @gd: the disk (block device) created by ubiblk
* @rq: the request queue to @gd
* @req_task: the thread processing @rq requests
+TODO: vol_lock is bad name, not clean what it protects, the below comment is
+also vague
* @vol_lock: protects write access to the elements of this structure
- * @queue_lock: avoids concurrent accesses to the request queue
- * @list: linked list structure
+ * @queue_lock: protects the request queue
+ * @list: links &struct ubiblk_dev objects
*/
struct ubiblk_dev {
+/* TODO: let's name this structure ubiblk_info, to be consistent with UBI's
+ * naming conventions. */
struct ubi_volume_desc *desc;
struct ubi_volume_info *vi;
int ubi_num;
int vol_id;
int refcnt;
-
struct gendisk *gd;
struct request_queue *rq;
struct task_struct *req_task;
-
struct mutex vol_lock;
-
spinlock_t queue_lock;
-
struct list_head list;
};

@@ -105,24 +105,23 @@ static struct ubiblk_dev *find_dev(struct ubi_volume_info *vi)
}

/**
- * do_ubiblk_request - Read a LEB and fill the request buffer with the
- * requested sector.
+ * do_request - fill the request buffer by reading the UBI volume.
* @req: the request data structure
* @dev: the ubiblk device on which the request is issued
+ *
+ * Returns zero in case of success and a negative error code in case of
+ * failure.
*/
-static int do_ubiblk_request(struct request *req, struct ubiblk_dev *dev)
+static int do_request(struct request *req, struct ubiblk_dev *dev)
+/* TODO: if struct ubiblk_dev becomes struct ubiblk_info, how about to
+ * name all variables of this type "inf"? */
{
unsigned long start, len, read_bytes;
- int offset;
- int leb;
- int ret;
+ int offset, leb, ret;

start = blk_rq_pos(req) << 9;
len = blk_rq_cur_bytes(req);
read_bytes = 0;
-
- /* We are always reading. No need to handle writing for now */
-
leb = start / dev->vi->usable_leb_size;
offset = start % dev->vi->usable_leb_size;

@@ -130,31 +129,34 @@ static int do_ubiblk_request(struct request *req, struct ubiblk_dev *dev)
if (offset + len > dev->vi->usable_leb_size)
len = dev->vi->usable_leb_size - offset;

- if (unlikely(blk_rq_pos(req) + blk_rq_cur_sectors(req) >
- get_capacity(req->rq_disk))) {
+ if (blk_rq_pos(req) + blk_rq_cur_sectors(req) >
+ get_capacity(req->rq_disk)) {
+ /*
+ * TODO: snitize the error message, e.g.,
+ * "cannot read sector %llu beyond device size %llu"
+ */
dev_err(disk_to_dev(dev->gd),
"attempting to read too far\n");
+ /*
+ * TODO: hmm, is -EIO the right error? What other block
+ * devices return in this case? Any specific pointer
+ * please?
+ */
return -EIO;
}

- /* Read (len) bytes of LEB (leb) from (offset) and put the
- * result in the buffer given by the request.
- * If the request is overlapping on several lebs, (read_bytes)
- * will be > 0 and the data will be put in the buffer at
- * offset (read_bytes)
- */
- ret = ubi_read(dev->desc, leb, req->buffer + read_bytes,
- offset, len);
-
+ ret = ubi_read(dev->desc, leb, req->buffer + read_bytes, offset,
+ len);
if (ret) {
- dev_err(disk_to_dev(dev->gd), "ubi_read error\n");
+ dev_err(disk_to_dev(dev->gd),
+ "can't read %d bytes from LEB %d:%d, error %d\n",
+ len, leb, offset, ret);
return ret;
}

read_bytes += len;
-
len = blk_rq_cur_bytes(req) - read_bytes;
- leb++;
+ leb += 1;
offset = 0;
} while (read_bytes < blk_rq_cur_bytes(req));

@@ -162,38 +164,34 @@ static int do_ubiblk_request(struct request *req, struct ubiblk_dev *dev)
}

/**
- * ubiblk_request - wakes the processing thread
- * @rq: the request queue which device is to be awaken
+ * ubiblk_request - wakes the processing thread.
+ * @rq: the request queue which requires processing
*/
+/* TODO: bad name, may be wakeup_req_thread() would be better? */
static void ubiblk_request(struct request_queue *rq)
{
struct ubiblk_dev *dev;
struct request *req;

dev = rq->queuedata;
-
- if (!dev)
+ if (dev)
+ wake_up_process(dev->req_task);
+ else {
+ /* TODO: an error message or WARN here ? */
while ((req = blk_fetch_request(rq)) != NULL)
__blk_end_request_all(req, -ENODEV);
- else
- wake_up_process(dev->req_task);
+ }
}

-/**
- * ubiblk_open - open a UBI volume (get the volume descriptor).
- * @bdev: the corresponding block device
- * @mode: opening mode (don't care as long as ubiblk is read-only)
- */
static int ubiblk_open(struct block_device *bdev, fmode_t mode)
{
struct ubiblk_dev *dev = bdev->bd_disk->private_data;
int err;

mutex_lock(&dev->vol_lock);
- dev_dbg(disk_to_dev(dev->gd), "open(); refcnt = %d\n", dev->refcnt);
if (dev->refcnt > 0) {
/*
- * The volume is already opened ; just increase the reference
+ * The volume is already opened, just increase the reference
* counter.
*/
dev->refcnt++;
@@ -201,11 +199,12 @@ static int ubiblk_open(struct block_device *bdev, fmode_t mode)
return 0;
}

- dev->desc = ubi_open_volume(dev->ubi_num, dev->vol_id,
- UBI_READONLY);
+ dev->desc = ubi_open_volume(dev->ubi_num, dev->vol_id, UBI_READONLY);
if (IS_ERR(dev->desc)) {
+ /* TODO: Failed to open what? which volume? Why not to print
+ * full information? Could you please go through _all_ error
+ * message and assess them WRT niceness to the user? */
dev_err(disk_to_dev(dev->gd), "failed to open");
-
err = PTR_ERR(dev->desc);
dev->desc = NULL;
goto out_unlock;
@@ -219,7 +218,6 @@ static int ubiblk_open(struct block_device *bdev, fmode_t mode)
ubi_get_volume_info(dev->desc, dev->vi);

dev->refcnt++;
- dev_dbg(disk_to_dev(dev->gd), "opened mode=%d\n", mode);
mutex_unlock(&dev->vol_lock);
return 0;

@@ -231,38 +229,30 @@ out_unlock:
return err;
}

-/**
- * ubiblk_release - close a UBI volume (close the volume descriptor).
- * @gd: the disk that was previously opened
- * @mode: don't care
- */
static int ubiblk_release(struct gendisk *gd, fmode_t mode)
{
struct ubiblk_dev *dev = gd->private_data;

mutex_lock(&dev->vol_lock);
- dev_dbg(disk_to_dev(dev->gd), "release(); refcnt = %d\n", dev->refcnt);
-
dev->refcnt--;
if (dev->refcnt == 0) {
kfree(dev->vi);
dev->vi = NULL;
-
ubi_close_volume(dev->desc);
dev->desc = NULL;
-
- dev_dbg(disk_to_dev(dev->gd), "released, mode=%d\n", mode);
}
-
mutex_unlock(&dev->vol_lock);
+
return 0;
}

/**
- * ubiblk_thread - loop on the block request queue and wait for new
- * requests ; run them with do_ubiblk_request(). Mostly copied from
- * mtd_blkdevs.c.
+ * ubiblk_thread - dispatch UBI requests.
* @arg: the ubiblk device which request queue to process
+ *
+ * This function loops on the block request queue and waits for new requests.
+ * Returns zero in case of success and a negative error code in case of
+ * failure.
*/
static int ubiblk_thread(void *arg)
{
@@ -270,8 +260,9 @@ static int ubiblk_thread(void *arg)
struct request_queue *rq = dev->rq;
struct request *req = NULL;

+ /* TODO: I doubt you need to disable IRQs because you do not have any
+ * of them! Please, investigate this. */
spin_lock_irq(rq->queue_lock);
-
while (!kthread_should_stop()) {
int res;

@@ -282,40 +273,37 @@ static int ubiblk_thread(void *arg)

if (kthread_should_stop())
set_current_state(TASK_RUNNING);
-
spin_unlock_irq(rq->queue_lock);
+
schedule();
+
spin_lock_irq(rq->queue_lock);
continue;
}
-
spin_unlock_irq(rq->queue_lock);

mutex_lock(&dev->vol_lock);
- res = do_ubiblk_request(req, dev);
+ res = do_request(req, dev);
mutex_unlock(&dev->vol_lock);

spin_lock_irq(rq->queue_lock);
-
if (!__blk_end_request_cur(req, res))
- req = NULL;
+ req = NULL;
}

if (req)
__blk_end_request_all(req, -EIO);
-
spin_unlock_irq(rq->queue_lock);

return 0;
}

/**
- * ubiblk_create - create a ubiblk device proxying a UBI volume.
+ * ubiblk_create - create a ubiblk device.
* @vi: the UBI volume information data structure
*
- * An UBI volume has been created ; create a corresponding ubiblk device:
- * Initialize the locks, the structure, the block layer infos and start a
- * req_task.
+ * Creates a ubiblk device for UBI volume described by @vi. Returns zero in
+ * case of success and a negative error code in case of failure.
*/
static int ubiblk_create(struct ubi_volume_info *vi)
{
@@ -371,16 +359,14 @@ static int ubiblk_create(struct ubi_volume_info *vi)
blk_queue_logical_block_size(dev->rq, BLK_SIZE);
dev->gd->queue = dev->rq;

- /* Borrowed from mtd_blkdevs.c */
- /* Create processing req_task
- *
+ /*
* The processing of the request has to be done in process context (it
- * might sleep) but blk_run_queue can't block ; so we need to separate
+ * might sleep) but blk_run_queue can't block; so we need to separate
* the event of a request being added to the queue (which triggers the
* callback ubiblk_request - that is set with blk_init_queue())
* and the processing of that request.
*
- * Thus, the sole purpose of ubi_ubiblk_reuqest is to wake the kthread
+ * Thus, the sole purpose of ubiblk_request is to wake the kthread
* up so that it will process the request queue
*/
dev->req_task = kthread_run(ubiblk_thread, dev, "%s%d_%d",
@@ -396,7 +382,6 @@ static int ubiblk_create(struct ubi_volume_info *vi)
dev_info(disk_to_dev(dev->gd),
"created from ubi%d:%d(%s)\n", dev->ubi_num, dev->vol_id,
vi->name);
-
mutex_unlock(&devlist_lock);

return 0;
@@ -417,15 +402,14 @@ out_unlock:
* ubiblk_remove - destroy a ubiblk device.
* @vi: the UBI volume information data structure
*
- * A UBI volume has been removed or we are requested to unproxify a volume ;
- * destroy the corresponding ubiblk device.
+ * Destroys the ubiblk device for UBI volume described by @vi. Returns zero in
+ * case of success and a negative error code in case of failure.
*/
static int ubiblk_remove(struct ubi_volume_info *vi)
{
struct ubiblk_dev *dev;

mutex_lock(&devlist_lock);
-
dev = find_dev(vi);
if (!dev) {
mutex_unlock(&devlist_lock);
@@ -445,7 +429,6 @@ static int ubiblk_remove(struct ubi_volume_info *vi)
blk_cleanup_queue(dev->rq);
kthread_stop(dev->req_task);
put_disk(dev->gd);
-
list_del(&dev->list);
mutex_unlock(&dev->vol_lock);
mutex_unlock(&devlist_lock);
@@ -459,17 +442,15 @@ static int ubiblk_remove(struct ubi_volume_info *vi)
* ubiblk_resize - resize a ubiblk device.
* @vi: the UBI volume information data structure
*
- * A UBI volume has been resized, change the ubiblk device geometry accordingly.
+ * A UBI volume has been resized, change the ubiblk device geometry
+ * accordingly. Returns zero in case of success and a negative error code in
+ * case of failure.
*/
static int ubiblk_resize(struct ubi_volume_info *vi)
{
struct ubiblk_dev *dev;
int disk_capacity;

- /* We don't touch the list, but we better lock it: it could be that the
- * device gets removed between the time the device has been found and
- * the time we access dev->gd
- */
mutex_lock(&devlist_lock);
dev = find_dev(vi);
if (!dev) {
@@ -482,10 +463,9 @@ static int ubiblk_resize(struct ubi_volume_info *vi)
mutex_lock(&dev->vol_lock);
disk_capacity = (vi->size * vi->usable_leb_size) >> 9;
set_capacity(dev->gd, disk_capacity);
- dev_dbg(disk_to_dev(dev->gd), "resized to %d LEBs\n", vi->size);
mutex_unlock(&dev->vol_lock);
-
mutex_unlock(&devlist_lock);
+
return 0;
}

--
Best Regards,
Artem Bityutskiy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/