On Fri, 28 Feb 2014 08:56:29 +0900 Minchan Kim <[email protected]> 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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
