From: Nathan Chancellor <natechancellor@gmail.com>
To: Nikolay Borisov <nborisov@suse.com>
Cc: linux-btrfs@vger.kernel.org, paulmck@linux.ibm.com,
linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH] btrfs: Hook btrfs' DRW lock to locktorture infrastructure
Date: Mon, 5 Aug 2019 09:36:21 -0700 [thread overview]
Message-ID: <20190805163621.GA94502@archlinux-threadripper> (raw)
In-Reply-To: <20190719084808.5877-1-nborisov@suse.com>
On Fri, Jul 19, 2019 at 11:48:08AM +0300, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>
> Hello Paul,
>
> Here is the code I used to test the DRW lock via the lock torture infrastructure.
> It's rather ugly but got the job done for me. It's definitely not in a mergeable
> form. At the very least I think including btrfs headers constitutes a violation
> of separation of different subsystems. Would it be acceptable to guard them
> behind something like "#if BTRFS && BTRFS_DEBUG" ?
>
> I'm really posting this just for posterity/provenance purposes. I'm fine with
> dropping the patch.
>
>
> fs/btrfs/locking.h | 1 +
> kernel/locking/locktorture.c | 77 +++++++++++++++++++++++++++++++++++-
> 2 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h
> index 44378c65f843..27627d4fd3a9 100644
> --- a/fs/btrfs/locking.h
> +++ b/fs/btrfs/locking.h
> @@ -9,6 +9,7 @@
> #include <linux/atomic.h>
> #include <linux/wait.h>
> #include <linux/percpu_counter.h>
> +#include "extent_io.h"
>
> #define BTRFS_WRITE_LOCK 1
> #define BTRFS_READ_LOCK 2
> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> index 80a463d31a8d..774e10a25876 100644
> --- a/kernel/locking/locktorture.c
> +++ b/kernel/locking/locktorture.c
> @@ -30,6 +30,8 @@
> #include <linux/slab.h>
> #include <linux/percpu-rwsem.h>
> #include <linux/torture.h>
> +#include "../../fs/btrfs/ctree.h"
> +#include "../../fs/btrfs/locking.h"
>
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Paul E. McKenney <paulmck@linux.ibm.com>");
> @@ -85,6 +87,7 @@ struct lock_torture_ops {
>
> unsigned long flags; /* for irq spinlocks */
> const char *name;
> + bool multiple;
> };
>
> struct lock_torture_cxt {
> @@ -600,6 +603,7 @@ static void torture_percpu_rwsem_up_read(void) __releases(pcpu_rwsem)
> percpu_up_read(&pcpu_rwsem);
> }
>
> +
> static struct lock_torture_ops percpu_rwsem_lock_ops = {
> .init = torture_percpu_rwsem_init,
> .writelock = torture_percpu_rwsem_down_write,
> @@ -612,6 +616,76 @@ static struct lock_torture_ops percpu_rwsem_lock_ops = {
> .name = "percpu_rwsem_lock"
> };
>
> +static struct btrfs_drw_lock torture_drw_lock;
> +
> +void torture_drw_init(void)
> +{
> + BUG_ON(btrfs_drw_lock_init(&torture_drw_lock));
> +}
> +
> +static int torture_drw_write_lock(void) __acquires(torture_drw_lock)
> +{
> + btrfs_drw_write_lock(&torture_drw_lock);
> + return 0;
> +}
> +
> +static void torture_drw_write_unlock(void) __releases(torture_drw_lock)
> +{
> + btrfs_drw_write_unlock(&torture_drw_lock);
> +}
> +
> +static int torture_drw_read_lock(void) __acquires(torture_drw_lock)
> +{
> + btrfs_drw_read_lock(&torture_drw_lock);
> + return 0;
> +}
> +
> +static void torture_drw_read_unlock(void) __releases(torture_drw_lock)
> +{
> + btrfs_drw_read_unlock(&torture_drw_lock);
> +}
> +
> +static void torture_drw_write_delay(struct torture_random_state *trsp)
> +{
> + const unsigned long longdelay_ms = 100;
> +
> + /* We want a long delay occasionally to force massive contention. */
> + if (!(torture_random(trsp) %
> + (cxt.nrealwriters_stress * 2000 * longdelay_ms)))
> + mdelay(longdelay_ms * 10);
> + else
> + mdelay(longdelay_ms / 10);
> + if (!(torture_random(trsp) % (cxt.nrealwriters_stress * 20000)))
> + torture_preempt_schedule(); /* Allow test to be preempted. */
> +}
> +
> +static void torture_drw_read_delay(struct torture_random_state *trsp)
> +{
> + const unsigned long longdelay_ms = 100;
> +
> + /* We want a long delay occasionally to force massive contention. */
> + if (!(torture_random(trsp) %
> + (cxt.nrealreaders_stress * 2000 * longdelay_ms)))
> + mdelay(longdelay_ms * 2);
> + else
> + mdelay(longdelay_ms / 2);
> + if (!(torture_random(trsp) % (cxt.nrealreaders_stress * 20000)))
> + torture_preempt_schedule(); /* Allow test to be preempted. */
> +}
> +
> +static struct lock_torture_ops btrfs_drw_lock_ops = {
> + .init = torture_drw_init,
> + .writelock = torture_drw_write_lock,
> + .write_delay = torture_drw_write_delay,
> + .task_boost = torture_boost_dummy,
> + .writeunlock = torture_drw_write_unlock,
> + .readlock = torture_drw_read_lock,
> + .read_delay = torture_drw_read_delay, /* figure what to do with this */
> + .readunlock = torture_drw_read_unlock,
> + .multiple = true,
> + .name = "btrfs_drw_lock"
> +};
> +
> /*
> * Lock torture writer kthread. Repeatedly acquires and releases
> * the lock, checking for duplicate acquisitions.
> @@ -630,7 +704,7 @@ static int lock_torture_writer(void *arg)
>
> cxt.cur_ops->task_boost(&rand);
> cxt.cur_ops->writelock();
> - if (WARN_ON_ONCE(lock_is_write_held))
> + if (!cxt.cur_ops->multiple && WARN_ON_ONCE(lock_is_write_held))
> lwsp->n_lock_fail++;
> lock_is_write_held = 1;
> if (WARN_ON_ONCE(lock_is_read_held))
> @@ -852,6 +926,7 @@ static int __init lock_torture_init(void)
> #endif
> &rwsem_lock_ops,
> &percpu_rwsem_lock_ops,
> + &btrfs_drw_lock_ops
> };
>
> if (!torture_init_begin(torture_type, verbose))
> --
> 2.17.1
>
Looks like this is in next-20190805 and causes a link time error when
CONFIG_BTRFS_FS is unset:
LD vmlinux.o
MODPOST vmlinux.o
MODINFO modules.builtin.modinfo
ld.lld: error: undefined symbol: btrfs_drw_lock_init
>>> referenced by locktorture.c
>>> locking/locktorture.o:(torture_drw_init) in archive kernel/built-in.a
ld.lld: error: undefined symbol: btrfs_drw_write_lock
>>> referenced by locktorture.c
>>> locking/locktorture.o:(torture_drw_write_lock) in archive kernel/built-in.a
ld.lld: error: undefined symbol: btrfs_drw_write_unlock
>>> referenced by locktorture.c
>>> locking/locktorture.o:(torture_drw_write_unlock) in archive kernel/built-in.a
ld.lld: error: undefined symbol: btrfs_drw_read_lock
>>> referenced by locktorture.c
>>> locking/locktorture.o:(torture_drw_read_lock) in archive kernel/built-in.a
ld.lld: error: undefined symbol: btrfs_drw_read_unlock
>>> referenced by locktorture.c
>>> locking/locktorture.o:(torture_drw_read_unlock) in archive kernel/built-in.a
If this commit is to remain around, there should probably be static
inline stubs in fs/btrfs/locking.h. Apologies if this has already been
reported, I still see the commit in the btrfs for-next branch.
Cheers,
Nathan
next prev parent reply other threads:[~2019-08-05 16:36 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-19 8:39 [PATCH v2 0/2] Refactor snapshot vs nocow writers locking Nikolay Borisov
2019-07-19 8:39 ` [PATCH v2 1/2] btrfs: Implement DRW lock Nikolay Borisov
2019-07-19 8:39 ` [PATCH v2 2/2] btrfs: convert snapshot/nocow exlcusion to drw lock Nikolay Borisov
2019-07-19 8:48 ` [RFC PATCH] btrfs: Hook btrfs' DRW lock to locktorture infrastructure Nikolay Borisov
2019-08-05 16:36 ` Nathan Chancellor [this message]
2019-08-05 18:17 ` David Sterba
2019-07-29 14:13 ` [PATCH v2 0/2] Refactor snapshot vs nocow writers locking Valentin Schneider
2019-07-29 15:33 ` Catalin Marinas
2019-07-29 16:32 ` Valentin Schneider
2019-07-30 11:03 ` Valentin Schneider
2019-07-30 12:11 ` Nikolay Borisov
2019-07-30 13:36 ` Valentin Schneider
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=20190805163621.GA94502@archlinux-threadripper \
--to=natechancellor@gmail.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nborisov@suse.com \
--cc=paulmck@linux.ibm.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).