From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755132AbcFQENT (ORCPT ); Fri, 17 Jun 2016 00:13:19 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:33073 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750877AbcFQENS (ORCPT ); Fri, 17 Jun 2016 00:13:18 -0400 Date: Fri, 17 Jun 2016 13:13:01 +0900 From: Sergey Senozhatsky To: Geliang Tang Cc: Minchan Kim , Nitin Gupta , Sergey Senozhatsky , linux-kernel@vger.kernel.org Subject: Re: [PATCH] zram: use writer semaphores in device attributes store Message-ID: <20160617041301.GA490@swordfish> References: <8f203f0623625f4c2065557c7bc8f3b6fd8b6ed2.1466134116.git.geliangtang@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8f203f0623625f4c2065557c7bc8f3b6fd8b6ed2.1466134116.git.geliangtang@gmail.com> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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