From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Pavel Reichl <preichl@redhat.com>
Cc: Brian Foster <bfoster@redhat.com>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH v11 4/4] xfs: replace mrlock_t with rw_semaphores
Date: Mon, 12 Oct 2020 14:30:43 -0700 [thread overview]
Message-ID: <20201012213043.GY6540@magnolia> (raw)
In-Reply-To: <a6afba10-64a2-a30c-94de-e99a324a6114@redhat.com>
On Mon, Oct 12, 2020 at 11:02:51PM +0200, Pavel Reichl wrote:
>
> > ...
> >> @@ -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.
> >
>
> Sure we can! But do we want to?
>
> I think that these asserts are supposed to make sure that only flags
> for one of the inode's locks are used eg. ILOCK, MMAPLOCK or IOLOCK
> but no combination! So if we reach this 3rd condition we already know
> that the flags for ILOCK and MMAPLOCK were not set. However if there's
> possibility for more locks to be added in the future or just for the
> 'code symmetry' purposes - I have no problem to update the code.
It's generally a good idea not to leave logic bombs of the sort where
where the debugging code can bitrot into incorrectness if someone
unwittingly adds another level of locking later.
(That said, I really hope we don't; I already consider it a little
strange to have separate io and mmap locks...)
--D
next prev parent reply other threads:[~2020-10-12 21:30 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
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 [this message]
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=20201012213043.GY6540@magnolia \
--to=darrick.wong@oracle.com \
--cc=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).