lkml.org 
[lkml]   [2020]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V4 09/13] fs/xfs: Add write aops lock to xfs layer
On Thu, Feb 20, 2020 at 04:41:30PM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
>
> XFS requires the use of the aops of an inode to quiesced prior to
> changing it to/from the DAX aops vector.
>
> Take the aops write lock while changing DAX state.
>
> We define a new XFS_DAX_EXCL lock type to carry the lock through to
> transaction completion.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
> ---
> Changes from v3:
> Change locking function names to reflect changes in previous
> patches.
>
> Changes from V2:
> Change name of patch (WAS: fs/xfs: Add lock/unlock state to xfs)
> Remove the xfs specific lock and move to the vfs layer.
> We still use XFS_LOCK_DAX_EXCL to be able to pass this
> flag through to the transaction code. But we no longer
> have a lock specific to xfs. This removes a lot of code
> from the XFS layer, preps us for using this in ext4, and
> is actually more straight forward now that all the
> locking requirements are better known.
>
> Fix locking order comment
> Rework for new 'state' names
> (Other comments on the previous patch are not applicable with
> new patch as much of the code was removed in favor of the vfs
> level lock)
> ---
> fs/xfs/xfs_inode.c | 22 ++++++++++++++++++++--
> fs/xfs/xfs_inode.h | 7 +++++--
> 2 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 35df324875db..5b014c428f0f 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared(
> *
> * Basic locking order:
> *
> - * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
> + * s_dax_sem -> i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
> *
> * mmap_sem locking order:
> *
> * i_rwsem -> page lock -> mmap_sem
> - * mmap_sem -> i_mmap_lock -> page_lock
> + * s_dax_sem -> mmap_sem -> i_mmap_lock -> page_lock
> *
> * The difference in mmap_sem locking order mean that we cannot hold the
> * i_mmap_lock over syscall based read(2)/write(2) based IO. These IO paths can
> @@ -182,6 +182,9 @@ xfs_ilock(
> (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
> ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_SUBCLASS_MASK)) == 0);
>
> + if (lock_flags & XFS_DAX_EXCL)
> + inode_aops_down_write(VFS_I(ip));

I largely don't see the point of adding this to xfs_ilock/iunlock.

It's only got one caller, so I don't see much point in adding it to
an interface that has over a hundred other call sites that don't
need or use this lock. just open code it where it is needed in the
ioctl code.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com

\
 
 \ /
  Last update: 2020-02-24 01:36    [W:0.264 / U:13.768 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site