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

[Artem Bityutskiy <dedekind1@gmail.com>]

Hi,

thanks for the patch, it is quite good I think, but I still have few
comments. I did not review very carefully because of limited amount of
free time.

There are few checkpatch.pl complaints, could you please take a look?

Note, often I give a comment for one place, but there are many other
similar places with the same stuff.

On Wed, 2011-08-17 at 15:17 +0200, david.wagner@free-electrons.com
wrote:
> TODO:
>  * the modules keeps a table of the devices which length is the maximum number
>    of UBI volumes.  There should be a better solution (linked list or, as
>    Christoph Hellwig suggests, a radix tree (idr)).

Wold be nice to do the same change for UBI, BTW :-)

> Here is the v3 of ubiblk implementing ioctls for on-demand creation/removal of
> ubiblk device ; is it what you were thinking of ?

Looks like.

[snip]

> +config MTD_UBI_UBIBLK
> +	tristate "Read-only block transition layer on top of UBI"
> +	help
> +	   Read-only block interface on top of UBI.
> +
> +	   This option adds ubiblk, which creates a read-ony block device from
> +	   UBI volumes.  It makes it possible to use block filesystems on top of
> +	   UBI (and thus, on top of MTDs while avoiding bad blocks).

s/block filesystems/R/O block filesystems/
s/on top of UBI/on top of UBI volumes/
s/and thus, on top of MTDs/and hence, on top of MTD devices/
> +
> +	   ubiblk devices are created by sending appropriate ioctl to the
> +	   ubiblk_ctrl device node.

s/sending/invoking/

[snip]

Would be good to add a section to the UBI Documentation describing
ubiblk by sending a patch against mtd-www:

http://git.infradead.org/mtd-www.git

[snip]

> +/*
> + * Structure representing a ubiblk device, proxying a UBI volume
> + */
> +struct ubiblk_dev {
> +	struct ubi_volume_desc *vol_desc;
> +	struct ubi_volume_info *vol_info;

Since this piece of code is part of drivers/mtd/ubi/, I think that it
makes sense to make it very consistent with the rest of the code. It is
then better to use consistent names for variables of this type: "desc"
and "vi".

> +	int ubi_num;
> +	int vol_id;
> +
> +	/* Block stuff */
> +	struct gendisk *gd;
> +	struct request_queue *rq;
> +	struct task_struct *thread;

Here is one comment I got from hch once which I also saved in the git
history: 6f904ff0e39ea88f81eb77e8dfb4e1238492f0a8

    hch: a convention I tend to use and I've seen in various places
    is to always use _task for the storage of the task_struct pointer,
    and thread everywhere else.  This especially helps with having
    foo_thread for the actual thread and foo_task for a global
    variable keeping the task_struct pointer

Let's follow it and call this field "<something_meaningful>_task", e.g.,
"req_task" (request queue/dispatcher/etc task) ? Keep using "_thread"
for other stuff.

You can also change UBI correspondingly.

> +	/* Protects the access to the UBI volume */
> +	struct mutex lock;

Lets call all locks <something_meaningful>_lock, e.g., volumes_lock or
vol_lock.

> +
> +	/* Avoids concurrent accesses to the request queue */
> +	spinlock_t queue_lock;
> +};

And for consistency, it is better to use kerneldoc comments, like the
rest of UBI, but if you hate them, I will not insist.

[snip]

> +	mutex_lock(&dev->lock);
> +	set_capacity(dev->gd,
> +		     (vol_info->size * vol_info->usable_leb_size) >> 9);
> +	mutex_unlock(&dev->lock);
> +	pr_debug("Resized ubiblk%d_%d to %d LEBs\n", vol_info->ubi_num,
> +		 vol_info->vol_id, vol_info->size);

If you find a way to properly use dev_dbg() instead of pr_debug(), your
messages will be automatically prefixed with "ubiblk%d_%d", and your
messages will be shorter - so less code, smaller binary size. See below
my other comment.

[snip]

> +static int ubiblk_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:
> +		break;
> +	case UBI_VOLUME_REMOVED:
> +		ubiblk_remove(&nt->vi);
> +		break;
> +	case UBI_VOLUME_RESIZED:
> +		ubiblk_resized(&nt->vi);
> +		break;
> +	case UBI_VOLUME_UPDATED:
> +		break;
> +	case UBI_VOLUME_RENAMED:
> +		break;
> +	default:
> +		break;

Please, remove cases which do nothing and let them end up in "default:".

[snip]

> +static long ubiblk_ctrl_ioctl(struct file *file, unsigned int cmd,
> +			      unsigned long arg)
> +{
> +	int err = 0;
> +	void __user *argp = (void __user *)arg;
> +
> +	struct ubi_volume_desc *vol_desc;
> +	struct ubi_volume_info vol_info;
> +	struct ubiblk_ctrl_req req;
> +
> +	if (!capable(CAP_SYS_RESOURCE))
> +		return -EPERM;
> +
> +	err = copy_from_user(&req, argp, sizeof(struct ubiblk_ctrl_req));
> +	if (err)
> +		return -EFAULT;
> +
> +	if (req.ubi_num < 0 || req.vol_id < 0)
> +		return -EINVAL;
> +
> +	vol_desc = ubi_open_volume(req.ubi_num, req.vol_id, UBI_READONLY);
> +	if (IS_ERR(vol_desc)) {
> +		pr_err("Opening volume %d on device %d failed\n",
> +		       req.vol_id, req.ubi_num);

Because dynamic_debug usually adds prefixes to messages, or pr_fmt is
usually used to add a prefix, I suggest to make all pr_* / dev_*
messages to start with small letter.

Also, should we use dev_* macros instead? I have always thought that
pr_* is used only if you do not have "struct device", no? But you do
have it, AFAICS:

1. For messages which are not related to a specific ubiblk device,
ubi_blk_ctrl_dev.this_device (may be ubi_blk_ctrk_dev then can be
shortened to ctrl_dev?)

2. For messages that are about a specific device - not exactly sure, but
I bet there is a struct device for your ubiblk_%d_%d device. Probably
disk_to_dev(<struct ubiblk_dev>->gd)

Try to find this out and test. We should not use pr_* unless there is a
good reason why dev_* does not work.

> +		return PTR_ERR(vol_desc);
> +	}
> +
> +	ubi_get_volume_info(vol_desc, &vol_info);
> +
> +	switch (cmd) {
> +	case UBIBLK_IOCADD:
> +		pr_info("Creating a ubiblk device proxing ubi%d:%d\n",
> +			req.ubi_num, req.vol_id);
> +		ubiblk_create(&vol_info);

Please, return the error code!

> +		break;
> +	case UBIBLK_IOCDEL:
> +		pr_info("Removing ubiblk from ubi%d:%d\n",
> +			req.ubi_num, req.vol_id);
> +		ubiblk_remove(&vol_info);

And here!

[snip]

> +static int __init ubi_ubiblk_init(void)
> +{
> +	int ret = 0;
> +
> +	pr_info("UBIBLK starting\n");

Please, kill this message, it looks more like tracing, not info. I
suggest you to add one single message at the end like "blah blah
initialized, major number %d".

> +
> +	ret = register_blkdev(0, "ubiblk");
> +	if (ret <= 0) {
> +		pr_err("UBIBLK: could not register_blkdev\n");
> +		return -ENODEV;
> +	}
> +	ubiblk_major = ret;
> +	pr_info("UBIBLK: device's major: %d\n", ubiblk_major);

Please, kill this message as well, or turn it into pr_debug()/dev_dbg().

Talking about messages:

1. Remove this UBIBLK: prefix from all of them. Rationale: if dynamic
debug is enabled, the dynamic debug infrastructure will add it for you,
see "Documentation/dynamic-debug-howto.txt". Otherwise you can always
define "pr_fmt" and add the prefix you wish.

[snip]

> +static void __exit ubi_ubiblk_exit(void)
> +{
> +	int i;
> +
> +	pr_info("UBIBLK: going to exit\n");

Please, kill these. Again, if you really feel like it - add one single
message like "blah exited" at the end.

[snip]

> +/* Structure to be passed to UBIBLK_IOCADD or IOCDEL ioctl */
> +struct ubiblk_ctrl_req {
> +	__s32 ubi_num;
> +	__s32 vol_id;
> +};
> +
> +/* ioctl commands of the UBI control character device */

Please, document here that you share "O" with UBI. Also document this in
UBI in a separate patch.

> +
> +#define UBIBLK_CTRL_IOC_MAGIC 'O'
> +
> +/* Create a ubiblk device from a UBI volume */
> +#define UBIBLK_IOCADD _IOW(UBIBLK_CTRL_IOC_MAGIC, 0x10, struct ubiblk_ctrl_req)
> +/* Delete a ubiblk device */
> +#define UBIBLK_IOCDEL _IOW(UBIBLK_CTRL_IOC_MAGIC, 0x11, struct ubiblk_ctrl_req)
> +
> +#endif


-- 
Best Regards,
Artem Bityutskiy