All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: linux-kernel@vger.kernel.org, xfs@oss.sgi.com
Subject: Re: [PATCH] xfs: clarify lock ordering comment
Date: Fri, 9 Oct 2015 09:24:50 +1100	[thread overview]
Message-ID: <20151008222450.GH32150@dastard> (raw)
In-Reply-To: <1444341481-14139-1-git-send-email-ross.zwisler@linux.intel.com>

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... ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

WARNING: multiple messages have this Message-ID (diff)
From: Dave Chinner <david@fromorbit.com>
To: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: linux-kernel@vger.kernel.org, xfs@oss.sgi.com
Subject: Re: [PATCH] xfs: clarify lock ordering comment
Date: Fri, 9 Oct 2015 09:24:50 +1100	[thread overview]
Message-ID: <20151008222450.GH32150@dastard> (raw)
In-Reply-To: <1444341481-14139-1-git-send-email-ross.zwisler@linux.intel.com>

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... ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2015-10-08 22:24 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 [this message]
2015-10-08 22:24   ` Dave Chinner
2015-10-08 23:02   ` Ross Zwisler
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=20151008222450.GH32150@dastard \
    --to=david@fromorbit.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ross.zwisler@linux.intel.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 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.