From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754002AbbKCARr (ORCPT ); Mon, 2 Nov 2015 19:17:47 -0500 Received: from mail-pa0-f50.google.com ([209.85.220.50]:32850 "EHLO mail-pa0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751612AbbKCARl (ORCPT ); Mon, 2 Nov 2015 19:17:41 -0500 Date: Tue, 3 Nov 2015 09:18:35 +0900 From: Sergey Senozhatsky To: Geliang Tang Cc: Minchan Kim , Andrew Morton , linux-kernel@vger.kernel.org, Sergey Senozhatsky , Sergey Senozhatsky Subject: Re: [PATCH] zram: move init_done check to the beginning of disksize_store Message-ID: <20151103001835.GA1693@swordfish> References: <20151101234358.GA4783@swordfish> <20151102135349.GA16606@bogon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151102135349.GA16606@bogon> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, [please don't trim the Cc list] On (11/02/15 21:53), Geliang Tang wrote: > > Those things are not connected, absolutely. zram_meta_alloc() can fail > > even on un-initialized device. In any case disksize_store() does not end > > up changing the state of the device and reports the error back, so I don't > > see any real value in this change. Seems that we come across this change > > something like once a year (this patch is not the first). > > Thanks for your reply. > > zram_meta_alloc() always fails on initialized device at debugfs_create_dir(): > debugfs dir creation failed Aha, so you are talking about calling disksize_store() on - already initialized device that - uses zsmalloc with - CONFIG_ZSMALLOC_STAT as an alacator (zbud is not affected). that should have been mentioned in the commit message. hm, does the patch _actually_ make it better? splitting a ->init_lock regions is *never* a good idea. consider an un-initialized device. CPU0 CPU1 down_read(&zram->init_lock); if (init_done(zram)) return -EBUSY; up_read(&zram->init_lock); down_read(&zram->init_lock); if (init_done(zram)) return -EBUSY; up_read(&zram->init_lock); meta = zram_meta_alloc(); meta = zram_meta_alloc(); comp = zcomp_create(); comp = zcomp_create(); down_write(&zram->init_lock); zram->meta = meta; ... up_write(&zram->init_lock); down_write(&zram->init_lock); zram->meta = meta; ... up_write(&zram->init_lock); So the above code will return a "debugfs dir creation failed" IFF zsmalloc with CONFIG_ZSMALLOC_STAT enabled is being used (regardless the effort). And it will totally succeed and lead to N (depending on the number of concurrent disksize_store() calls) pool memory and zcomp_create memory leaks otherwise. Because of this part: --- down_write(&zram->init_lock); - if (init_done(zram)) { - pr_info("Cannot change disksize for initialized device\n"); - err = -EBUSY; - goto out_destroy_comp; - } - init_waitqueue_head(&zram->io_done); atomic_set(&zram->refcount, 1); zram->meta = meta; --- > zram: Error creating memory pool > > I think we should deal with this error. Is it so critical? I mean it does not break the existing device; we print the error and keep the existing device fully functional. The error line may be a bit misleading, which is another thing and can be solved documenting the behavior, *for example*. (I just don't really want to complicate the code for a corner case which doesn't even break anything). -ss