From: Eric Sandeen <sandeen@sandeen.net>
To: Christoph Hellwig <hch@infradead.org>, Pavel Reichl <preichl@redhat.com>
Cc: linux-xfs@vger.kernel.org, Dave Chinner <dchinner@redhat.com>
Subject: Re: [PATCH v2 2/7] xfs: Update checking excl. locks for ilock
Date: Tue, 4 Feb 2020 10:08:45 -0600 [thread overview]
Message-ID: <80f1173e-9181-c8cc-c06f-cbbfaa46a138@sandeen.net> (raw)
In-Reply-To: <20200204062118.GB2910@infradead.org>
On 2/4/20 12:21 AM, Christoph Hellwig wrote:
>> - ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>> + ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
>
> I think this is a very bad interface. Either we keep our good old
> xfs_isilocked that just operates on the inode and lock flags, or
> we use something that gets the actual lock passed. But an interface
> that encodes the lock in both the function called and the flags, and
> one that doesn't follow neither the XFS lock flags conventions nor
> the core kernel convention is just not very useful.
I think this came out of Dave's suggestion on the previous patchset,
but I agree with you Chrisoph. Even if there is a future reason to
split it out into a function for each type, I don't see a reason to
do it now, and this interface is awkward.
I'd prefer to keep xfs_isilocked() with the current calling convention and
just change its internals to use lockdep. Dave spotted a bug in the
current implementation, but I think that can be fixed.
Splitting out the 3 lock testing functions seems to me like complexity
creep that doesn't need to be in this series.
Dave, thoughts?
-Eric
next prev parent reply other threads:[~2020-02-04 16:08 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-03 17:58 [PATCH v2 0/7] xfs: Remove wrappers for some semaphores Pavel Reichl
2020-02-03 17:58 ` [PATCH v2 1/7] xfs: Add xfs_is_{i,io,mmap}locked functions Pavel Reichl
2020-02-03 21:00 ` Chaitanya Kulkarni
2020-02-03 23:15 ` Dave Chinner
2020-02-03 23:45 ` Eric Sandeen
2020-02-04 6:19 ` Christoph Hellwig
2020-02-03 17:58 ` [PATCH v2 2/7] xfs: Update checking excl. locks for ilock Pavel Reichl
2020-02-03 23:16 ` Dave Chinner
2020-02-04 6:21 ` Christoph Hellwig
2020-02-04 16:08 ` Eric Sandeen [this message]
2020-02-04 21:06 ` Dave Chinner
2020-02-04 22:20 ` Eric Sandeen
2020-02-03 17:58 ` [PATCH v2 3/7] xfs: Update checking read or write " Pavel Reichl
2020-02-03 23:18 ` Dave Chinner
2020-02-03 17:58 ` [PATCH v2 4/7] xfs: Update checking for iolock Pavel Reichl
2020-02-03 23:24 ` Dave Chinner
2020-02-03 17:58 ` [PATCH v2 5/7] xfs: Update checking for mmaplock Pavel Reichl
2020-02-03 23:25 ` Dave Chinner
2020-02-03 17:58 ` [PATCH v2 6/7] xfs: update excl. lock check for IOLOCK and ILOCK Pavel Reichl
2020-02-03 23:35 ` Dave Chinner
2020-02-03 17:58 ` [PATCH v2 7/7] xfs: Replace mrlock_t by rw_semaphore Pavel Reichl
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=80f1173e-9181-c8cc-c06f-cbbfaa46a138@sandeen.net \
--to=sandeen@sandeen.net \
--cc=dchinner@redhat.com \
--cc=hch@infradead.org \
--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).