linux-kernel - Re: [PATCH] zram: use writer semaphores in device attributes store

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160617041301.GA490@swordfish>
Date:	Fri, 17 Jun 2016 13:13:01 +0900
From:	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:	Geliang Tang <geliangtang@...il.com>
Cc:	Minchan Kim <minchan@...nel.org>, Nitin Gupta <ngupta@...are.org>,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] zram: use writer semaphores in device attributes store

Hello,

On (06/17/16 11:31), Geliang Tang wrote:
[..]
> Since the device attributes store provides write access. This patch uses
> down_write()/up_write() instead of down_read()/up_read() in
> mem_used_max_store() and compact_store().

we use ->init_lock not to make attrs exclusive, but to prevent
concurrent reset/etc.

> Signed-off-by: Geliang Tang <geliangtang@...il.com>
> ---
>  drivers/block/zram/zram_drv.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 7454cf1..cfed743 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -293,13 +293,13 @@ static ssize_t mem_used_max_store(struct device *dev,
>  	if (err || val != 0)
>  		return -EINVAL;
>  
> -	down_read(&zram->init_lock);
> +	down_write(&zram->init_lock);
>  	if (init_done(zram)) {
>  		struct zram_meta *meta = zram->meta;
>  		atomic_long_set(&zram->stats.max_used_pages,
>  				zs_get_total_pages(meta->mem_pool));
>  	}
> -	up_read(&zram->init_lock);
> +	up_write(&zram->init_lock);

not critical. can work.


>  	return len;
>  }
> @@ -372,15 +372,15 @@ static ssize_t compact_store(struct device *dev,
>  	struct zram *zram = dev_to_zram(dev);
>  	struct zram_meta *meta;
>  
> -	down_read(&zram->init_lock);
> +	down_write(&zram->init_lock);
>  	if (!init_done(zram)) {
> -		up_read(&zram->init_lock);
> +		up_write(&zram->init_lock);
>  		return -EINVAL;
>  	}
>  
>  	meta = zram->meta;
>  	zs_compact(meta->mem_pool);
> -	up_read(&zram->init_lock);
> +	up_write(&zram->init_lock);

pool compaction can take some time. *probably* seconds in the worst
case, when the device is under IO pressure and class' locks are heavily
contended and every class has a considerable number of objects to move.
so I think we don't want to block other attrs while we compact the pool,
compaction takes care of the concurrency internally.

	-ss