On (02/28/14 16:32), Andrew Morton wrote:
> Date: Fri, 28 Feb 2014 16:32:06 -0800
> From: Andrew Morton <a...@linux-foundation.org>
> To: Minchan Kim <minc...@kernel.org>
> Cc: Sasha Levin <sasha.le...@oracle.com>, ngu...@vflare.org, LKML
> <linux-kernel@vger.kernel.org>, Sergey Senozhatsky
> <sergey.senozhat...@gmail.com>
> Subject: Re: zram: lockdep spew for zram->init_lock
> X-Mailer: Sylpheed 3.2.0beta5 (GTK+ 2.24.10; x86_64-pc-linux-gnu)
>
> On Fri, 28 Feb 2014 08:56:29 +0900 Minchan Kim <minc...@kernel.org> wrote:
>
> > Sasha reported following below lockdep spew of zram.
> >
> > It was introduced by [1] in recent linux-next but it's false positive
> > because zram_meta_alloc with down_write(init_lock) couldn't be called
> > during zram is working as swap device so we could annotate the lock.
> >
> > But I don't think it's worthy because it would make greate lockdep
> > less effective. Instead, move zram_meta_alloc out of the lock as good
> > old day so we could do unnecessary allocation/free of zram_meta for
> > initialied device as Sergey claimed in [1] but it wouldn't be common
> > and be harmful if someone might do it. Rather than, I'd like to respect
> > lockdep which is great tool to prevent upcoming subtle bugs.
> >
> > [1] zram: delete zram_init_device
> >
> > ...
> >
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -537,26 +537,27 @@ static ssize_t disksize_store(struct device *dev,
> > struct device_attribute *attr, const char *buf, size_t len)
> > {
> > u64 disksize;
> > + struct zram_meta *meta;
> > struct zram *zram = dev_to_zram(dev);
> >
> > disksize = memparse(buf, NULL);
> > if (!disksize)
> > return -EINVAL;
> >
> > + disksize = PAGE_ALIGN(disksize);
> > + meta = zram_meta_alloc(disksize);
> > + if (!meta)
> > + return -ENOMEM;
> > +
> > down_write(&zram->init_lock);
> > if (init_done(zram)) {
> > + zram_meta_free(meta);
> > up_write(&zram->init_lock);
> > pr_info("Cannot change disksize for initialized device\n");
> > return -EBUSY;
> > }
> >
> > - disksize = PAGE_ALIGN(disksize);
> > - zram->meta = zram_meta_alloc(disksize);
> > - if (!zram->meta) {
> > - up_write(&zram->init_lock);
> > - return -ENOMEM;
> > - }
> > -
> > + zram->meta = meta;
> > zram->disksize = disksize;
> > set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
> > up_write(&zram->init_lock);
>
> When applying zram-use-zcomp-compressing-backends.patch on top of this
> we get a bit of a mess, and simple conflict resolution results in a
> leak.
>
> disksize_store() was one of those nasty functions which does multiple
> "return" statements after performing locking and resource allocation.
> As usual, this led to a resource leak. Remember folks, "return" is a
> goto in disguise.
>
>
> Here's what I ended up with. Please review.
>
looks good to me.
Acked-by: Sergey Senozhatsky <sergey.senozhat...@gmail.com>
> static ssize_t disksize_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t len)
> {
> u64 disksize;
> struct zram_meta *meta;
> struct zram *zram = dev_to_zram(dev);
> int err;
>
> disksize = memparse(buf, NULL);
> if (!disksize)
> return -EINVAL;
>
> disksize = PAGE_ALIGN(disksize);
> meta = zram_meta_alloc(disksize);
> if (!meta)
> return -ENOMEM;
>
> down_write(&zram->init_lock);
> if (init_done(zram)) {
> pr_info("Cannot change disksize for initialized device\n");
> err = -EBUSY;
> goto out_free_meta;
> }
>
> zram->comp = zcomp_create(default_compressor);
> if (!zram->comp) {
> pr_info("Cannot initialise %s compressing backend\n",
> default_compressor);
> err = -EINVAL;
> goto out_free_meta;
> }
>
> zram->meta = meta;
> zram->disksize = disksize;
> set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);
> up_write(&zram->init_lock);
>
> return len;
>
> out_free_meta:
> up_write(&zram->init_lock);
> zram_meta_free(meta);
> return err;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
