From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752392AbbDWEWs (ORCPT ); Thu, 23 Apr 2015 00:22:48 -0400 Received: from mail-pd0-f171.google.com ([209.85.192.171]:34814 "EHLO mail-pd0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751320AbbDWEWq (ORCPT ); Thu, 23 Apr 2015 00:22:46 -0400 Date: Thu, 23 Apr 2015 13:23:00 +0900 From: Sergey Senozhatsky To: Minchan Kim Cc: Sergey Senozhatsky , Andrew Morton , Nitin Gupta , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCHv2 10/10] zram: add dynamic device add/remove functionality Message-ID: <20150423042300.GA724@swordfish> References: <1429185356-11096-1-git-send-email-sergey.senozhatsky@gmail.com> <1429185356-11096-11-git-send-email-sergey.senozhatsky@gmail.com> <20150423030626.GI24928@blaptop> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150423030626.GI24928@blaptop> 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 (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'll revisit it. -ss