From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wu Fengguang <fengguang.wu@intel.com>
Subject: Re: [PATCH 20/45] NFS: introduce writeback wait queue
Date: Wed, 7 Oct 2009 17:52:53 +0800
Message-ID: <20091007095253.GE5646@localhost>
References: <20091007073818.318088777@intel.com> <20091007074903.749770316@intel.com> <1254905600.26976.218.camel@twins> <20091007090722.GA5646@localhost> <20091007091705.GO30316@wotan.suse.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>,
	Andrew Morton <akpm@linux-foundation.org>,
	Theodore Tso <tytso@mit.edu>,
	Christoph Hellwig <hch@infradead.org>,
	Dave Chinner <david@fromorbit.com>,
	Chris Mason <chris.mason@oracle.com>,
	"Li, Shaohua" <shaohua.li@intel.com>,
	Myklebust Trond <Trond.Myklebust@netapp.com>,
	"jens.axboe@oracle.com" <jens.axboe@oracle.com>,
	Jan Kara <jack@suse.cz>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
To: Nick Piggin <npiggin@suse.de>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mga14.intel.com ([143.182.124.37]:4943 "EHLO mga14.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751355AbZJGJxp (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Wed, 7 Oct 2009 05:53:45 -0400
Content-Disposition: inline
In-Reply-To: <20091007091705.GO30316@wotan.suse.de>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Wed, Oct 07, 2009 at 05:17:05PM +0800, Nick Piggin wrote:
> On Wed, Oct 07, 2009 at 05:07:22PM +0800, Wu Fengguang wrote:
> > On Wed, Oct 07, 2009 at 04:53:20PM +0800, Peter Zijlstra wrote:
> > > wakeup implies a full memory barrier.
> > 
> > If so, this smp_mb__after_clear_bit() line is also not necessary?
> > 
> > void clear_bdi_congested(struct backing_dev_info *bdi, int sync)
> > {
> > //...
> >         clear_bit(bit, &bdi->state);
> >         smp_mb__after_clear_bit();
> >         if (waitqueue_active(wqh))
> >                 wake_up(wqh);
> 
> Typically in these patterns you do need a barrier. You need to
> ensure the load to check the waitqueue is performed after the
> clear_bit.
> 
> The other side does:
> add_to_waitqueue
> set_current_state /* has mb() in it */ 
> if (test_bit())
>    schedule()

Thanks, just moved the mb() up to follow clear_bit():

                if (test_bit(BDI_sync_congested, &bdi->state)) {
                        clear_bdi_congested(bdi, BLK_RW_SYNC);
                        smp_mb__after_clear_bit();
                }
                if (waitqueue_active(&wqh[BLK_RW_SYNC]))
                        wake_up(&wqh[BLK_RW_SYNC]);

Thanks,
Fengguang