From mboxrd@z Thu Jan 1 00:00:00 1970 From: Fernando Luis Vazquez Cao Subject: Re: [PATCH 1/9] vfs: add __iterate_supers() and helpers around it Date: Tue, 25 Sep 2012 19:03:11 +0900 Message-ID: <5061815F.5070706@lab.ntt.co.jp> References: <1347605006.6868.2.camel@nexus.lab.ntt.co.jp> <1347605104.6868.3.camel@nexus.lab.ntt.co.jp> <20120925091119.GA8049@quack.suse.cz> <50617C78.8060701@lab.ntt.co.jp> <20120925095248.GE8049@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Al Viro , Josef Bacik , Eric Sandeen , Dave Chinner , Christoph Hellwig , linux-fsdevel@vger.kernel.org To: Jan Kara Return-path: Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:60617 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753499Ab2IYKDf (ORCPT ); Tue, 25 Sep 2012 06:03:35 -0400 In-Reply-To: <20120925095248.GE8049@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 2012/09/25 18:52, Jan Kara wrote: > On Tue 25-09-12 18:42:16, Fernando Luis Vazquez Cao wrote: >> On 2012=E5=B9=B409=E6=9C=8825=E6=97=A5 18:11, Jan Kara wrote: >>> On Fri 14-09-12 15:45:04, Fernando Luis V=C3=A1zquez Cao wrote: >>>> iterate_supers() calls a function provided by the caller with the = s_umount >>>> semaphore taken in read mode. However, there may be cases where wr= ite mode >>>> is preferable, so we add __iterate_supers(), which lets one >>>> specify the mode of the lock, and replace iterate_supers with two = helpers >>>> around __iterate_supers(), iterate_supers_read() and iterate_super= s_write(). >>>> >>>> This will be used to fix the emergency thaw (filesystem unfreeze) = code, which >>>> iterates over the list of superblocks but needs to hold the s_umou= nt semaphore >>>> in _write_ mode bebore carrying out the actual thaw operation. >>>> >>>> This patch introduces no semantic changes since iterate_supers() u= sers become >>>> iterate_supers_read() which is equivalent. >>>> >>>> Cc: Josef Bacik >>>> Cc: Eric Sandeen >>>> Cc: Christoph Hellwig >>>> Cc: Jan Kara >>>> Cc: Dave Chinner >>>> Signed-off-by: Fernando Luis Vazquez Cao >>>> --- >>> ... >>>> diff -urNp linux-3.6-rc5-orig/fs/super.c linux-3.6-rc5/fs/super.c >>>> --- linux-3.6-rc5-orig/fs/super.c 2012-09-14 11:53:43.416703312 +0= 900 >>>> +++ linux-3.6-rc5/fs/super.c 2012-09-14 12:30:52.188833193 +0900 >>>> @@ -537,14 +537,22 @@ void drop_super(struct super_block *sb) >>>> EXPORT_SYMBOL(drop_super); >>>> /** >>>> - * iterate_supers - call function for all active superblocks >>>> + * __iterate_supers - call function for all active superblocks >>>> * @f: function to call >>>> * @arg: argument to pass to it >>>> + * @wlock: mode of superblock lock (false->read lock, true->write= lock) >>>> * >>>> * Scans the superblock list and calls given function, passing i= t >>>> * locked superblock and given argument. >>>> + * >>>> + * When the caller asks for the superblock lock (s_umount semapho= re) to be >>>> + * taken in write mode, the lock is taken but not released becaus= e the >>>> + * function provided by the caller may deactivate the superblock = itself. >>>> + * It is that function's job to unlock the superblock as needed i= n such a >>>> + * case. >>>> */ >>>> -void iterate_supers(void (*f)(struct super_block *, void *), void= *arg) >>>> +static void __iterate_supers(void (*f)(struct super_block *, void= *), void *arg, >>>> + bool wlock) >>>> { >>>> struct super_block *sb, *p =3D NULL; >>>> @@ -555,10 +563,19 @@ void iterate_supers(void (*f)(struct sup >>>> sb->s_count++; >>>> spin_unlock(&sb_lock); >>>> - down_read(&sb->s_umount); >>>> + if (wlock) >>>> + down_write(&sb->s_umount); >>>> + else >>>> + down_read(&sb->s_umount); >>>> + >>>> if (sb->s_root && (sb->s_flags & MS_BORN)) >>>> f(sb, arg); >>>> - up_read(&sb->s_umount); >>>> + >>>> + /* When the semaphore was taken in write mode the function >>>> + * provided by the caller takes care of unlocking it as >>>> + * needed. See explanation above for details. */ >>>> + if (!wlock) >>>> + up_read(&sb->s_umount); >>>> spin_lock(&sb_lock); >>>> if (p) >>> These locking rules are ugly and counterintuitive. People will e= asily >>> get them wrong and create bugs. I'd rather see emergency thaw retak= e the >>> s_umount semaphore so that iterate_supers() can drop it... >> I guess you are referring to treating the write lock differently >> and not dropping the lock inside __iterate_supers(). The >> problem is that f() may release the last reference to the >> superblock which in turn will go away, so letting >> __iterate_supers() drop the lock is not safe (I added a >> comment about this issue in the function itself). > Well, except that iterate_supers() actually takes a passive refere= nce > (s_count) of the superblock. Thus deactivate_locked_super() will neve= r > really destroy it. So what I propose should be safe. Good point. I missed the fact that we are taking a passive reference there, which should simplifying locking. I will take that into account for the next revision. Thanks, =46ernando -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html