From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Pavel Reichl <preichl@redhat.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v13 4/4] xfs: replace mrlock_t with rw_semaphores
Date: Tue, 3 Nov 2020 11:41:18 -0800 [thread overview]
Message-ID: <20201103194118.GL7123@magnolia> (raw)
In-Reply-To: <20201102194135.174806-5-preichl@redhat.com>
On Mon, Nov 02, 2020 at 08:41:35PM +0100, Pavel Reichl wrote:
> Remove mrlock_t as it does not provide any extra value over
> rw_semaphores. Make i_lock and i_mmaplock native rw_semaphores and
> replace mr*() functions with native rwsem calls.
>
> Release the lock in xfs_btree_split() just before the work-queue
> executing xfs_btree_split_worker() is scheduled and make
> xfs_btree_split_worker() to acquire the lock as a first thing and
> release it just before returning from the function. This it done so the
> ownership of the lock is transfered between kernel threads and thus
> lockdep won't complain about lock being held by a different kernel
> thread.
>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> ---
> fs/xfs/libxfs/xfs_btree.c | 16 ++++++++
> fs/xfs/mrlock.h | 78 ---------------------------------------
> fs/xfs/xfs_inode.c | 52 ++++++++++++++------------
> fs/xfs/xfs_inode.h | 4 +-
> fs/xfs/xfs_iops.c | 4 +-
> fs/xfs/xfs_linux.h | 2 +-
> fs/xfs/xfs_super.c | 6 +--
> 7 files changed, 51 insertions(+), 111 deletions(-)
> delete mode 100644 fs/xfs/mrlock.h
>
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index 2d25bab68764..181d5797c97b 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -2816,6 +2816,10 @@ xfs_btree_split_worker(
> unsigned long pflags;
> unsigned long new_pflags = PF_MEMALLOC_NOFS;
>
> + /*
> + * Tranfer lock ownership to workqueue task.
"Transfer", not "tranfer". I'll just fix that so you don't have to send
this yet again.
MR. LOCK is dead, long live MR. LOCK!
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> + */
> + rwsem_acquire(&args->cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_);
> /*
> * we are in a transaction context here, but may also be doing work
> * in kswapd context, and hence we may need to inherit that state
> @@ -2829,6 +2833,7 @@ xfs_btree_split_worker(
>
> args->result = __xfs_btree_split(args->cur, args->level, args->ptrp,
> args->key, args->curp, args->stat);
> + rwsem_release(&args->cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_);
> complete(args->done);
>
> current_restore_flags_nested(&pflags, new_pflags);
> @@ -2863,8 +2868,19 @@ xfs_btree_split(
> args.done = &done;
> args.kswapd = current_is_kswapd();
> INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker);
> + /*
> + * Update lockdep's ownership information to reflect transfer of the
> + * ilock from the current task to the worker. Otherwise assertions that
> + * the lock is held (such as when logging the inode) might fail due to
> + * incorrect task owner state.
> + */
> + rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_);
> queue_work(xfs_alloc_wq, &args.work);
> wait_for_completion(&done);
> + /*
> + * Tranfer lock ownership back to the thread.
> + */
> + rwsem_acquire(&cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_);
> destroy_work_on_stack(&args.work);
> return args.result;
> }
> diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
> deleted file mode 100644
> index 79155eec341b..000000000000
> --- a/fs/xfs/mrlock.h
> +++ /dev/null
> @@ -1,78 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Copyright (c) 2000-2006 Silicon Graphics, Inc.
> - * All Rights Reserved.
> - */
> -#ifndef __XFS_SUPPORT_MRLOCK_H__
> -#define __XFS_SUPPORT_MRLOCK_H__
> -
> -#include <linux/rwsem.h>
> -
> -typedef struct {
> - struct rw_semaphore mr_lock;
> -#if defined(DEBUG) || defined(XFS_WARN)
> - int mr_writer;
> -#endif
> -} mrlock_t;
> -
> -#if defined(DEBUG) || defined(XFS_WARN)
> -#define mrinit(mrp, name) \
> - do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0)
> -#else
> -#define mrinit(mrp, name) \
> - do { init_rwsem(&(mrp)->mr_lock); } while (0)
> -#endif
> -
> -#define mrlock_init(mrp, t,n,s) mrinit(mrp, n)
> -#define mrfree(mrp) do { } while (0)
> -
> -static inline void mraccess_nested(mrlock_t *mrp, int subclass)
> -{
> - down_read_nested(&mrp->mr_lock, subclass);
> -}
> -
> -static inline void mrupdate_nested(mrlock_t *mrp, int subclass)
> -{
> - down_write_nested(&mrp->mr_lock, subclass);
> -#if defined(DEBUG) || defined(XFS_WARN)
> - mrp->mr_writer = 1;
> -#endif
> -}
> -
> -static inline int mrtryaccess(mrlock_t *mrp)
> -{
> - return down_read_trylock(&mrp->mr_lock);
> -}
> -
> -static inline int mrtryupdate(mrlock_t *mrp)
> -{
> - if (!down_write_trylock(&mrp->mr_lock))
> - return 0;
> -#if defined(DEBUG) || defined(XFS_WARN)
> - mrp->mr_writer = 1;
> -#endif
> - return 1;
> -}
> -
> -static inline void mrunlock_excl(mrlock_t *mrp)
> -{
> -#if defined(DEBUG) || defined(XFS_WARN)
> - mrp->mr_writer = 0;
> -#endif
> - up_write(&mrp->mr_lock);
> -}
> -
> -static inline void mrunlock_shared(mrlock_t *mrp)
> -{
> - up_read(&mrp->mr_lock);
> -}
> -
> -static inline void mrdemote(mrlock_t *mrp)
> -{
> -#if defined(DEBUG) || defined(XFS_WARN)
> - mrp->mr_writer = 0;
> -#endif
> - downgrade_write(&mrp->mr_lock);
> -}
> -
> -#endif /* __XFS_SUPPORT_MRLOCK_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 16d481cf3793..43ecfcb63f99 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -191,14 +191,15 @@ xfs_ilock(
> }
>
> if (lock_flags & XFS_MMAPLOCK_EXCL)
> - mrupdate_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
> + down_write_nested(&ip->i_mmaplock,
> + XFS_MMAPLOCK_DEP(lock_flags));
> else if (lock_flags & XFS_MMAPLOCK_SHARED)
> - mraccess_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
> + down_read_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
>
> if (lock_flags & XFS_ILOCK_EXCL)
> - mrupdate_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
> + down_write_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
> else if (lock_flags & XFS_ILOCK_SHARED)
> - mraccess_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
> + down_read_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
> }
>
> /*
> @@ -242,27 +243,27 @@ xfs_ilock_nowait(
> }
>
> if (lock_flags & XFS_MMAPLOCK_EXCL) {
> - if (!mrtryupdate(&ip->i_mmaplock))
> + if (!down_write_trylock(&ip->i_mmaplock))
> goto out_undo_iolock;
> } else if (lock_flags & XFS_MMAPLOCK_SHARED) {
> - if (!mrtryaccess(&ip->i_mmaplock))
> + if (!down_read_trylock(&ip->i_mmaplock))
> goto out_undo_iolock;
> }
>
> if (lock_flags & XFS_ILOCK_EXCL) {
> - if (!mrtryupdate(&ip->i_lock))
> + if (!down_write_trylock(&ip->i_lock))
> goto out_undo_mmaplock;
> } else if (lock_flags & XFS_ILOCK_SHARED) {
> - if (!mrtryaccess(&ip->i_lock))
> + if (!down_read_trylock(&ip->i_lock))
> goto out_undo_mmaplock;
> }
> return 1;
>
> out_undo_mmaplock:
> if (lock_flags & XFS_MMAPLOCK_EXCL)
> - mrunlock_excl(&ip->i_mmaplock);
> + up_write(&ip->i_mmaplock);
> else if (lock_flags & XFS_MMAPLOCK_SHARED)
> - mrunlock_shared(&ip->i_mmaplock);
> + up_read(&ip->i_mmaplock);
> out_undo_iolock:
> if (lock_flags & XFS_IOLOCK_EXCL)
> up_write(&VFS_I(ip)->i_rwsem);
> @@ -309,14 +310,14 @@ xfs_iunlock(
> up_read(&VFS_I(ip)->i_rwsem);
>
> if (lock_flags & XFS_MMAPLOCK_EXCL)
> - mrunlock_excl(&ip->i_mmaplock);
> + up_write(&ip->i_mmaplock);
> else if (lock_flags & XFS_MMAPLOCK_SHARED)
> - mrunlock_shared(&ip->i_mmaplock);
> + up_read(&ip->i_mmaplock);
>
> if (lock_flags & XFS_ILOCK_EXCL)
> - mrunlock_excl(&ip->i_lock);
> + up_write(&ip->i_lock);
> else if (lock_flags & XFS_ILOCK_SHARED)
> - mrunlock_shared(&ip->i_lock);
> + up_read(&ip->i_lock);
>
> trace_xfs_iunlock(ip, lock_flags, _RET_IP_);
> }
> @@ -335,9 +336,9 @@ xfs_ilock_demote(
> ~(XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL|XFS_ILOCK_EXCL)) == 0);
>
> if (lock_flags & XFS_ILOCK_EXCL)
> - mrdemote(&ip->i_lock);
> + downgrade_write(&ip->i_lock);
> if (lock_flags & XFS_MMAPLOCK_EXCL)
> - mrdemote(&ip->i_mmaplock);
> + downgrade_write(&ip->i_mmaplock);
> if (lock_flags & XFS_IOLOCK_EXCL)
> downgrade_write(&VFS_I(ip)->i_rwsem);
>
> @@ -375,19 +376,22 @@ xfs_isilocked(
> struct xfs_inode *ip,
> uint lock_flags)
> {
> - if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> - if (!(lock_flags & XFS_ILOCK_SHARED))
> - return !!ip->i_lock.mr_writer;
> - return rwsem_is_locked(&ip->i_lock.mr_lock);
> + if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
> + ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
> + return __xfs_rwsem_islocked(&ip->i_lock, lock_flags,
> + XFS_ILOCK_FLAG_SHIFT);
> }
>
> - if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
> - if (!(lock_flags & XFS_MMAPLOCK_SHARED))
> - return !!ip->i_mmaplock.mr_writer;
> - return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
> + if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
> + ASSERT(!(lock_flags &
> + ~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
> + return __xfs_rwsem_islocked(&ip->i_mmaplock, lock_flags,
> + XFS_MMAPLOCK_FLAG_SHIFT);
> }
>
> if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
> + ASSERT(!(lock_flags &
> + ~(XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)));
> return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, lock_flags,
> XFS_IOLOCK_FLAG_SHIFT);
> }
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 1392a9c452ae..66ceb127192e 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -39,8 +39,8 @@ typedef struct xfs_inode {
>
> /* Transaction and locking information. */
> struct xfs_inode_log_item *i_itemp; /* logging information */
> - mrlock_t i_lock; /* inode lock */
> - mrlock_t i_mmaplock; /* inode mmap IO lock */
> + struct rw_semaphore i_lock; /* inode lock */
> + struct rw_semaphore i_mmaplock; /* inode mmap IO lock */
> atomic_t i_pincount; /* inode pin count */
>
> /*
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 5e165456da68..8181f6785a7a 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1336,9 +1336,9 @@ xfs_setup_inode(
> */
> lockdep_set_class(&inode->i_rwsem,
> &inode->i_sb->s_type->i_mutex_dir_key);
> - lockdep_set_class(&ip->i_lock.mr_lock, &xfs_dir_ilock_class);
> + lockdep_set_class(&ip->i_lock, &xfs_dir_ilock_class);
> } else {
> - lockdep_set_class(&ip->i_lock.mr_lock, &xfs_nondir_ilock_class);
> + lockdep_set_class(&ip->i_lock, &xfs_nondir_ilock_class);
> }
>
> /*
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 5b7a1e201559..64e28ec16cf7 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -22,7 +22,6 @@ typedef __u32 xfs_nlink_t;
> #include "xfs_types.h"
>
> #include "kmem.h"
> -#include "mrlock.h"
>
> #include <linux/semaphore.h>
> #include <linux/mm.h>
> @@ -61,6 +60,7 @@ typedef __u32 xfs_nlink_t;
> #include <linux/ratelimit.h>
> #include <linux/rhashtable.h>
> #include <linux/xattr.h>
> +#include <linux/rwsem.h>
>
> #include <asm/page.h>
> #include <asm/div64.h>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e3e229e52512..380ba196b165 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -708,10 +708,8 @@ xfs_fs_inode_init_once(
> atomic_set(&ip->i_pincount, 0);
> spin_lock_init(&ip->i_flags_lock);
>
> - mrlock_init(&ip->i_mmaplock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
> - "xfsino", ip->i_ino);
> - mrlock_init(&ip->i_lock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
> - "xfsino", ip->i_ino);
> + init_rwsem(&ip->i_mmaplock);
> + init_rwsem(&ip->i_lock);
> }
>
> /*
> --
> 2.26.2
>
next prev parent reply other threads:[~2020-11-03 19:41 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-02 19:41 [PATCH v13 0/4] xfs: Remove wrappers for some semaphores Pavel Reichl
2020-11-02 19:41 ` [PATCH v13 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl
2020-11-02 19:41 ` [PATCH v13 2/4] xfs: clean up whitespace in xfs_isilocked() calls Pavel Reichl
2020-11-02 19:41 ` [PATCH v13 3/4] xfs: xfs_isilocked() can only check a single lock type Pavel Reichl
2020-11-02 19:41 ` [PATCH v13 4/4] xfs: replace mrlock_t with rw_semaphores Pavel Reichl
2020-11-03 19:41 ` Darrick J. Wong [this message]
2020-11-04 0:53 ` Darrick J. Wong
2020-11-05 22:08 ` Dave Chinner
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=20201103194118.GL7123@magnolia \
--to=darrick.wong@oracle.com \
--cc=linux-xfs@vger.kernel.org \
--cc=preichl@redhat.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.