From: Brian Foster <bfoster@redhat.com>
To: Pavel Reichl <preichl@redhat.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v11 4/4] xfs: replace mrlock_t with rw_semaphores
Date: Mon, 12 Oct 2020 12:04:12 -0400 [thread overview]
Message-ID: <20201012160412.GK917726@bfoster> (raw)
In-Reply-To: <20201009195515.82889-5-preichl@redhat.com>
On Fri, Oct 09, 2020 at 09:55:15PM +0200, 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>
> ---
> fs/xfs/libxfs/xfs_btree.c | 24 ++++++++++++
> fs/xfs/mrlock.h | 78 ---------------------------------------
> fs/xfs/xfs_inode.c | 50 +++++++++++++------------
> 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, 57 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..d1fdb180f7ae 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
...
> @@ -2863,8 +2875,20 @@ 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 that we
> + * will be transferring the ilock from this thread to the
> + * worker.
> + */
Can we update this comment to explain why we need to do this? E.g., I'm
assuming there's a lockdep splat somewhere down in the split worker
without it, but it's not immediately clear where and so it might not be
obvious if we're ever able to remove this.
> + rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_);
> queue_work(xfs_alloc_wq, &args.work);
> wait_for_completion(&done);
> + /*
> + * Update lockdep's lock ownership information to point to
> + * this thread as the lock owner now that the worker item is
> + * done.
> + */
> + 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/xfs_inode.c b/fs/xfs/xfs_inode.c
> index a3baec1c5bcf..213a4a947854 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
...
> @@ -384,16 +385,17 @@ 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)) {
Can we add a similar assert for this case as we have for the others?
Otherwise the rest looks fairly straightforward to me.
Brian
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 77776af75c77..896a088989f2 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 80a13c8561d8..66cca3e599c7 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 ab737fed7b12..ba37217f86d2 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 71ac6c1cdc36..00be9cfa29fa 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-10-12 16:04 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-09 19:55 [PATCH v11 0/4] xfs: Remove wrappers for some semaphores Pavel Reichl
2020-10-09 19:55 ` [PATCH v11 1/4] xfs: Refactor xfs_isilocked() Pavel Reichl
2020-10-12 16:03 ` Brian Foster
2020-10-12 21:28 ` Darrick J. Wong
2020-10-13 11:04 ` Brian Foster
2020-10-14 21:04 ` Pavel Reichl
2020-10-15 10:32 ` Brian Foster
2020-10-15 8:20 ` Christoph Hellwig
2020-10-09 19:55 ` [PATCH v11 2/4] xfs: clean up whitespace in xfs_isilocked() calls Pavel Reichl
2020-10-12 16:03 ` Brian Foster
2020-10-15 8:17 ` Christoph Hellwig
2020-10-09 19:55 ` [PATCH v11 3/4] xfs: xfs_isilocked() can only check a single lock type Pavel Reichl
2020-10-12 16:03 ` Brian Foster
2020-10-15 8:17 ` Christoph Hellwig
2020-10-09 19:55 ` [PATCH v11 4/4] xfs: replace mrlock_t with rw_semaphores Pavel Reichl
2020-10-12 16:04 ` Brian Foster [this message]
2020-10-12 20:44 ` Pavel Reichl
2020-10-13 11:04 ` Brian Foster
2020-10-13 13:39 ` Pavel Reichl
2020-10-13 13:49 ` Brian Foster
2020-10-12 21:02 ` Pavel Reichl
2020-10-12 21:30 ` Darrick J. Wong
2020-10-13 11:07 ` Brian Foster
2020-10-15 8:21 ` Christoph Hellwig
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=20201012160412.GK917726@bfoster \
--to=bfoster@redhat.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).