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

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

Richard,

First of all, thanks for reviewing!

<nitpick>
If at all possible, it would be better if you could [snip]
the parts of the e-mail unrelated to the discussion.
It's a bit harder to follow when you include the whole patch
in your reply. Not a big deal, of course.
</nitpick>

On Sat, Feb 08, 2014 at 10:37:19PM +0100, Richard Weinberger wrote:
> On Wed, Jan 29, 2014 at 9:38 PM, Ezequiel Garcia
> <ezequiel.garcia@free-electrons.com> wrote:
[..]
> > +
> > +config MTD_UBI_BLOCK_WRITE_SUPPORT
> > +       bool "Enable write support (DANGEROUS)"
> > +       default n
> > +       depends on MTD_UBI_BLOCK
> > +       select MTD_UBI_BLOCK_CACHED
> > +       help
> > +          This is a *very* dangerous feature. Using a regular block-oriented
> > +          filesystem might impact heavily on a flash device wear.
> > +          Use with extreme caution.
> > +
> > +          If in doubt, say "N".
> 
> I really vote for dropping write support at all.
> 

I'll reply to this later.

[..]
> > +
> > +static int ubiblock_fill_cache(struct ubiblock *dev, int leb_num,
> > +                              struct ubiblock_cache *cache)
> > +{
> > +       int ret;
> > +
> > +       /* Warn if we fill cache while being dirty */
> > +       WARN_ON(cache->state == STATE_DIRTY);
> > +
> > +       cache->leb_num = leb_num;
> > +       cache->state = STATE_CLEAN;
> > +
> > +       ret = ubi_read(dev->desc, leb_num, cache->buffer, 0,
> > +                      dev->leb_size);
> > +       if (ret) {
> > +               ubi_err("%s ubi_read error %d", dev->gd->disk_name, ret);
> > +               return ret;
> > +       }
> 
> If read fails we still end up with a valid cache entry?
> Please set STATE_CLEAN only after a successful read.
> 

Good catch. I'll fix that.

[..]
> > +
> > +               mutex_lock(&dev->vol_mutex);
> > +               res = do_ubiblock_request(dev, req);
> > +               mutex_unlock(&dev->vol_mutex);
> 
> This means that you can never do parallel IO?
> 

Indeed. Feel free to prepare a follow-up patch improving it,
once this is merged.

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