From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 1/9] vfs: add __iterate_supers() and helpers around it Date: Tue, 25 Sep 2012 11:52:48 +0200 Message-ID: <20120925095248.GE8049@quack.suse.cz> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Jan Kara , Al Viro , Josef Bacik , Eric Sandeen , Dave Chinner , Christoph Hellwig , linux-fsdevel@vger.kernel.org, fernando@intellilink.co.jp To: Fernando Luis Vazquez Cao Return-path: Received: from cantor2.suse.de ([195.135.220.15]:46983 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753739Ab2IYJwu (ORCPT ); Tue, 25 Sep 2012 05:52:50 -0400 Content-Disposition: inline In-Reply-To: <50617C78.8060701@lab.ntt.co.jp> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 wri= te mode > >>is preferable, so we add __iterate_supers(), which lets one > >>specify the mode of the lock, and replace iterate_supers with two h= elpers > >>around __iterate_supers(), iterate_supers_read() and iterate_supers= _write(). > >> > >>This will be used to fix the emergency thaw (filesystem unfreeze) c= ode, which > >>iterates over the list of superblocks but needs to hold the s_umoun= t semaphore > >>in _write_ mode bebore carrying out the actual thaw operation. > >> > >>This patch introduces no semantic changes since iterate_supers() us= ers 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 +09= 00 > >>+++ 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 it > >> * locked superblock and given argument. > >>+ * > >>+ * When the caller asks for the superblock lock (s_umount semaphor= e) to be > >>+ * taken in write mode, the lock is taken but not released because= the > >>+ * function provided by the caller may deactivate the superblock i= tself. > >>+ * 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; > >>@@ -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 ea= sily > >get them wrong and create bugs. I'd rather see emergency thaw retake= the > >s_umount semaphore so that iterate_supers() can drop it... >=20 > 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 reference (s_count) of the superblock. Thus deactivate_locked_super() will never really destroy it. So what I propose should be safe. > 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. I know but really the assymetry in the locking has to be observed by = the callback function. And if you have a callback function which doesn't wa= nt to deactivate superblock, it is far from obvious it should drop the s_umount semaphore... And I'm happy how you documented stuff but people read documentation only after they spot problems so it is better to hav= e interfaces which are hard to get wrong. Honza --=20 Jan Kara SUSE Labs, CR -- 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