From: Ben Myers <bpm@sgi.com>
To: Alex Elder <elder@inktank.com>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 1/2] xfs: memory barrier before wake_up_bit()
Date: Thu, 7 Feb 2013 09:44:37 -0600 [thread overview]
Message-ID: <20130207154436.GC22182@sgi.com> (raw)
In-Reply-To: <511061CD.8070206@inktank.com>
On Mon, Feb 04, 2013 at 07:35:09PM -0600, Alex Elder wrote:
> On 02/04/2013 05:06 PM, Dave Chinner wrote:
> > On Mon, Feb 04, 2013 at 10:13:11AM -0600, Alex Elder wrote:
> >> In xfs_ifunlock() there is a call to wake_up_bit() after clearing
> >> the flush lock on the xfs inode. This is not guaranteed to be safe,
> >> as noted in the comments above wake_up_bit() beginning with:
> >>
> >> In order for this to function properly, as it uses
> >> waitqueue_active() internally, some kind of memory
> >> barrier must be done prior to calling this.
> >>
> >> I claim no mastery of the details and subtlety of memory barrier
> >> use, but I believe the issue is that the call to waitqueue_active()
> >> in __wake_up_bit(), could be operating on a value of "wq" that is
> >> out of date. This patch fixes this by inserting a call to smp_mb()
> >> in xfs_iunlock before calling wake_up_bit(), along the lines of
> >> what's done in unlock_new_inode(). A litte more explanation
> >> follows.
> >>
> >>
> >> In __xfs_iflock(), prepare_to_wait_exclusive() adds a wait queue
> >> entry to the end of a bit wait queue before setting the current task
> >> state to UNINTERRUPTIBLE. And although setting the task state
> >> issues a full smp_mb() (which ensures changes made are visible to
> >> the rest of the system at that point) that alone does not guarantee
> >> that other CPUs will instantly avail themselves of the updated
> >> value. A separate CPU needs to issue at least a read barrier in
> >> order to ensure the wq value it uses to determine whether there are
> >> waiters is up-to-date, and waitqueue_active() does not do that.
> >
> > You can probably trim most of this and simply point at the comment
> > describing wake_up_bit()....
>
> Yeah, I know. I just wanted to sort of say what I was
> thinking to get confirmation (or correction). I now
> have a much better understanding of barriers than I did
> before, but there are still corners I haven't wrapped
> my head around.
>
> Ben, please feel free do trim off this stuff as you
> see fit.
Applied.
Thanks Alex!
-Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
next prev parent reply other threads:[~2013-02-07 15:44 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-04 16:12 [PATCH 0/2] xfs: insert memory barriers before wake_up_bit() Alex Elder
2013-02-04 16:13 ` [PATCH 1/2] xfs: memory barrier " Alex Elder
2013-02-04 23:06 ` Dave Chinner
2013-02-05 1:35 ` Alex Elder
2013-02-07 15:44 ` Ben Myers [this message]
2013-02-04 16:13 ` [PATCH 2/2] xfs: another " Alex Elder
2013-02-04 23:26 ` Dave Chinner
2013-02-05 1:38 ` Alex Elder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130207154436.GC22182@sgi.com \
--to=bpm@sgi.com \
--cc=elder@inktank.com \
--cc=xfs@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox