lkml.org 
[lkml]   [2014]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: zram: lockdep spew for zram->init_lock
On Fri, 28 Feb 2014 08:56:29 +0900 Minchan Kim <minchan@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.

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;
}



\
 
 \ /
  Last update: 2014-03-01 02:21    [W:0.378 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site