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 18:42:16 +0900 Message-ID: <50617C78.8060701@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> 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, fernando@intellilink.co.jp To: Jan Kara Return-path: Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:60380 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754536Ab2IYJmo (ORCPT ); Tue, 25 Sep 2012 05:42:44 -0400 In-Reply-To: <20120925091119.GA8049@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 writ= e mode >> is preferable, so we add __iterate_supers(), which lets one >> specify the mode of the lock, and replace iterate_supers with two he= lpers >> around __iterate_supers(), iterate_supers_read() and iterate_supers_= write(). >> >> This will be used to fix the emergency thaw (filesystem unfreeze) co= de, which >> iterates over the list of superblocks but needs to hold the s_umount= semaphore >> in _write_ mode bebore carrying out the actual thaw operation. >> >> This patch introduces no semantic changes since iterate_supers() use= rs 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 +090= 0 >> +++ 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); >> =20 >> /** >> - * 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 l= ock) >> * >> * Scans the superblock list and calls given function, passing it >> * locked superblock and given argument. >> + * >> + * When the caller asks for the superblock lock (s_umount semaphore= ) to be >> + * taken in write mode, the lock is taken but not released because = the >> + * function provided by the caller may deactivate the superblock it= self. >> + * It is that function's job to unlock the superblock as needed in = 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; >> =20 >> @@ -555,10 +563,19 @@ void iterate_supers(void (*f)(struct sup >> sb->s_count++; >> spin_unlock(&sb_lock); >> =20 >> - 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); >> =20 >> spin_lock(&sb_lock); >> if (p) > These locking rules are ugly and counterintuitive. People will eas= ily > get them wrong and create bugs. I'd rather see emergency thaw retake = 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). Regarding the ugliness, please notice that __iterate_supers is static and is not supposed to be used directly; I added two wrappers around it (a read variant that is semantically identical to what we have now and a write variant) and documented them as thoroughly as I could. 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