From: Artem Bityutskiy <dedekind1@gmail.com>
To: David Wagner <david.wagner@free-electrons.com>
Cc: linux-mtd <linux-mtd@lists.infradead.org>,
linux-embedded <linux-embedded@vger.kernel.org>,
lkml <linux-kernel@vger.kernel.org>,
Tim Bird <tim.bird@am.sony.com>,
David Woodhouse <dwmw2@infradead.org>,
Ricard Wanderlof <ricard.wanderlof@axis.com>,
Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCHv9] UBI: new module ubiblk: block layer on top of UBI
Date: Sat, 01 Oct 2011 17:08:44 +0300 [thread overview]
Message-ID: <1317478130.19426.151.camel@sauron> (raw)
In-Reply-To: <1317048049-19391-1-git-send-email-david.wagner@free-electrons.com>
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
prev parent reply other threads:[~2011-10-01 14:08 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1308922482-14967-1-git-send-email-david.wagner@free-electrons.com>
2011-06-28 15:24 ` [RFC PATCHv2] UBI: new module ubiblk: block layer on top of UBI david.wagner
2011-06-29 6:54 ` Artem Bityutskiy
2011-07-26 12:27 ` [PATCH] " David Wagner
2011-07-26 12:34 ` Christoph Hellwig
2011-07-26 12:58 ` David Wagner
2011-07-28 6:14 ` Artem Bityutskiy
2011-08-15 11:56 ` Artem Bityutskiy
2011-08-17 13:17 ` [PATCHv3] " david.wagner
2011-08-17 14:20 ` [PATCH] Tools for controling ubiblk David Wagner
2011-08-22 8:17 ` Artem Bityutskiy
2011-08-22 7:39 ` [PATCHv3] UBI: new module ubiblk: block layer on top of UBI Artem Bityutskiy
2011-08-22 7:42 ` Artem Bityutskiy
2011-08-24 16:23 ` Arnd Bergmann
2011-08-25 7:06 ` Artem Bityutskiy
2011-08-25 15:12 ` Arnd Bergmann
2011-09-01 12:55 ` David Wagner
2011-09-06 3:44 ` Artem Bityutskiy
2011-09-06 4:10 ` Artem Bityutskiy
2011-09-06 4:29 ` Artem Bityutskiy
2011-09-08 15:26 ` Arnd Bergmann
2011-09-09 11:53 ` Artem Bityutskiy
2011-09-09 12:02 ` Artem Bityutskiy
2011-09-09 14:25 ` Arnd Bergmann
2011-09-09 15:27 ` Artem Bityutskiy
2011-09-09 14:41 ` David Wagner
2011-09-09 14:51 ` Arnd Bergmann
2011-09-11 10:18 ` Artem Bityutskiy
2011-09-11 10:35 ` David Wagner
2011-08-24 16:15 ` [PATCHv4] " david.wagner
2011-08-24 16:21 ` [PATCH] document ubiblk's usage of the same ioctl magic as a part " David Wagner
2011-09-06 4:58 ` Artem Bityutskiy
2011-09-06 4:55 ` [PATCHv4] UBI: new module ubiblk: block layer on top " Artem Bityutskiy
2011-09-12 9:51 ` [PATCHv5] " David Wagner
2011-09-19 4:50 ` Artem Bityutskiy
2011-09-22 7:58 ` [PATCHv6] " David Wagner
2011-09-23 10:58 ` Artem Bityutskiy
2011-09-26 12:58 ` David Wagner
2011-09-26 9:17 ` Ricard Wanderlof
2011-09-26 12:38 ` [PATCHv7] " David Wagner
2011-09-26 13:20 ` Artem Bityutskiy
2011-09-26 14:25 ` [PATCHv8] " David Wagner
2011-09-26 14:36 ` Artem Bityutskiy
2011-09-26 14:40 ` [PATCHv9] " David Wagner
2011-10-01 14:08 ` Artem Bityutskiy [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1317478130.19426.151.camel@sauron \
--to=dedekind1@gmail.com \
--cc=arnd@arndb.de \
--cc=david.wagner@free-electrons.com \
--cc=dwmw2@infradead.org \
--cc=linux-embedded@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=ricard.wanderlof@axis.com \
--cc=tim.bird@am.sony.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).