From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756260AbbDWGUZ (ORCPT ); Thu, 23 Apr 2015 02:20:25 -0400 Received: from mail-pd0-f180.google.com ([209.85.192.180]:36743 "EHLO mail-pd0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753680AbbDWGUX (ORCPT ); Thu, 23 Apr 2015 02:20:23 -0400 Date: Thu, 23 Apr 2015 15:20:15 +0900 From: Minchan Kim To: Sergey Senozhatsky Cc: Sergey Senozhatsky , Andrew Morton , Nitin Gupta , linux-kernel@vger.kernel.org Subject: Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality Message-ID: <20150423062015.GA6315@blaptop> References: <1429185356-11096-1-git-send-email-sergey.senozhatsky@gmail.com> <1429185356-11096-11-git-send-email-sergey.senozhatsky@gmail.com> <20150423030626.GI24928@blaptop> <20150423042300.GA724@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150423042300.GA724@swordfish> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 23, 2015 at 01:23:00PM +0900, Sergey Senozhatsky wrote: > On (04/23/15 12:06), Minchan Kim wrote: > > > +Example: > > > + cat /sys/class/zram-control/zram_add > > > > Why do we put zram-contol there rather than /sys/block/zram > > that's what clsss_register() does. > > [..] > > > > @@ -1168,8 +1172,15 @@ static int zram_add(int device_id) > > > > Why do zram_add need device_id? > > We decided to remove option user pass device_id. > > will cleanup. it was simpler at that time to support both devices > created by sysfs request and devices pre-crated for num_devices > module param. > > > > > +static struct zram *zram_lookup(int dev_id) > > > +{ > > > + struct zram *zram; > > > + > > > + zram = idr_find(&zram_index_idr, dev_id); > > > + if (zram) > > > + return zram; > > > + return ERR_PTR(-ENODEV); > > > > Just return NULL which is more simple and readable. > > > > ok. > > > [..] > > > + /* > > > + * First, make ->disksize device attr RO, closing > > > + * zram_remove() vs disksize_store() race window > > > > Why don't you use zram->init_lock to protect the race? > > zram_reset_device() takes this lock internally. but, it > unlocks the device upon the return from zram_reset_device(): > > lock idr_lock > zram_reset_device() > lock bd_mutex > __zam_reset_device() > lock init_lock > reset > unlock init_lock ---\ > unlock bd_mutex | > |<----- disksize_store() race window > zram_remove() ---/ > unlock idr_lock > > > until we call zram_remove() (which does sysfs_remove_group()) device has > sysfs attrs and, thus, disksize_store() can arrive in the middle. the > simplest things I came up with was that RO bit on sysfs disksize attrs. > I can factor out another set of __foo function to handle it differently, > not sure if this worth it. I believe we should handle it with init_lock more elegantly rather than RO trick. The init_lock was born to protect race between init and exit although we didn't need it until now since we don't have dynamic device feature. So, with refactoring, we should handle it with init_lock. > > I'll revisit it. Thanks. I believe you will find. -- Kind regards, Minchan Kim