Re: [RFC/PATCH v2] ubi: Add ubiblock read-write driver

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

Hi Mike,

On Thu, Apr 18, 2013 at 04:30:55PM -0400, Mike Frysinger wrote:
> On Wednesday 12 December 2012 07:21:52 Ezequiel Garcia wrote:
> > Block device emulation on top of ubi volumes with read/write support.
> > Block devices are created upon user request through the
> > 'vol' module parameter.
> > 
> > For instance,
> > 
> >   $ modprobe ubiblock vol=/dev/ubi0_0
> >   $ modprobe ubiblock vol=0,rootfs
> 
> i played around with ubiblk before finding this newer version.  one thing i 
> think this is missing that ubiblk had is an ioctl interface for adding new 
> block volumes on the fly.  you can attach ubi volumes at runtime, but the only 
> way to attach ubiblocks is by loading/unloading the module, or by rebooting 
> and tweaking the command line.
> 
> imo, that support needs to be re-added.  it'd be great if we could do it via 
> the existing /dev/ubi_ctrl knob, but maybe that'll only work if ubi+ubiblock 
> are built into the kernel, or if ubiblock is merged with ubi ?
>

Yes, such support should be re-added. I'll think about it.

> > Read/write access is expected to work fairly well because the
> > request queue at block elevator orders block transfers to achieve
> > space-locality.
> > In other words, it's expected that reads and writes gets ordered
> > to address the same LEB.
> 
> i wonder if the write support should be put behind a CONFIG option.  
> personally, the write support is kind of neat and semi-useful for development, 
> but i don't plan on shipping anything on that :).  i just want read-only 
> support to load an ext2 fs on top of UBI.
> 

Mmm... good input. Maybe putting write support behind a CONFIG and
showing a big fat warning when the module loads will do?
(something to prevent regular users from using this carelessly).

May I ask why would you want to put ext2 fs? Have you considered f2fs?

> > +static int ubiblock_open(struct block_device *bdev, fmode_t mode)
> > +{
> > +	struct ubiblock *dev = bdev->bd_disk->private_data;
> > +	int ubi_mode = UBI_READONLY;
> > +	int ret;
> > +
> > +	mutex_lock(&dev->vol_mutex);
> > +	if (dev->refcnt > 0) {
> > +		/*
> > +		 * The volume is already opened,
> > +		 * just increase the reference counter
> > +		 */
> > +		dev->refcnt++;
> > +		mutex_unlock(&dev->vol_mutex);
> > +		return 0;
> > +	}
> > +
> > +	if (mode & FMODE_WRITE)
> > +		ubi_mode = UBI_READWRITE;
> 
> hmm, you handle ro vs rw here ...
> 
> > +	ret = ubiblock_alloc_cache(&dev->read_cache, dev->leb_size);
> > +	if (ret)
> > +		goto out_free;
> > +
> > +	ret = ubiblock_alloc_cache(&dev->write_cache, dev->leb_size);
> > +	if (ret)
> > +		goto out_free_cache;
> 
> ... but you always alloc a write cache even when it's mounted ro ?

Good catch.

I'll see if I can cook a v3 one of these days.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com