From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752203AbbHMLCC (ORCPT ); Thu, 13 Aug 2015 07:02:02 -0400 Received: from mx2.suse.de ([195.135.220.15]:47925 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751442AbbHMLCA (ORCPT ); Thu, 13 Aug 2015 07:02:00 -0400 Date: Thu, 13 Aug 2015 13:01:55 +0200 From: Jan Kara To: Oleg Nesterov Cc: Al Viro , Dave Chinner , Dave Hansen , Jan Kara , "Paul E. McKenney" , Peter Zijlstra , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 9/8] don't fool lockdep in freeze_super() and thaw_super() paths Message-ID: <20150813110155.GI26599@quack.suse.cz> References: <20150811170343.GA26881@redhat.com> <20150812131138.GA7462@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150812131138.GA7462@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 12-08-15 15:11:38, Oleg Nesterov wrote: > On 08/11, Oleg Nesterov wrote: > > > > The only essential change is that I dropped the lockdep improvements > > as we discussed. This means that 8/8 was changed a bit, and I decided > > to add the new documentation patch, see 3/8. > > Update: The recent > > [PATCH 0/2] xfs: kill lockdep false positives from readdir > > changes from Dave fixed the problems ILOCK false-positives. So we can > add the additional patch which (modulo comments) just turns v2 back into > v1. > > Dave, Jan, you seem to agree with these patches. How should we route > this all? Regarding the routing, ideally Al Viro should take these as a VFS maintainer. > ------------------------------------------------------------------------------- > Subject: [PATCH v2 9/8] don't fool lockdep in freeze_super() and thaw_super() paths > > sb_wait_write()->percpu_rwsem_release() fools lockdep to avoid the > false-positives. Now that xfs was fixed by Dave we can remove it and > change freeze_super() and thaw_super() to run with s_writers.rw_sem > locks held; we add two trivial helpers for that, sb_freeze_release() > and sb_freeze_acquire(). > > While at it, kill the outdated part of the comment above sb_wait_write. > > Signed-off-by: Oleg Nesterov The patch looks good. Just one nit: > + for (level = SB_FREEZE_LEVELS; --level >= 0; ) > + percpu_rwsem_release(sb->s_writers.rw_sem + level, 0, _THIS_IP_); It is more common (and to me more readable) to have the loop written as: for (level = SB_FREEZE_LEVELS - 1; level >= 0; level--) I agree what you do is shorter but IMHO it's just an unnecessary obfuscation :) Otherwise feel free to add: Reviewed-by: Jan Kara Honza > +} > + > +/* > + * Tell lockdep we are holding these locks before we call ->unfreeze_fs(sb). > + */ > +static void sb_freeze_acquire(struct super_block *sb) > { > int level; > > for (level = 0; level < SB_FREEZE_LEVELS; ++level) > percpu_rwsem_acquire(sb->s_writers.rw_sem + level, 0, _THIS_IP_); > +} > + > +static void sb_freeze_unlock(struct super_block *sb) > +{ > + int level; > > for (level = SB_FREEZE_LEVELS; --level >= 0; ) > percpu_up_write(sb->s_writers.rw_sem + level); > @@ -1329,6 +1336,7 @@ int freeze_super(struct super_block *sb) > * sees write activity when frozen is set to SB_FREEZE_COMPLETE. > */ > sb->s_writers.frozen = SB_FREEZE_COMPLETE; > + sb_freeze_release(sb); > up_write(&sb->s_umount); > return 0; > } > @@ -1355,11 +1363,14 @@ int thaw_super(struct super_block *sb) > goto out; > } > > + sb_freeze_acquire(sb); > + > if (sb->s_op->unfreeze_fs) { > error = sb->s_op->unfreeze_fs(sb); > if (error) { > printk(KERN_ERR > "VFS:Filesystem thaw failed\n"); > + sb_freeze_release(sb); > up_write(&sb->s_umount); > return error; > } > -- > 1.5.5.1 > > -- Jan Kara SUSE Labs, CR