All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ross Zwisler <ross.zwisler@linux.intel.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>,
	linux-kernel@vger.kernel.org, xfs@oss.sgi.com
Subject: Re: [PATCH] xfs: clarify lock ordering comment
Date: Thu, 8 Oct 2015 17:02:42 -0600	[thread overview]
Message-ID: <20151008230242.GA14219@linux.intel.com> (raw)
In-Reply-To: <20151008222450.GH32150@dastard>

On Fri, Oct 09, 2015 at 09:24:50AM +1100, Dave Chinner wrote:
> On Thu, Oct 08, 2015 at 03:58:01PM -0600, Ross Zwisler wrote:
> > Replace "i_mmap_lock" with "mmap_lock" in the lock ordering comment above
> > xfs_filemap_page_mkwrite().  The lock in question is actually the
> > XFS_MMAPLOCK_SHARED rw_semaphore (no leading "i"), and this comment is
> 
> struct xfs_inode {
> ....
> 	        mrlock_t                i_mmaplock;     /* inode mmap IO lock */
> ....
> 
> > easily confused with the "i_mmap_lock_[read|write]" functions that operate
> > on struct address_space->i_mmap_rwsem.  This clarification is especially
> > important because address_space->i_mmap_rwsem is taken down in the DAX
> > code as part of this fault path.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > ---
> >  fs/xfs/xfs_file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index f429662..b190033 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1477,7 +1477,7 @@ xfs_file_llseek(
> >   *
> >   * mmap_sem (MM)
> >   *   sb_start_pagefault(vfs, freeze)
> > - *     i_mmap_lock (XFS - truncate serialisation)
> > + *     mmap_lock (XFS - truncate serialisation)
> 
> As per above, the XFS lock is "i_mmaplock"...
> 
> The lock names are annotated with the subsystem the lock belongs to
> to avoid this confusion. Along with the lock ordering (inside
> sb_start_pagefault) this should indicate that it's not the
> "i_mmap_lock (MM - vma serialisation)" lock... ;)

Ah, that makes sense, thanks. :)

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

WARNING: multiple messages have this Message-ID (diff)
From: Ross Zwisler <ross.zwisler@linux.intel.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>,
	linux-kernel@vger.kernel.org, xfs@oss.sgi.com
Subject: Re: [PATCH] xfs: clarify lock ordering comment
Date: Thu, 8 Oct 2015 17:02:42 -0600	[thread overview]
Message-ID: <20151008230242.GA14219@linux.intel.com> (raw)
In-Reply-To: <20151008222450.GH32150@dastard>

On Fri, Oct 09, 2015 at 09:24:50AM +1100, Dave Chinner wrote:
> On Thu, Oct 08, 2015 at 03:58:01PM -0600, Ross Zwisler wrote:
> > Replace "i_mmap_lock" with "mmap_lock" in the lock ordering comment above
> > xfs_filemap_page_mkwrite().  The lock in question is actually the
> > XFS_MMAPLOCK_SHARED rw_semaphore (no leading "i"), and this comment is
> 
> struct xfs_inode {
> ....
> 	        mrlock_t                i_mmaplock;     /* inode mmap IO lock */
> ....
> 
> > easily confused with the "i_mmap_lock_[read|write]" functions that operate
> > on struct address_space->i_mmap_rwsem.  This clarification is especially
> > important because address_space->i_mmap_rwsem is taken down in the DAX
> > code as part of this fault path.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > ---
> >  fs/xfs/xfs_file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index f429662..b190033 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1477,7 +1477,7 @@ xfs_file_llseek(
> >   *
> >   * mmap_sem (MM)
> >   *   sb_start_pagefault(vfs, freeze)
> > - *     i_mmap_lock (XFS - truncate serialisation)
> > + *     mmap_lock (XFS - truncate serialisation)
> 
> As per above, the XFS lock is "i_mmaplock"...
> 
> The lock names are annotated with the subsystem the lock belongs to
> to avoid this confusion. Along with the lock ordering (inside
> sb_start_pagefault) this should indicate that it's not the
> "i_mmap_lock (MM - vma serialisation)" lock... ;)

Ah, that makes sense, thanks. :)

  reply	other threads:[~2015-10-08 23:02 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-08 21:58 [PATCH] xfs: clarify lock ordering comment Ross Zwisler
2015-10-08 21:58 ` Ross Zwisler
2015-10-08 22:24 ` Dave Chinner
2015-10-08 22:24   ` Dave Chinner
2015-10-08 23:02   ` Ross Zwisler [this message]
2015-10-08 23:02     ` Ross Zwisler

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=20151008230242.GA14219@linux.intel.com \
    --to=ross.zwisler@linux.intel.com \
    --cc=david@fromorbit.com \
    --cc=linux-kernel@vger.kernel.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.