From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Sandeen Subject: Re: [PATCH 1/9] vfs: add __iterate_supers() helper Date: Thu, 13 Sep 2012 21:36:39 -0500 Message-ID: <50529837.5060307@redhat.com> References: <1347533862.5646.2.camel@nexus.lab.ntt.co.jp> <1347534020.5646.4.camel@nexus.lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Al Viro , Josef Bacik , Dave Chinner , Christoph Hellwig , Jan Kara , linux-fsdevel@vger.kernel.org To: =?UTF-8?B?RmVybmFuZG8gTHVpcyBWw6F6cXVleiBDYW8=?= Return-path: Received: from mx1.redhat.com ([209.132.183.28]:41121 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752725Ab2INCgs (ORCPT ); Thu, 13 Sep 2012 22:36:48 -0400 In-Reply-To: <1347534020.5646.4.camel@nexus.lab.ntt.co.jp> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 9/13/12 6:00 AM, Fernando Luis V=C3=A1zquez Cao wrote: > iterate_supers() calls a function provided by the caller with the s_u= mount > semaphore taken in read mode. However, there may be cases where write= mode > is preferable, so we add a new helper, __iterate_supers(), which lets= one > specify the mode of the lock. >=20 > This will be used to fix the emergency thaw (filesystem unfreeze) cod= e, 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. I'll go ahead & comment on this even though all of this is to support t= he emergency thaw misfeature which I'm liking less and less this evening. I think this split is a little odd, usually __foo() provides some funda= mental action, and foo_*() functions call it after possibly defining or alteri= ng aspects of that action, or preparing for that action. So having iterate_supers() do read locks, and __iterate_supers do eithe= r read or write, seems asymmetric, and isn't very well self-documenting. Why not have i.e. iterate_supers_read() and iterate_supers_write(), eac= h of which can call __iterate_supers(blah, blah, locktype). Anyway, it'd mean changing 5 callers but it's a little clearer and more= symmetric to me. What do you think? The logic seems fine, just a question about the style. Thanks, -Eric > Cc: Josef Bacik > Cc: Eric Sandeen > Cc: Christoph Hellwig > Cc: Jan Kara > Cc: Dave Chinner > Signed-off-by: Fernando Luis Vazquez Cao > --- >=20 > 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-12 18:45:13.818046999 +0900 > +++ linux-3.6-rc5/fs/super.c 2012-09-12 19:08:58.214034467 +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 lo= ck) > * > * 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 t= he > + * function provided by the caller may deactivate the superblock its= elf. > + * It is that function's job to unlock the superblock as needed in s= uch a > + * case. > */ > -void iterate_supers(void (*f)(struct super_block *, void *), void *a= rg) > +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) > @@ -571,6 +588,19 @@ void iterate_supers(void (*f)(struct sup > } > =20 > /** > + * iterate_supers - call function for all active superblocks > + * @f: function to call > + * @arg: argument to pass to it > + * > + * Scans the superblock list and calls given function, passing it > + * a superblock locked in _read_ mode and given argument. > + */ > +void iterate_supers(void (*f)(struct super_block *, void *), void *a= rg) > +{ > + __iterate_supers(f, arg , false); > +} > + > +/** > * iterate_supers_type - call function for superblocks of given type > * @type: fs type > * @f: function to call > diff -urNp linux-3.6-rc5-orig/include/linux/fs.h linux-3.6-rc5/includ= e/linux/fs.h > --- linux-3.6-rc5-orig/include/linux/fs.h 2012-09-12 18:45:14.0020470= 01 +0900 > +++ linux-3.6-rc5/include/linux/fs.h 2012-09-12 18:46:39.466082276 +0= 900 > @@ -2684,6 +2684,8 @@ extern struct super_block *get_super(str > extern struct super_block *get_super_thawed(struct block_device *); > extern struct super_block *get_active_super(struct block_device *bde= v); > extern void drop_super(struct super_block *sb); > +extern void __iterate_supers(void (*f)(struct super_block *, void *)= , void *arg, > + bool wlock); > extern void iterate_supers(void (*)(struct super_block *, void *), v= oid *); > extern void iterate_supers_type(struct file_system_type *, > void (*)(struct super_block *, void *), void *); >=20 >=20 -- 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