* [PATCH V2 1/5] xfsprogs: introduce liburcu support
2021-09-24 14:09 [PATCH V2 0/5] xfsprogs: generic serialisation primitives Chandan Babu R
@ 2021-09-24 14:09 ` Chandan Babu R
2021-09-24 21:51 ` Eric Sandeen
2021-09-29 20:46 ` Eric Sandeen
2021-09-24 14:09 ` [PATCH V2 2/5] libxfs: add spinlock_t wrapper Chandan Babu R
` (3 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: Chandan Babu R @ 2021-09-24 14:09 UTC (permalink / raw)
To: linux-xfs; +Cc: Dave Chinner, david, sandeen, djwong, Chandan Babu R
From: Dave Chinner <dchinner@redhat.com>
The upcoming buffer cache rework/kerenl sync-up requires atomic
variables. I could use C++11 atomics build into GCC, but they are a
pain to work with and shoe-horn into the kernel atomic variable API.
Much easier is to introduce a dependency on liburcu - the userspace
RCU library. This provides atomic variables that very closely match
the kernel atomic variable API, and it provides a very similar
memory model and memory barrier support to the kernel. And we get
RCU support that has an identical interface to the kernel and works
the same way.
Hence kernel code written with RCU algorithms and atomic variables
will just slot straight into the userspace xfsprogs code without us
having to think about whether the lockless algorithms will work in
userspace or not. This reduces glue and hoop jumping, and gets us
a step closer to having the entire userspace libxfs code MT safe.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
[chandan.babu@oracle.com: Add m4 macros to detect availability of liburcu]
Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
---
configure.ac | 3 +++
copy/Makefile | 3 ++-
copy/xfs_copy.c | 3 +++
db/Makefile | 3 ++-
debian/control | 2 +-
growfs/Makefile | 3 ++-
include/builddefs.in | 4 +++-
include/platform_defs.h.in | 1 +
libfrog/workqueue.c | 3 +++
libxfs/init.c | 3 +++
libxfs/libxfs_priv.h | 3 +--
logprint/Makefile | 3 ++-
m4/Makefile | 1 +
m4/package_urcu.m4 | 24 ++++++++++++++++++++++++
mdrestore/Makefile | 3 ++-
mkfs/Makefile | 2 +-
repair/Makefile | 2 +-
repair/prefetch.c | 9 +++++++--
repair/progress.c | 4 +++-
scrub/Makefile | 3 ++-
scrub/progress.c | 2 ++
21 files changed, 69 insertions(+), 15 deletions(-)
create mode 100644 m4/package_urcu.m4
diff --git a/configure.ac b/configure.ac
index 56871745..61c6cf40 100644
--- a/configure.ac
+++ b/configure.ac
@@ -154,6 +154,9 @@ AC_PACKAGE_NEED_UUIDCOMPARE
AC_PACKAGE_NEED_PTHREAD_H
AC_PACKAGE_NEED_PTHREADMUTEXINIT
+AC_PACKAGE_NEED_URCU_H
+AC_PACKAGE_NEED_RCU_SET_POINTER_SYM
+
AC_HAVE_FADVISE
AC_HAVE_MADVISE
AC_HAVE_MINCORE
diff --git a/copy/Makefile b/copy/Makefile
index 449b235f..1b00cd0d 100644
--- a/copy/Makefile
+++ b/copy/Makefile
@@ -9,7 +9,8 @@ LTCOMMAND = xfs_copy
CFILES = xfs_copy.c
HFILES = xfs_copy.h
-LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBPTHREAD) $(LIBRT)
+LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBPTHREAD) $(LIBRT) \
+ $(LIBURCU)
LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBFROG)
LLDFLAGS = -static-libtool-libs
diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index fc7d225f..f5eff969 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -110,6 +110,7 @@ do_message(int flags, int code, const char *fmt, ...)
fprintf(stderr,
_("Aborting XFS copy -- logfile error -- reason: %s\n"),
strerror(errno));
+ rcu_unregister_thread();
pthread_exit(NULL);
}
}
@@ -224,6 +225,7 @@ begin_reader(void *arg)
{
thread_args *args = arg;
+ rcu_register_thread();
for (;;) {
pthread_mutex_lock(&args->wait);
if (do_write(args, NULL))
@@ -243,6 +245,7 @@ handle_error:
if (--glob_masks.num_working == 0)
pthread_mutex_unlock(&mainwait);
pthread_mutex_unlock(&glob_masks.mutex);
+ rcu_unregister_thread();
pthread_exit(NULL);
return NULL;
}
diff --git a/db/Makefile b/db/Makefile
index beafb105..5c017898 100644
--- a/db/Makefile
+++ b/db/Makefile
@@ -18,7 +18,8 @@ CFILES = $(HFILES:.h=.c) btdump.c btheight.c convert.c info.c namei.c \
timelimit.c
LSRCFILES = xfs_admin.sh xfs_ncheck.sh xfs_metadump.sh
-LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
+LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) \
+ $(LIBURCU)
LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBFROG)
LLDFLAGS += -static-libtool-libs
diff --git a/debian/control b/debian/control
index e4ec897c..71c08167 100644
--- a/debian/control
+++ b/debian/control
@@ -3,7 +3,7 @@ Section: admin
Priority: optional
Maintainer: XFS Development Team <linux-xfs@vger.kernel.org>
Uploaders: Nathan Scott <nathans@debian.org>, Anibal Monsalve Salazar <anibal@debian.org>, Bastian Germann <bastiangermann@fishpost.de>
-Build-Depends: libinih-dev (>= 53), uuid-dev, dh-autoreconf, debhelper (>= 5), gettext, libtool, libedit-dev, libblkid-dev (>= 2.17), linux-libc-dev, libdevmapper-dev, libattr1-dev, libicu-dev, pkg-config
+Build-Depends: libinih-dev (>= 53), uuid-dev, dh-autoreconf, debhelper (>= 5), gettext, libtool, libedit-dev, libblkid-dev (>= 2.17), linux-libc-dev, libdevmapper-dev, libattr1-dev, libicu-dev, pkg-config, liburcu-dev
Standards-Version: 4.0.0
Homepage: https://xfs.wiki.kernel.org/
diff --git a/growfs/Makefile b/growfs/Makefile
index a107d348..08601de7 100644
--- a/growfs/Makefile
+++ b/growfs/Makefile
@@ -9,7 +9,8 @@ LTCOMMAND = xfs_growfs
CFILES = xfs_growfs.c
-LLDLIBS = $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
+LLDLIBS = $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) \
+ $(LIBURCU)
ifeq ($(ENABLE_EDITLINE),yes)
LLDLIBS += $(LIBEDITLINE) $(LIBTERMCAP)
diff --git a/include/builddefs.in b/include/builddefs.in
index e8f447f9..78eddf4a 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -22,6 +22,7 @@ LDFLAGS =
LIBRT = @librt@
LIBUUID = @libuuid@
+LIBURCU = @liburcu@
LIBPTHREAD = @libpthread@
LIBTERMCAP = @libtermcap@
LIBEDITLINE = @libeditline@
@@ -125,7 +126,8 @@ CROND_DIR = @crond_dir@
GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall
# -Wbitwise -Wno-transparent-union -Wno-old-initializer -Wno-decl
-PCFLAGS = -D_GNU_SOURCE $(GCCFLAGS)
+# _LGPL_SOURCE is for liburcu to work correctly with GPL/LGPL programs
+PCFLAGS = -D_LGPL_SOURCE -D_GNU_SOURCE $(GCCFLAGS)
ifeq ($(HAVE_UMODE_T),yes)
PCFLAGS += -DHAVE_UMODE_T
endif
diff --git a/include/platform_defs.h.in b/include/platform_defs.h.in
index 539bdbec..7c6b3ada 100644
--- a/include/platform_defs.h.in
+++ b/include/platform_defs.h.in
@@ -23,6 +23,7 @@
#include <limits.h>
#include <stdbool.h>
#include <libgen.h>
+#include <urcu.h>
typedef struct filldir filldir_t;
diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
index 8c1a163e..702a53e2 100644
--- a/libfrog/workqueue.c
+++ b/libfrog/workqueue.c
@@ -11,6 +11,7 @@
#include <stdbool.h>
#include <errno.h>
#include <assert.h>
+#include <urcu.h>
#include "workqueue.h"
/* Main processing thread */
@@ -24,6 +25,7 @@ workqueue_thread(void *arg)
* Loop pulling work from the passed in work queue.
* Check for notification to exit after every chunk of work.
*/
+ rcu_register_thread();
while (1) {
pthread_mutex_lock(&wq->lock);
@@ -60,6 +62,7 @@ workqueue_thread(void *arg)
(wi->function)(wi->queue, wi->index, wi->arg);
free(wi);
}
+ rcu_unregister_thread();
return NULL;
}
diff --git a/libxfs/init.c b/libxfs/init.c
index 1ec83791..b06faf8a 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -310,6 +310,8 @@ libxfs_init(libxfs_init_t *a)
fd = -1;
flags = (a->isreadonly | a->isdirect);
+ rcu_init();
+ rcu_register_thread();
radix_tree_init();
if (a->volname) {
@@ -1023,6 +1025,7 @@ libxfs_destroy(
libxfs_bcache_free();
cache_destroy(libxfs_bcache);
leaked = destroy_zones();
+ rcu_unregister_thread();
if (getenv("LIBXFS_LEAK_CHECK") && leaked)
exit(1);
}
diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 7181a858..db90e173 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -210,8 +210,7 @@ enum ce { CE_DEBUG, CE_CONT, CE_NOTE, CE_WARN, CE_ALERT, CE_PANIC };
#define spin_unlock(a) ((void) 0)
#define likely(x) (x)
#define unlikely(x) (x)
-#define rcu_read_lock() ((void) 0)
-#define rcu_read_unlock() ((void) 0)
+
/* Need to be able to handle this bare or in control flow */
static inline bool WARN_ON(bool expr) {
return (expr);
diff --git a/logprint/Makefile b/logprint/Makefile
index 758504b3..cdedbd0d 100644
--- a/logprint/Makefile
+++ b/logprint/Makefile
@@ -12,7 +12,8 @@ CFILES = logprint.c \
log_copy.c log_dump.c log_misc.c \
log_print_all.c log_print_trans.c log_redo.c
-LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
+LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) \
+ $(LIBURCU)
LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBFROG)
LLDFLAGS = -static-libtool-libs
diff --git a/m4/Makefile b/m4/Makefile
index c6c73dc9..73120530 100644
--- a/m4/Makefile
+++ b/m4/Makefile
@@ -24,6 +24,7 @@ LSRCFILES = \
package_services.m4 \
package_types.m4 \
package_icu.m4 \
+ package_urcu.m4 \
package_utilies.m4 \
package_uuiddev.m4 \
multilib.m4 \
diff --git a/m4/package_urcu.m4 b/m4/package_urcu.m4
new file mode 100644
index 00000000..74a24db9
--- /dev/null
+++ b/m4/package_urcu.m4
@@ -0,0 +1,24 @@
+AC_DEFUN([AC_PACKAGE_NEED_URCU_H],
+ [ AC_CHECK_HEADERS([urcu.h])
+ if test $ac_cv_header_urcu_h = no; then
+ echo
+ echo 'FATAL ERROR: could not find a valid URCU header.'
+ echo 'Install the Userspace RCU development package.'
+ exit 1
+ fi
+ ])
+
+AC_DEFUN([AC_PACKAGE_NEED_RCU_SET_POINTER_SYM],
+ [ AC_CHECK_FUNCS(rcu_set_pointer_sym)
+ if test $ac_cv_func_rcu_set_pointer_sym = yes; then
+ liburcu=""
+ else
+ AC_CHECK_LIB(urcu, rcu_set_pointer_sym,, [
+ echo
+ echo 'FATAL ERROR: could not find a valid urcu library.'
+ echo 'Install the Userspace RCU development package.'
+ exit 1])
+ liburcu="-lurcu"
+ fi
+ AC_SUBST(liburcu)
+ ])
diff --git a/mdrestore/Makefile b/mdrestore/Makefile
index d946955b..8f28ddab 100644
--- a/mdrestore/Makefile
+++ b/mdrestore/Makefile
@@ -8,7 +8,8 @@ include $(TOPDIR)/include/builddefs
LTCOMMAND = xfs_mdrestore
CFILES = xfs_mdrestore.c
-LLDLIBS = $(LIBXFS) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBUUID)
+LLDLIBS = $(LIBXFS) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBUUID) \
+ $(LIBURCU)
LTDEPENDENCIES = $(LIBXFS) $(LIBFROG)
LLDFLAGS = -static
diff --git a/mkfs/Makefile b/mkfs/Makefile
index b8805f7e..811ba9db 100644
--- a/mkfs/Makefile
+++ b/mkfs/Makefile
@@ -11,7 +11,7 @@ HFILES =
CFILES = proto.c xfs_mkfs.c
LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
- $(LIBUUID) $(LIBINIH)
+ $(LIBUUID) $(LIBINIH) $(LIBURCU)
LTDEPENDENCIES += $(LIBXFS) $(LIBXCMD) $(LIBFROG)
LLDFLAGS = -static-libtool-libs
diff --git a/repair/Makefile b/repair/Makefile
index 5f0764d1..47536ca1 100644
--- a/repair/Makefile
+++ b/repair/Makefile
@@ -72,7 +72,7 @@ CFILES = \
xfs_repair.c
LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBXCMD) $(LIBFROG) $(LIBUUID) $(LIBRT) \
- $(LIBPTHREAD) $(LIBBLKID)
+ $(LIBPTHREAD) $(LIBBLKID) $(LIBURCU)
LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBXCMD) $(LIBFROG)
LLDFLAGS = -static-libtool-libs
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 48affa18..22a0c0c9 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -660,6 +660,7 @@ pf_io_worker(
if (buf == NULL)
return NULL;
+ rcu_register_thread();
pthread_mutex_lock(&args->lock);
while (!args->queuing_done || !btree_is_empty(args->io_queue)) {
pftrace("waiting to start prefetch I/O for AG %d", args->agno);
@@ -682,6 +683,7 @@ pf_io_worker(
free(buf);
pftrace("finished prefetch I/O for AG %d", args->agno);
+ rcu_unregister_thread();
return NULL;
}
@@ -726,6 +728,8 @@ pf_queuing_worker(
struct xfs_ino_geometry *igeo = M_IGEO(mp);
unsigned long long cluster_mask;
+ rcu_register_thread();
+
cluster_mask = (1ULL << igeo->inodes_per_cluster) - 1;
for (i = 0; i < PF_THREAD_COUNT; i++) {
@@ -739,7 +743,7 @@ pf_queuing_worker(
args->io_threads[i] = 0;
if (i == 0) {
pf_skip_prefetch_thread(args);
- return NULL;
+ goto out;
}
/*
* since we have at least one I/O thread, use them for
@@ -779,7 +783,6 @@ pf_queuing_worker(
* Start processing as well, in case everything so
* far was already prefetched and the queue is empty.
*/
-
pf_start_io_workers(args);
pf_start_processing(args);
sem_wait(&args->ra_count);
@@ -841,6 +844,8 @@ pf_queuing_worker(
if (next_args)
pf_create_prefetch_thread(next_args);
+out:
+ rcu_unregister_thread();
return NULL;
}
diff --git a/repair/progress.c b/repair/progress.c
index e5a9c1ef..f6c4d988 100644
--- a/repair/progress.c
+++ b/repair/progress.c
@@ -182,6 +182,7 @@ progress_rpt_thread (void *p)
do_error (_("progress_rpt: cannot malloc progress msg buffer\n"));
running = 1;
+ rcu_register_thread();
/*
* Specify a repeating timer that fires each MSG_INTERVAL seconds.
@@ -286,7 +287,8 @@ progress_rpt_thread (void *p)
do_warn(_("cannot delete timer\n"));
free (msgbuf);
- return (NULL);
+ rcu_unregister_thread();
+ return NULL;
}
int
diff --git a/scrub/Makefile b/scrub/Makefile
index 47c887eb..849e3afd 100644
--- a/scrub/Makefile
+++ b/scrub/Makefile
@@ -71,7 +71,8 @@ spacemap.c \
vfs.c \
xfs_scrub.c
-LLDLIBS += $(LIBHANDLE) $(LIBFROG) $(LIBPTHREAD) $(LIBICU_LIBS) $(LIBRT)
+LLDLIBS += $(LIBHANDLE) $(LIBFROG) $(LIBPTHREAD) $(LIBICU_LIBS) $(LIBRT) \
+ $(LIBURCU)
LTDEPENDENCIES += $(LIBHANDLE) $(LIBFROG)
LLDFLAGS = -static
diff --git a/scrub/progress.c b/scrub/progress.c
index 15247b7c..a3d096f9 100644
--- a/scrub/progress.c
+++ b/scrub/progress.c
@@ -116,6 +116,7 @@ progress_report_thread(void *arg)
struct timespec abstime;
int ret;
+ rcu_register_thread();
pthread_mutex_lock(&pt.lock);
while (1) {
uint64_t progress_val;
@@ -139,6 +140,7 @@ progress_report_thread(void *arg)
progress_report(progress_val);
}
pthread_mutex_unlock(&pt.lock);
+ rcu_unregister_thread();
return NULL;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V2 1/5] xfsprogs: introduce liburcu support
2021-09-24 14:09 ` [PATCH V2 1/5] xfsprogs: introduce liburcu support Chandan Babu R
@ 2021-09-24 21:51 ` Eric Sandeen
2021-09-25 10:24 ` Chandan Babu R
2021-09-25 23:05 ` Dave Chinner
2021-09-29 20:46 ` Eric Sandeen
1 sibling, 2 replies; 21+ messages in thread
From: Eric Sandeen @ 2021-09-24 21:51 UTC (permalink / raw)
To: Chandan Babu R, linux-xfs; +Cc: Dave Chinner, david, djwong
On 9/24/21 9:09 AM, Chandan Babu R wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> The upcoming buffer cache rework/kerenl sync-up requires atomic
> variables. I could use C++11 atomics build into GCC, but they are a
> pain to work with and shoe-horn into the kernel atomic variable API.
>
> Much easier is to introduce a dependency on liburcu - the userspace
> RCU library. This provides atomic variables that very closely match
> the kernel atomic variable API, and it provides a very similar
> memory model and memory barrier support to the kernel. And we get
> RCU support that has an identical interface to the kernel and works
> the same way.
>
> Hence kernel code written with RCU algorithms and atomic variables
> will just slot straight into the userspace xfsprogs code without us
> having to think about whether the lockless algorithms will work in
> userspace or not. This reduces glue and hoop jumping, and gets us
> a step closer to having the entire userspace libxfs code MT safe.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> [chandan.babu@oracle.com: Add m4 macros to detect availability of liburcu]
Thanks for fixing that up. I had tried to use rcu_init like Dave originally
had, and I like that better in general, but I had trouble with it - I guess
maybe it gets redefined based on memory model magic and the actual symbol
"rcu_init" maybe isn't available? I didn't dig very much.
Also, dumb question from me - how do we know where we need the
rcu_[un]register_thread() calls? Will it be obvious if we miss it
in the future? "each thread must invoke this function before its
first call to rcu_read_lock() or call_rcu()."
Thanks,
-Eric
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/5] xfsprogs: introduce liburcu support
2021-09-24 21:51 ` Eric Sandeen
@ 2021-09-25 10:24 ` Chandan Babu R
2021-09-25 23:05 ` Dave Chinner
1 sibling, 0 replies; 21+ messages in thread
From: Chandan Babu R @ 2021-09-25 10:24 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs, Dave Chinner, david, djwong
On 25 Sep 2021 at 03:21, Eric Sandeen wrote:
> On 9/24/21 9:09 AM, Chandan Babu R wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>> The upcoming buffer cache rework/kerenl sync-up requires atomic
>> variables. I could use C++11 atomics build into GCC, but they are a
>> pain to work with and shoe-horn into the kernel atomic variable API.
>> Much easier is to introduce a dependency on liburcu - the userspace
>> RCU library. This provides atomic variables that very closely match
>> the kernel atomic variable API, and it provides a very similar
>> memory model and memory barrier support to the kernel. And we get
>> RCU support that has an identical interface to the kernel and works
>> the same way.
>> Hence kernel code written with RCU algorithms and atomic variables
>> will just slot straight into the userspace xfsprogs code without us
>> having to think about whether the lockless algorithms will work in
>> userspace or not. This reduces glue and hoop jumping, and gets us
>> a step closer to having the entire userspace libxfs code MT safe.
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>> [chandan.babu@oracle.com: Add m4 macros to detect availability of liburcu]
>
> Thanks for fixing that up. I had tried to use rcu_init like Dave originally
> had, and I like that better in general, but I had trouble with it - I guess
> maybe it gets redefined based on memory model magic and the actual symbol
> "rcu_init" maybe isn't available? I didn't dig very much.
Yes, rcu_init() gets defined to one of the variants of userspace rcu. Hence
there is no function named rcu_init().
>
> Also, dumb question from me - how do we know where we need the
> rcu_[un]register_thread() calls? Will it be obvious if we miss it
> in the future? "each thread must invoke this function before its
> first call to rcu_read_lock() or call_rcu()."
Unfortunately, I don't think there is an obvious method to detect if calls to
rcu_[un]register_thread() is missing from any code changes implemented in the
future.
PS: I am no expert at RCU. My knowledge about RCU is limited to what I could
learn by reading articles on the world wide web.
--
chandan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/5] xfsprogs: introduce liburcu support
2021-09-24 21:51 ` Eric Sandeen
2021-09-25 10:24 ` Chandan Babu R
@ 2021-09-25 23:05 ` Dave Chinner
2021-09-27 18:48 ` Eric Sandeen
1 sibling, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2021-09-25 23:05 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Chandan Babu R, linux-xfs, Dave Chinner, djwong
On Fri, Sep 24, 2021 at 04:51:32PM -0500, Eric Sandeen wrote:
> On 9/24/21 9:09 AM, Chandan Babu R wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > The upcoming buffer cache rework/kerenl sync-up requires atomic
> > variables. I could use C++11 atomics build into GCC, but they are a
> > pain to work with and shoe-horn into the kernel atomic variable API.
> >
> > Much easier is to introduce a dependency on liburcu - the userspace
> > RCU library. This provides atomic variables that very closely match
> > the kernel atomic variable API, and it provides a very similar
> > memory model and memory barrier support to the kernel. And we get
> > RCU support that has an identical interface to the kernel and works
> > the same way.
> >
> > Hence kernel code written with RCU algorithms and atomic variables
> > will just slot straight into the userspace xfsprogs code without us
> > having to think about whether the lockless algorithms will work in
> > userspace or not. This reduces glue and hoop jumping, and gets us
> > a step closer to having the entire userspace libxfs code MT safe.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > [chandan.babu@oracle.com: Add m4 macros to detect availability of liburcu]
>
> Thanks for fixing that up. I had tried to use rcu_init like Dave originally
> had, and I like that better in general, but I had trouble with it - I guess
> maybe it gets redefined based on memory model magic and the actual symbol
> "rcu_init" maybe isn't available? I didn't dig very much.
Ah, so I just checked where the m4/package_urcu.m4 file went -
forgot to re-add it after it rejected on apply. The diff is this:
diff --git a/m4/package_urcu.m4 b/m4/package_urcu.m4
new file mode 100644
index 000000000000..9b0dee35d9a1
--- /dev/null
+++ b/m4/package_urcu.m4
@@ -0,0 +1,22 @@
+AC_DEFUN([AC_PACKAGE_NEED_URCU_H],
+ [ AC_CHECK_HEADERS(urcu.h)
+ if test $ac_cv_header_urcu_h = no; then
+ AC_CHECK_HEADERS(urcu.h,, [
+ echo
+ echo 'FATAL ERROR: could not find a valid urcu header.'
+ exit 1])
+ fi
+ ])
+
+AC_DEFUN([AC_PACKAGE_NEED_RCU_INIT],
+ [ AC_MSG_CHECKING([for liburcu])
+ AC_TRY_COMPILE([
+#define _GNU_SOURCE
+#include <urcu.h>
+ ], [
+ rcu_init();
+ ], liburcu=-lurcu
+ AC_MSG_RESULT(yes),
+ AC_MSG_RESULT(no))
+ AC_SUBST(liburcu)
+ ])
And this ends up with the output on a current debian system of:
checking urcu.h usability... yes
checking urcu.h presence... yes
checking for urcu.h... yes
checking for liburcu... yes
IOWs, the package check I was using directly probed for rcu_init()
being present in liburcu. Hence if liburcu is not providing this
function via the default linking like we use with xfsprogs, then
we fail at the configure step.
So it liburcu is not providing rcu_init(), then the configure step
should fail, and you need to work out what is different about the
liburcu that is installed on the distro you are building on....
> Also, dumb question from me - how do we know where we need the
> rcu_[un]register_thread() calls? Will it be obvious if we miss it
> in the future? "each thread must invoke this function before its
> first call to rcu_read_lock() or call_rcu()."
urcu should fire an assert on any attempt to access urcu TLS storage
because the "urcu.registered" flag in the TLS area has not been
initialised. IOWs, if we miss register/unregister then things will
go bad in urcu calls and it should be noticed.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/5] xfsprogs: introduce liburcu support
2021-09-25 23:05 ` Dave Chinner
@ 2021-09-27 18:48 ` Eric Sandeen
0 siblings, 0 replies; 21+ messages in thread
From: Eric Sandeen @ 2021-09-27 18:48 UTC (permalink / raw)
To: Dave Chinner; +Cc: Chandan Babu R, linux-xfs, Dave Chinner, djwong
On 9/25/21 6:05 PM, Dave Chinner wrote:
> On Fri, Sep 24, 2021 at 04:51:32PM -0500, Eric Sandeen wrote:
>> On 9/24/21 9:09 AM, Chandan Babu R wrote:
>>> From: Dave Chinner <dchinner@redhat.com>
>>>
>>> The upcoming buffer cache rework/kerenl sync-up requires atomic
>>> variables. I could use C++11 atomics build into GCC, but they are a
>>> pain to work with and shoe-horn into the kernel atomic variable API.
>>>
>>> Much easier is to introduce a dependency on liburcu - the userspace
>>> RCU library. This provides atomic variables that very closely match
>>> the kernel atomic variable API, and it provides a very similar
>>> memory model and memory barrier support to the kernel. And we get
>>> RCU support that has an identical interface to the kernel and works
>>> the same way.
>>>
>>> Hence kernel code written with RCU algorithms and atomic variables
>>> will just slot straight into the userspace xfsprogs code without us
>>> having to think about whether the lockless algorithms will work in
>>> userspace or not. This reduces glue and hoop jumping, and gets us
>>> a step closer to having the entire userspace libxfs code MT safe.
>>>
>>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>>> [chandan.babu@oracle.com: Add m4 macros to detect availability of liburcu]
>>
>> Thanks for fixing that up. I had tried to use rcu_init like Dave originally
>> had, and I like that better in general, but I had trouble with it - I guess
>> maybe it gets redefined based on memory model magic and the actual symbol
>> "rcu_init" maybe isn't available? I didn't dig very much.
>
> Ah, so I just checked where the m4/package_urcu.m4 file went -
> forgot to re-add it after it rejected on apply. The diff is this:
>
> diff --git a/m4/package_urcu.m4 b/m4/package_urcu.m4
> new file mode 100644
> index 000000000000..9b0dee35d9a1
> --- /dev/null
> +++ b/m4/package_urcu.m4
> @@ -0,0 +1,22 @@
> +AC_DEFUN([AC_PACKAGE_NEED_URCU_H],
> + [ AC_CHECK_HEADERS(urcu.h)
> + if test $ac_cv_header_urcu_h = no; then
> + AC_CHECK_HEADERS(urcu.h,, [
> + echo
> + echo 'FATAL ERROR: could not find a valid urcu header.'
> + exit 1])
> + fi
> + ])
> +
> +AC_DEFUN([AC_PACKAGE_NEED_RCU_INIT],
> + [ AC_MSG_CHECKING([for liburcu])
> + AC_TRY_COMPILE([
> +#define _GNU_SOURCE
> +#include <urcu.h>
> + ], [
> + rcu_init();
> + ], liburcu=-lurcu
> + AC_MSG_RESULT(yes),
> + AC_MSG_RESULT(no))
> + AC_SUBST(liburcu)
> + ])
Works great here too. My error was not including urcu.h in the test I think,
and looking for the symbol directly. I will merge this version rather
than Chandan's.
Thanks,
-Eric
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 1/5] xfsprogs: introduce liburcu support
2021-09-24 14:09 ` [PATCH V2 1/5] xfsprogs: introduce liburcu support Chandan Babu R
2021-09-24 21:51 ` Eric Sandeen
@ 2021-09-29 20:46 ` Eric Sandeen
1 sibling, 0 replies; 21+ messages in thread
From: Eric Sandeen @ 2021-09-29 20:46 UTC (permalink / raw)
To: Chandan Babu R, linux-xfs; +Cc: Dave Chinner, david, djwong
On 9/24/21 9:09 AM, Chandan Babu R wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> The upcoming buffer cache rework/kerenl sync-up requires atomic
> variables. I could use C++11 atomics build into GCC, but they are a
> pain to work with and shoe-horn into the kernel atomic variable API.
>
> Much easier is to introduce a dependency on liburcu - the userspace
> RCU library. This provides atomic variables that very closely match
> the kernel atomic variable API, and it provides a very similar
> memory model and memory barrier support to the kernel. And we get
> RCU support that has an identical interface to the kernel and works
> the same way.
>
> Hence kernel code written with RCU algorithms and atomic variables
> will just slot straight into the userspace xfsprogs code without us
> having to think about whether the lockless algorithms will work in
> userspace or not. This reduces glue and hoop jumping, and gets us
> a step closer to having the entire userspace libxfs code MT safe.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> [chandan.babu@oracle.com: Add m4 macros to detect availability of liburcu]
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
I've added Dave's m4 macros in place of yours, and with that circuitous
route I think we have a good set of changes, so:
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
for the combined set of this-plus-dave's-m4's (elsewhere in replies
on this thread)
Thanks,
-Eric
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V2 2/5] libxfs: add spinlock_t wrapper
2021-09-24 14:09 [PATCH V2 0/5] xfsprogs: generic serialisation primitives Chandan Babu R
2021-09-24 14:09 ` [PATCH V2 1/5] xfsprogs: introduce liburcu support Chandan Babu R
@ 2021-09-24 14:09 ` Chandan Babu R
2021-09-24 22:06 ` Eric Sandeen
2021-09-24 14:09 ` [PATCH V2 3/5] atomic: convert to uatomic Chandan Babu R
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Chandan Babu R @ 2021-09-24 14:09 UTC (permalink / raw)
To: linux-xfs; +Cc: Dave Chinner, david, sandeen, djwong, Chandan Babu R
From: Dave Chinner <dchinner@redhat.com>
These provide the kernel spinlock_t interface, but are *not*
spinlocks. Spinlocks cannot be used by general purpose userspace
processes due to the fact they cannot control task preemption and
scheduling reliability. Hence these are implemented as a
pthread_mutex_t, similar to the way the kernel RT build implements
spinlock_t as a kernel mutex.
Because the current libxfs spinlock "implementation" just makes
spinlocks go away, we have to also add initialisation to spinlocks
that libxfs uses that are missing from the userspace implementation.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
[chandan.babu@oracle.com: Initialize inode log item spin lock]
Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
---
include/Makefile | 1 +
include/libxfs.h | 1 +
include/spinlock.h | 25 +++++++++++++++++++++++++
include/xfs_inode.h | 1 +
include/xfs_mount.h | 2 ++
include/xfs_trans.h | 1 +
libxfs/init.c | 4 +++-
libxfs/libxfs_priv.h | 4 +---
libxfs/logitem.c | 4 +++-
libxfs/rdwr.c | 2 ++
10 files changed, 40 insertions(+), 5 deletions(-)
create mode 100644 include/spinlock.h
diff --git a/include/Makefile b/include/Makefile
index 632b819f..f7c40a5c 100644
--- a/include/Makefile
+++ b/include/Makefile
@@ -16,6 +16,7 @@ LIBHFILES = libxfs.h \
kmem.h \
list.h \
parent.h \
+ spinlock.h \
xfs_inode.h \
xfs_log_recover.h \
xfs_metadump.h \
diff --git a/include/libxfs.h b/include/libxfs.h
index bc07655e..a494a1d4 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -18,6 +18,7 @@
#include "kmem.h"
#include "libfrog/radix-tree.h"
#include "atomic.h"
+#include "spinlock.h"
#include "xfs_types.h"
#include "xfs_fs.h"
diff --git a/include/spinlock.h b/include/spinlock.h
new file mode 100644
index 00000000..8da2325c
--- /dev/null
+++ b/include/spinlock.h
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019-20 RedHat, Inc.
+ * All Rights Reserved.
+ */
+#ifndef __LIBXFS_SPINLOCK_H__
+#define __LIBXFS_SPINLOCK_H__
+
+/*
+ * This implements kernel compatible spinlock exclusion semantics. These,
+ * however, are not spinlocks, as spinlocks cannot be reliably implemented in
+ * userspace without using realtime scheduling task contexts. Hence this
+ * interface is implemented with pthread mutexes and so can block, but this is
+ * no different to the kernel RT build which replaces spinlocks with mutexes.
+ * Hence we know it works.
+ */
+
+typedef pthread_mutex_t spinlock_t;
+
+#define spin_lock_init(l) pthread_mutex_init(l, NULL)
+#define spin_lock(l) pthread_mutex_lock(l)
+#define spin_trylock(l) (pthread_mutex_trylock(l) != EBUSY)
+#define spin_unlock(l) pthread_mutex_unlock(l)
+
+#endif /* __LIBXFS_SPINLOCK_H__ */
diff --git a/include/xfs_inode.h b/include/xfs_inode.h
index 0551fe45..08a62d83 100644
--- a/include/xfs_inode.h
+++ b/include/xfs_inode.h
@@ -43,6 +43,7 @@ struct inode {
struct timespec64 i_atime;
struct timespec64 i_mtime;
struct timespec64 i_ctime;
+ spinlock_t i_lock;
};
static inline uint32_t i_uid_read(struct inode *inode)
diff --git a/include/xfs_mount.h b/include/xfs_mount.h
index 12019c4b..2f320880 100644
--- a/include/xfs_mount.h
+++ b/include/xfs_mount.h
@@ -22,6 +22,7 @@ typedef struct xfs_mount {
#define m_icount m_sb.sb_icount
#define m_ifree m_sb.sb_ifree
#define m_fdblocks m_sb.sb_fdblocks
+ spinlock_t m_sb_lock;
/*
* Bitsets of per-fs metadata that have been checked and/or are sick.
@@ -32,6 +33,7 @@ typedef struct xfs_mount {
char *m_fsname; /* filesystem name */
int m_bsize; /* fs logical block size */
+ spinlock_t m_agirotor_lock;
xfs_agnumber_t m_agfrotor; /* last ag where space found */
xfs_agnumber_t m_agirotor; /* last ag dir inode alloced */
xfs_agnumber_t m_maxagi; /* highest inode alloc group */
diff --git a/include/xfs_trans.h b/include/xfs_trans.h
index ad76ecfd..2c55bb85 100644
--- a/include/xfs_trans.h
+++ b/include/xfs_trans.h
@@ -35,6 +35,7 @@ struct xfs_inode_log_item {
unsigned int ili_last_fields; /* fields when flushed*/
unsigned int ili_fields; /* fields to be logged */
unsigned int ili_fsync_fields; /* ignored by userspace */
+ spinlock_t ili_lock;
};
typedef struct xfs_buf_log_item {
diff --git a/libxfs/init.c b/libxfs/init.c
index b06faf8a..2c54546b 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -743,7 +743,9 @@ libxfs_mount(
mp->m_flags = (LIBXFS_MOUNT_32BITINODES|LIBXFS_MOUNT_32BITINOOPT);
mp->m_sb = *sb;
INIT_RADIX_TREE(&mp->m_perag_tree, GFP_KERNEL);
- sbp = &(mp->m_sb);
+ sbp = &mp->m_sb;
+ spin_lock_init(&mp->m_sb_lock);
+ spin_lock_init(&mp->m_agirotor_lock);
xfs_sb_mount_common(mp, sb);
diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index db90e173..e1e90268 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -48,6 +48,7 @@
#include "kmem.h"
#include "libfrog/radix-tree.h"
#include "atomic.h"
+#include "spinlock.h"
#include "xfs_types.h"
#include "xfs_arch.h"
@@ -205,9 +206,6 @@ enum ce { CE_DEBUG, CE_CONT, CE_NOTE, CE_WARN, CE_ALERT, CE_PANIC };
#endif
/* miscellaneous kernel routines not in user space */
-#define spin_lock_init(a) ((void) 0)
-#define spin_lock(a) ((void) 0)
-#define spin_unlock(a) ((void) 0)
#define likely(x) (x)
#define unlikely(x) (x)
diff --git a/libxfs/logitem.c b/libxfs/logitem.c
index 4d4e8080..b073cdb4 100644
--- a/libxfs/logitem.c
+++ b/libxfs/logitem.c
@@ -144,6 +144,8 @@ xfs_inode_item_init(
ip->i_ino, iip);
#endif
- xfs_log_item_init(mp, &iip->ili_item, XFS_LI_INODE);
+ spin_lock_init(&iip->ili_lock);
+
+ xfs_log_item_init(mp, &iip->ili_item, XFS_LI_INODE);
iip->ili_inode = ip;
}
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 713ef9af..a5fd0596 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1070,6 +1070,8 @@ libxfs_iget(
VFS_I(ip)->i_count = 1;
ip->i_ino = ino;
ip->i_mount = mp;
+ spin_lock_init(&VFS_I(ip)->i_lock);
+
error = xfs_imap(mp, tp, ip->i_ino, &ip->i_imap, 0);
if (error)
goto out_destroy;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V2 2/5] libxfs: add spinlock_t wrapper
2021-09-24 14:09 ` [PATCH V2 2/5] libxfs: add spinlock_t wrapper Chandan Babu R
@ 2021-09-24 22:06 ` Eric Sandeen
0 siblings, 0 replies; 21+ messages in thread
From: Eric Sandeen @ 2021-09-24 22:06 UTC (permalink / raw)
To: Chandan Babu R, linux-xfs; +Cc: Dave Chinner, david, djwong
On 9/24/21 9:09 AM, Chandan Babu R wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> These provide the kernel spinlock_t interface, but are *not*
> spinlocks. Spinlocks cannot be used by general purpose userspace
> processes due to the fact they cannot control task preemption and
> scheduling reliability. Hence these are implemented as a
> pthread_mutex_t, similar to the way the kernel RT build implements
> spinlock_t as a kernel mutex.
>
> Because the current libxfs spinlock "implementation" just makes
> spinlocks go away, we have to also add initialisation to spinlocks
> that libxfs uses that are missing from the userspace implementation.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> [chandan.babu@oracle.com: Initialize inode log item spin lock]
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
...
> +/*
> + * This implements kernel compatible spinlock exclusion semantics. These,
> + * however, are not spinlocks, as spinlocks cannot be reliably implemented in
> + * userspace without using realtime scheduling task contexts. Hence this
> + * interface is implemented with pthread mutexes and so can block, but this is
> + * no different to the kernel RT build which replaces spinlocks with mutexes.
> + * Hence we know it works.
> + */
> +
> +typedef pthread_mutex_t spinlock_t;
> +
> +#define spin_lock_init(l) pthread_mutex_init(l, NULL)
> +#define spin_lock(l) pthread_mutex_lock(l)
> +#define spin_trylock(l) (pthread_mutex_trylock(l) != EBUSY)
> +#define spin_unlock(l) pthread_mutex_unlock(l)
some whitespace mess here but I'll just clean that up.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V2 3/5] atomic: convert to uatomic
2021-09-24 14:09 [PATCH V2 0/5] xfsprogs: generic serialisation primitives Chandan Babu R
2021-09-24 14:09 ` [PATCH V2 1/5] xfsprogs: introduce liburcu support Chandan Babu R
2021-09-24 14:09 ` [PATCH V2 2/5] libxfs: add spinlock_t wrapper Chandan Babu R
@ 2021-09-24 14:09 ` Chandan Babu R
2021-09-24 22:13 ` Eric Sandeen
2021-09-24 14:09 ` [PATCH V2 4/5] libxfs: add kernel-compatible completion API Chandan Babu R
2021-09-24 14:09 ` [PATCH V2 5/5] libxfs: add wrappers for kernel semaphores Chandan Babu R
4 siblings, 1 reply; 21+ messages in thread
From: Chandan Babu R @ 2021-09-24 14:09 UTC (permalink / raw)
To: linux-xfs; +Cc: Dave Chinner, david, sandeen, djwong, Chandan Babu R
From: Dave Chinner <dchinner@redhat.com>
Now we have liburcu, we can make use of it's atomic variable
implementation. It is almost identical to the kernel API - it's just
got a "uatomic" prefix. liburcu also provides all the same aomtic
variable memory barriers as the kernel, so if we pull memory barrier
dependent kernel code across, it will just work with the right
barrier wrappers.
This is preparation the addition of more extensive atomic operations
the that kernel buffer cache requires to function correctly.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
[chandan.babu@oracle.com: Swap order of arguments provided to atomic[64]_[add|sub]()]
Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
---
include/atomic.h | 65 ++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 54 insertions(+), 11 deletions(-)
diff --git a/include/atomic.h b/include/atomic.h
index e0e1ba84..99cb85d3 100644
--- a/include/atomic.h
+++ b/include/atomic.h
@@ -7,21 +7,64 @@
#define __ATOMIC_H__
/*
- * Warning: These are not really atomic at all. They are wrappers around the
- * kernel atomic variable interface. If we do need these variables to be atomic
- * (due to multithreading of the code that uses them) we need to add some
- * pthreads magic here.
+ * Atomics are provided by liburcu.
+ *
+ * API and guidelines for which operations provide memory barriers is here:
+ *
+ * https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
+ *
+ * Unlike the kernel, the same interface supports 32 and 64 bit atomic integers.
*/
+#include <urcu/uatomic.h>
+#include "spinlock.h"
+
typedef int32_t atomic_t;
typedef int64_t atomic64_t;
-#define atomic_inc_return(x) (++(*(x)))
-#define atomic_dec_return(x) (--(*(x)))
+#define atomic_read(a) uatomic_read(a)
+#define atomic_set(a, v) uatomic_set(a, v)
+#define atomic_add(v, a) uatomic_add(a, v)
+#define atomic_sub(v, a) uatomic_sub(a, v)
+#define atomic_inc(a) uatomic_inc(a)
+#define atomic_dec(a) uatomic_dec(a)
+#define atomic_inc_return(a) uatomic_add_return(a, 1)
+#define atomic_dec_return(a) uatomic_sub_return(a, 1)
+#define atomic_dec_and_test(a) (atomic_dec_return(a) == 0)
+#define cmpxchg(a, o, n) uatomic_cmpxchg(a, o, n);
+
+static inline bool atomic_add_unless(atomic_t *a, int v, int u)
+{
+ int r = atomic_read(a);
+ int n, o;
+
+ do {
+ o = r;
+ if (o == u)
+ break;
+ n = o + v;
+ r = uatomic_cmpxchg(a, o, n);
+ } while (r != o);
+
+ return o != u;
+}
+
+static inline bool atomic_dec_and_lock(atomic_t *a, spinlock_t *lock)
+{
+ if (atomic_add_unless(a, -1, 1))
+ return 0;
+
+ spin_lock(lock);
+ if (atomic_dec_and_test(a))
+ return 1;
+ spin_unlock(lock);
+ return 0;
+}
-#define atomic64_read(x) *(x)
-#define atomic64_set(x, v) (*(x) = v)
-#define atomic64_add(v, x) (*(x) += v)
-#define atomic64_inc(x) ((*(x))++)
-#define atomic64_dec(x) ((*(x))--)
+#define atomic64_read(x) uatomic_read(x)
+#define atomic64_set(x, v) uatomic_set(x, v)
+#define atomic64_add(v, a) uatomic_add(a, v)
+#define atomic64_sub(v, a) uatomic_sub(a, v)
+#define atomic64_inc(a) uatomic_inc(a)
+#define atomic64_dec(a) uatomic_dec(a)
#endif /* __ATOMIC_H__ */
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V2 3/5] atomic: convert to uatomic
2021-09-24 14:09 ` [PATCH V2 3/5] atomic: convert to uatomic Chandan Babu R
@ 2021-09-24 22:13 ` Eric Sandeen
2021-09-25 10:26 ` Chandan Babu R
2021-09-25 23:15 ` Dave Chinner
0 siblings, 2 replies; 21+ messages in thread
From: Eric Sandeen @ 2021-09-24 22:13 UTC (permalink / raw)
To: Chandan Babu R, linux-xfs; +Cc: Dave Chinner, david, djwong
On 9/24/21 9:09 AM, Chandan Babu R wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Now we have liburcu, we can make use of it's atomic variable
> implementation. It is almost identical to the kernel API - it's just
> got a "uatomic" prefix. liburcu also provides all the same aomtic
> variable memory barriers as the kernel, so if we pull memory barrier
> dependent kernel code across, it will just work with the right
> barrier wrappers.
>
> This is preparation the addition of more extensive atomic operations
> the that kernel buffer cache requires to function correctly.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> [chandan.babu@oracle.com: Swap order of arguments provided to atomic[64]_[add|sub]()]
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
> include/atomic.h | 65 ++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 54 insertions(+), 11 deletions(-)
>
> diff --git a/include/atomic.h b/include/atomic.h
> index e0e1ba84..99cb85d3 100644
> --- a/include/atomic.h
> +++ b/include/atomic.h
> @@ -7,21 +7,64 @@
> #define __ATOMIC_H__
>
> /*
> - * Warning: These are not really atomic at all. They are wrappers around the
> - * kernel atomic variable interface. If we do need these variables to be atomic
> - * (due to multithreading of the code that uses them) we need to add some
> - * pthreads magic here.
> + * Atomics are provided by liburcu.
> + *
> + * API and guidelines for which operations provide memory barriers is here:
> + *
> + * https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
> + *
> + * Unlike the kernel, the same interface supports 32 and 64 bit atomic integers.
Given this, anyone have any objection to putting the #defines together at the
top, rather than hiding the 64 variants at the end of the file?
> */
> +#include <urcu/uatomic.h>
> +#include "spinlock.h"
> +
> typedef int32_t atomic_t;
> typedef int64_t atomic64_t;
>
> -#define atomic_inc_return(x) (++(*(x)))
> -#define atomic_dec_return(x) (--(*(x)))
> +#define atomic_read(a) uatomic_read(a)
> +#define atomic_set(a, v) uatomic_set(a, v)
> +#define atomic_add(v, a) uatomic_add(a, v)
> +#define atomic_sub(v, a) uatomic_sub(a, v)
> +#define atomic_inc(a) uatomic_inc(a)
> +#define atomic_dec(a) uatomic_dec(a)
> +#define atomic_inc_return(a) uatomic_add_return(a, 1)
> +#define atomic_dec_return(a) uatomic_sub_return(a, 1)
> +#define atomic_dec_and_test(a) (atomic_dec_return(a) == 0)
> +#define cmpxchg(a, o, n) uatomic_cmpxchg(a, o, n);
and I'll fix this whitespace.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 3/5] atomic: convert to uatomic
2021-09-24 22:13 ` Eric Sandeen
@ 2021-09-25 10:26 ` Chandan Babu R
2021-09-25 23:15 ` Dave Chinner
1 sibling, 0 replies; 21+ messages in thread
From: Chandan Babu R @ 2021-09-25 10:26 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs, Dave Chinner, david, djwong
On 25 Sep 2021 at 03:43, Eric Sandeen wrote:
> On 9/24/21 9:09 AM, Chandan Babu R wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>> Now we have liburcu, we can make use of it's atomic variable
>> implementation. It is almost identical to the kernel API - it's just
>> got a "uatomic" prefix. liburcu also provides all the same aomtic
>> variable memory barriers as the kernel, so if we pull memory barrier
>> dependent kernel code across, it will just work with the right
>> barrier wrappers.
>> This is preparation the addition of more extensive atomic operations
>> the that kernel buffer cache requires to function correctly.
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>> [chandan.babu@oracle.com: Swap order of arguments provided to atomic[64]_[add|sub]()]
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>> include/atomic.h | 65 ++++++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 54 insertions(+), 11 deletions(-)
>> diff --git a/include/atomic.h b/include/atomic.h
>> index e0e1ba84..99cb85d3 100644
>> --- a/include/atomic.h
>> +++ b/include/atomic.h
>> @@ -7,21 +7,64 @@
>> #define __ATOMIC_H__
>> /*
>> - * Warning: These are not really atomic at all. They are wrappers around the
>> - * kernel atomic variable interface. If we do need these variables to be atomic
>> - * (due to multithreading of the code that uses them) we need to add some
>> - * pthreads magic here.
>> + * Atomics are provided by liburcu.
>> + *
>> + * API and guidelines for which operations provide memory barriers is here:
>> + *
>> + * https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
>> + *
>> + * Unlike the kernel, the same interface supports 32 and 64 bit atomic integers.
>
> Given this, anyone have any objection to putting the #defines together at the
> top, rather than hiding the 64 variants at the end of the file?
>
I don't see any issue in doing that.
>> */
>> +#include <urcu/uatomic.h>
>> +#include "spinlock.h"
>> +
>> typedef int32_t atomic_t;
>> typedef int64_t atomic64_t;
>> -#define atomic_inc_return(x) (++(*(x)))
>> -#define atomic_dec_return(x) (--(*(x)))
>> +#define atomic_read(a) uatomic_read(a)
>> +#define atomic_set(a, v) uatomic_set(a, v)
>> +#define atomic_add(v, a) uatomic_add(a, v)
>> +#define atomic_sub(v, a) uatomic_sub(a, v)
>> +#define atomic_inc(a) uatomic_inc(a)
>> +#define atomic_dec(a) uatomic_dec(a)
>> +#define atomic_inc_return(a) uatomic_add_return(a, 1)
>> +#define atomic_dec_return(a) uatomic_sub_return(a, 1)
>> +#define atomic_dec_and_test(a) (atomic_dec_return(a) == 0)
>> +#define cmpxchg(a, o, n) uatomic_cmpxchg(a, o, n);
>
> and I'll fix this whitespace.
>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Thanks for the reviews.
--
chandan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 3/5] atomic: convert to uatomic
2021-09-24 22:13 ` Eric Sandeen
2021-09-25 10:26 ` Chandan Babu R
@ 2021-09-25 23:15 ` Dave Chinner
2021-09-25 23:18 ` Eric Sandeen
1 sibling, 1 reply; 21+ messages in thread
From: Dave Chinner @ 2021-09-25 23:15 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Chandan Babu R, linux-xfs, Dave Chinner, djwong
On Fri, Sep 24, 2021 at 05:13:30PM -0500, Eric Sandeen wrote:
> On 9/24/21 9:09 AM, Chandan Babu R wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Now we have liburcu, we can make use of it's atomic variable
> > implementation. It is almost identical to the kernel API - it's just
> > got a "uatomic" prefix. liburcu also provides all the same aomtic
> > variable memory barriers as the kernel, so if we pull memory barrier
> > dependent kernel code across, it will just work with the right
> > barrier wrappers.
> >
> > This is preparation the addition of more extensive atomic operations
> > the that kernel buffer cache requires to function correctly.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > [chandan.babu@oracle.com: Swap order of arguments provided to atomic[64]_[add|sub]()]
> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> > ---
> > include/atomic.h | 65 ++++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 54 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/atomic.h b/include/atomic.h
> > index e0e1ba84..99cb85d3 100644
> > --- a/include/atomic.h
> > +++ b/include/atomic.h
> > @@ -7,21 +7,64 @@
> > #define __ATOMIC_H__
> > /*
> > - * Warning: These are not really atomic at all. They are wrappers around the
> > - * kernel atomic variable interface. If we do need these variables to be atomic
> > - * (due to multithreading of the code that uses them) we need to add some
> > - * pthreads magic here.
> > + * Atomics are provided by liburcu.
> > + *
> > + * API and guidelines for which operations provide memory barriers is here:
> > + *
> > + * https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
> > + *
> > + * Unlike the kernel, the same interface supports 32 and 64 bit atomic integers.
>
> Given this, anyone have any objection to putting the #defines together at the
> top, rather than hiding the 64 variants at the end of the file?
I wanted to keep the -APIs- separate, because all the kernel
atomic/atomic64 stuff is already separate and type checked. I don't
see any point in commingling the two different atomic type APIs
just because the implementation ends up being the same and that some
wrappers are defines and others are static inline code.
Ideally, the wrappers should all be static inlines so we get correct
atomic_t/atomic64_t type checking in userspace. Those are the types
we care about in terms of libxfs, so to typecheck the API properly
these should -all- be static inlines. The patch as it stands was a
"get it working properly" patch, not a "finalised, strictly correct
API" patch. That was somethign for "down the road" as I polished the
patchset ready for eventual review.....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 3/5] atomic: convert to uatomic
2021-09-25 23:15 ` Dave Chinner
@ 2021-09-25 23:18 ` Eric Sandeen
2021-09-25 23:49 ` Dave Chinner
0 siblings, 1 reply; 21+ messages in thread
From: Eric Sandeen @ 2021-09-25 23:18 UTC (permalink / raw)
To: Dave Chinner; +Cc: Chandan Babu R, linux-xfs, Dave Chinner, djwong
On 9/25/21 6:15 PM, Dave Chinner wrote:
> On Fri, Sep 24, 2021 at 05:13:30PM -0500, Eric Sandeen wrote:
>> On 9/24/21 9:09 AM, Chandan Babu R wrote:
>>> From: Dave Chinner <dchinner@redhat.com>
>>>
>>> Now we have liburcu, we can make use of it's atomic variable
>>> implementation. It is almost identical to the kernel API - it's just
>>> got a "uatomic" prefix. liburcu also provides all the same aomtic
>>> variable memory barriers as the kernel, so if we pull memory barrier
>>> dependent kernel code across, it will just work with the right
>>> barrier wrappers.
>>>
>>> This is preparation the addition of more extensive atomic operations
>>> the that kernel buffer cache requires to function correctly.
>>>
>>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>>> [chandan.babu@oracle.com: Swap order of arguments provided to atomic[64]_[add|sub]()]
>>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>>> ---
>>> include/atomic.h | 65 ++++++++++++++++++++++++++++++++++++++++--------
>>> 1 file changed, 54 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/atomic.h b/include/atomic.h
>>> index e0e1ba84..99cb85d3 100644
>>> --- a/include/atomic.h
>>> +++ b/include/atomic.h
>>> @@ -7,21 +7,64 @@
>>> #define __ATOMIC_H__
>>> /*
>>> - * Warning: These are not really atomic at all. They are wrappers around the
>>> - * kernel atomic variable interface. If we do need these variables to be atomic
>>> - * (due to multithreading of the code that uses them) we need to add some
>>> - * pthreads magic here.
>>> + * Atomics are provided by liburcu.
>>> + *
>>> + * API and guidelines for which operations provide memory barriers is here:
>>> + *
>>> + * https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
>>> + *
>>> + * Unlike the kernel, the same interface supports 32 and 64 bit atomic integers.
>>
>> Given this, anyone have any objection to putting the #defines together at the
>> top, rather than hiding the 64 variants at the end of the file?
>
> I wanted to keep the -APIs- separate, because all the kernel
> atomic/atomic64 stuff is already separate and type checked. I don't
> see any point in commingling the two different atomic type APIs
> just because the implementation ends up being the same and that some
> wrappers are defines and others are static inline code.
>
> Ideally, the wrappers should all be static inlines so we get correct
> atomic_t/atomic64_t type checking in userspace. Those are the types
> we care about in terms of libxfs, so to typecheck the API properly
> these should -all- be static inlines. The patch as it stands was a
> "get it working properly" patch, not a "finalised, strictly correct
> API" patch. That was somethign for "down the road" as I polished the
> patchset ready for eventual review.....
Ok. Well, I was only talking about moving lines in your patch, nothing functional
at all. And ... that's why I had asked earlier (I think?) if your patch was
considered ready for review/merge, or just a demonstration of things to come.
So I guess changing it to a static inline as you suggest should be done before
merge. Anything else like that that you don't actually consider quite ready,
in the first 3 patches?
Thanks,
-Eric
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 3/5] atomic: convert to uatomic
2021-09-25 23:18 ` Eric Sandeen
@ 2021-09-25 23:49 ` Dave Chinner
0 siblings, 0 replies; 21+ messages in thread
From: Dave Chinner @ 2021-09-25 23:49 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Chandan Babu R, linux-xfs, Dave Chinner, djwong
On Sat, Sep 25, 2021 at 06:18:58PM -0500, Eric Sandeen wrote:
> On 9/25/21 6:15 PM, Dave Chinner wrote:
> > On Fri, Sep 24, 2021 at 05:13:30PM -0500, Eric Sandeen wrote:
> > > On 9/24/21 9:09 AM, Chandan Babu R wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > >
> > > > Now we have liburcu, we can make use of it's atomic variable
> > > > implementation. It is almost identical to the kernel API - it's just
> > > > got a "uatomic" prefix. liburcu also provides all the same aomtic
> > > > variable memory barriers as the kernel, so if we pull memory barrier
> > > > dependent kernel code across, it will just work with the right
> > > > barrier wrappers.
> > > >
> > > > This is preparation the addition of more extensive atomic operations
> > > > the that kernel buffer cache requires to function correctly.
> > > >
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > [chandan.babu@oracle.com: Swap order of arguments provided to atomic[64]_[add|sub]()]
> > > > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> > > > ---
> > > > include/atomic.h | 65 ++++++++++++++++++++++++++++++++++++++++--------
> > > > 1 file changed, 54 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/include/atomic.h b/include/atomic.h
> > > > index e0e1ba84..99cb85d3 100644
> > > > --- a/include/atomic.h
> > > > +++ b/include/atomic.h
> > > > @@ -7,21 +7,64 @@
> > > > #define __ATOMIC_H__
> > > > /*
> > > > - * Warning: These are not really atomic at all. They are wrappers around the
> > > > - * kernel atomic variable interface. If we do need these variables to be atomic
> > > > - * (due to multithreading of the code that uses them) we need to add some
> > > > - * pthreads magic here.
> > > > + * Atomics are provided by liburcu.
> > > > + *
> > > > + * API and guidelines for which operations provide memory barriers is here:
> > > > + *
> > > > + * https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
> > > > + *
> > > > + * Unlike the kernel, the same interface supports 32 and 64 bit atomic integers.
> > >
> > > Given this, anyone have any objection to putting the #defines together at the
> > > top, rather than hiding the 64 variants at the end of the file?
> >
> > I wanted to keep the -APIs- separate, because all the kernel
> > atomic/atomic64 stuff is already separate and type checked. I don't
> > see any point in commingling the two different atomic type APIs
> > just because the implementation ends up being the same and that some
> > wrappers are defines and others are static inline code.
> >
> > Ideally, the wrappers should all be static inlines so we get correct
> > atomic_t/atomic64_t type checking in userspace. Those are the types
> > we care about in terms of libxfs, so to typecheck the API properly
> > these should -all- be static inlines. The patch as it stands was a
> > "get it working properly" patch, not a "finalised, strictly correct
> > API" patch. That was somethign for "down the road" as I polished the
> > patchset ready for eventual review.....
>
> Ok. Well, I was only talking about moving lines in your patch, nothing functional
> at all. And ... that's why I had asked earlier (I think?) if your patch was
> considered ready for review/merge, or just a demonstration of things to come.
I though you were asking "does it work/been tested enough to merge"
to which I answered "yes". I did point out that it was a quick
forward port, so stuff like variables the wrong way around in
wrappers I had to add for the forward port shouldn't be a surprise.
> So I guess changing it to a static inline as you suggest should be done before
> merge.
I don't see that as necessary before merging it. It's the direction
these wrappers need to move in so that we get better consistency in
libxfs between user and kernel space. We don't have to do everyone
at once and make code pristine perfect before merging it. Merging
functional, working code is far better than trying to polish off
every rough edge of every patch before considering them for merge..
> Anything else like that that you don't actually consider quite ready,
> in the first 3 patches?
Nope.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V2 4/5] libxfs: add kernel-compatible completion API
2021-09-24 14:09 [PATCH V2 0/5] xfsprogs: generic serialisation primitives Chandan Babu R
` (2 preceding siblings ...)
2021-09-24 14:09 ` [PATCH V2 3/5] atomic: convert to uatomic Chandan Babu R
@ 2021-09-24 14:09 ` Chandan Babu R
2021-09-24 23:02 ` Eric Sandeen
2021-09-24 14:09 ` [PATCH V2 5/5] libxfs: add wrappers for kernel semaphores Chandan Babu R
4 siblings, 1 reply; 21+ messages in thread
From: Chandan Babu R @ 2021-09-24 14:09 UTC (permalink / raw)
To: linux-xfs; +Cc: Dave Chinner, david, sandeen, djwong
From: Dave Chinner <dchinner@redhat.com>
This is needed for the kernel buffer cache conversion to be able
to wait on IO synchrnously. It is implemented with pthread mutexes
and conditional variables.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
include/Makefile | 1 +
include/completion.h | 61 ++++++++++++++++++++++++++++++++++++++++++++
include/libxfs.h | 1 +
libxfs/libxfs_priv.h | 1 +
4 files changed, 64 insertions(+)
create mode 100644 include/completion.h
diff --git a/include/Makefile b/include/Makefile
index f7c40a5c..98031e70 100644
--- a/include/Makefile
+++ b/include/Makefile
@@ -12,6 +12,7 @@ LIBHFILES = libxfs.h \
atomic.h \
bitops.h \
cache.h \
+ completion.h \
hlist.h \
kmem.h \
list.h \
diff --git a/include/completion.h b/include/completion.h
new file mode 100644
index 00000000..92194c3f
--- /dev/null
+++ b/include/completion.h
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 RedHat, Inc.
+ * All Rights Reserved.
+ */
+#ifndef __LIBXFS_COMPLETION_H__
+#define __LIBXFS_COMPLETION_H__
+
+/*
+ * This implements kernel compatible completion semantics. This is slightly
+ * different to the way pthread conditional variables work in that completions
+ * can be signalled before the waiter tries to wait on the variable. In the
+ * pthread case, the completion is ignored and the waiter goes to sleep, whilst
+ * the kernel will see that the completion has already been completed and so
+ * will not block. This is handled through the addition of the the @signalled
+ * flag in the struct completion.
+ */
+struct completion {
+ pthread_mutex_t lock;
+ pthread_cond_t cond;
+ bool signalled; /* for kernel completion behaviour */
+ int waiters;
+};
+
+static inline void
+init_completion(struct completion *w)
+{
+ pthread_mutex_init(&w->lock, NULL);
+ pthread_cond_init(&w->cond, NULL);
+ w->signalled = false;
+}
+
+static inline void
+complete(struct completion *w)
+{
+ pthread_mutex_lock(&w->lock);
+ w->signalled = true;
+ pthread_cond_broadcast(&w->cond);
+ pthread_mutex_unlock(&w->lock);
+}
+
+/*
+ * Support for mulitple waiters requires that we count the number of waiters
+ * we have and only clear the signalled variable once all those waiters have
+ * been woken.
+ */
+static inline void
+wait_for_completion(struct completion *w)
+{
+ pthread_mutex_lock(&w->lock);
+ if (!w->signalled) {
+ w->waiters++;
+ pthread_cond_wait(&w->cond, &w->lock);
+ w->waiters--;
+ }
+ if (!w->waiters)
+ w->signalled = false;
+ pthread_mutex_unlock(&w->lock);
+}
+
+#endif /* __LIBXFS_COMPLETION_H__ */
diff --git a/include/libxfs.h b/include/libxfs.h
index a494a1d4..61475347 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -19,6 +19,7 @@
#include "libfrog/radix-tree.h"
#include "atomic.h"
#include "spinlock.h"
+#include "completion.h"
#include "xfs_types.h"
#include "xfs_fs.h"
diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index e1e90268..9f28fd90 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -49,6 +49,7 @@
#include "libfrog/radix-tree.h"
#include "atomic.h"
#include "spinlock.h"
+#include "completion.h"
#include "xfs_types.h"
#include "xfs_arch.h"
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V2 4/5] libxfs: add kernel-compatible completion API
2021-09-24 14:09 ` [PATCH V2 4/5] libxfs: add kernel-compatible completion API Chandan Babu R
@ 2021-09-24 23:02 ` Eric Sandeen
2021-09-25 10:29 ` Chandan Babu R
0 siblings, 1 reply; 21+ messages in thread
From: Eric Sandeen @ 2021-09-24 23:02 UTC (permalink / raw)
To: Chandan Babu R, linux-xfs; +Cc: Dave Chinner, david, djwong
On 9/24/21 9:09 AM, Chandan Babu R wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> This is needed for the kernel buffer cache conversion to be able
> to wait on IO synchrnously. It is implemented with pthread mutexes
> and conditional variables.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
I am inclined to not merge patches 4 or 5 until there's something that
uses it. It can be merged and tested together with consumers, rather
than adding unused code at this point. Thoughts?
-Eric
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 4/5] libxfs: add kernel-compatible completion API
2021-09-24 23:02 ` Eric Sandeen
@ 2021-09-25 10:29 ` Chandan Babu R
2021-09-27 20:33 ` Darrick J. Wong
2021-09-27 20:55 ` Dave Chinner
0 siblings, 2 replies; 21+ messages in thread
From: Chandan Babu R @ 2021-09-25 10:29 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs, Dave Chinner, david, djwong
On 25 Sep 2021 at 04:32, Eric Sandeen wrote:
> On 9/24/21 9:09 AM, Chandan Babu R wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>> This is needed for the kernel buffer cache conversion to be able
>> to wait on IO synchrnously. It is implemented with pthread mutexes
>> and conditional variables.
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>
> I am inclined to not merge patches 4 or 5 until there's something that
> uses it. It can be merged and tested together with consumers, rather
> than adding unused code at this point. Thoughts?
>
I think I will let Dave answer this question since I believe he most likely
has a roadmap on when the consumers will land.
--
chandan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 4/5] libxfs: add kernel-compatible completion API
2021-09-25 10:29 ` Chandan Babu R
@ 2021-09-27 20:33 ` Darrick J. Wong
2021-09-27 20:55 ` Dave Chinner
1 sibling, 0 replies; 21+ messages in thread
From: Darrick J. Wong @ 2021-09-27 20:33 UTC (permalink / raw)
To: Chandan Babu R; +Cc: Eric Sandeen, linux-xfs, Dave Chinner, david
On Sat, Sep 25, 2021 at 03:59:00PM +0530, Chandan Babu R wrote:
> On 25 Sep 2021 at 04:32, Eric Sandeen wrote:
> > On 9/24/21 9:09 AM, Chandan Babu R wrote:
> >> From: Dave Chinner <dchinner@redhat.com>
> >> This is needed for the kernel buffer cache conversion to be able
> >> to wait on IO synchrnously. It is implemented with pthread mutexes
> >> and conditional variables.
> >> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> >
> > I am inclined to not merge patches 4 or 5 until there's something that
> > uses it. It can be merged and tested together with consumers, rather
> > than adding unused code at this point. Thoughts?
> >
>
> I think I will let Dave answer this question since I believe he most likely
> has a roadmap on when the consumers will land.
Technically speaking, one /could/ port xfs_scrub to use the kernel
completion API instead of calling pthread APIs directly, but I don't see
much gain from churning that.
--D
> --
> chandan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V2 4/5] libxfs: add kernel-compatible completion API
2021-09-25 10:29 ` Chandan Babu R
2021-09-27 20:33 ` Darrick J. Wong
@ 2021-09-27 20:55 ` Dave Chinner
1 sibling, 0 replies; 21+ messages in thread
From: Dave Chinner @ 2021-09-27 20:55 UTC (permalink / raw)
To: Chandan Babu R; +Cc: Eric Sandeen, linux-xfs, Dave Chinner, djwong
On Sat, Sep 25, 2021 at 03:59:00PM +0530, Chandan Babu R wrote:
> On 25 Sep 2021 at 04:32, Eric Sandeen wrote:
> > On 9/24/21 9:09 AM, Chandan Babu R wrote:
> >> From: Dave Chinner <dchinner@redhat.com>
> >> This is needed for the kernel buffer cache conversion to be able
> >> to wait on IO synchrnously. It is implemented with pthread mutexes
> >> and conditional variables.
> >> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> >
> > I am inclined to not merge patches 4 or 5 until there's something that
> > uses it. It can be merged and tested together with consumers, rather
> > than adding unused code at this point. Thoughts?
> >
>
> I think I will let Dave answer this question since I believe he most likely
> has a roadmap on when the consumers will land.
Don't care, as long as everyone agrees with the direction. FWIW, I'm
much more likely to use this completion interface in new userspace
code than I am to use pthreads directly...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V2 5/5] libxfs: add wrappers for kernel semaphores
2021-09-24 14:09 [PATCH V2 0/5] xfsprogs: generic serialisation primitives Chandan Babu R
` (3 preceding siblings ...)
2021-09-24 14:09 ` [PATCH V2 4/5] libxfs: add kernel-compatible completion API Chandan Babu R
@ 2021-09-24 14:09 ` Chandan Babu R
4 siblings, 0 replies; 21+ messages in thread
From: Chandan Babu R @ 2021-09-24 14:09 UTC (permalink / raw)
To: linux-xfs; +Cc: Dave Chinner, david, sandeen, djwong
From: Dave Chinner <dchinner@redhat.com>
Implemented via pthread mutexes.
On Linux, fast pthread mutexes don't actaully check which thread
owns the lock on unlock, so can be used in situations where the
unlock occurs in a different thread to the lock. This is
non-portable behaviour, so if other platforms are supported, this
may need to be converted to posix semaphores.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
include/Makefile | 1 +
include/libxfs.h | 1 +
include/sema.h | 35 +++++++++++++++++++++++++++++++++++
libxfs/libxfs_priv.h | 1 +
4 files changed, 38 insertions(+)
create mode 100644 include/sema.h
diff --git a/include/Makefile b/include/Makefile
index 98031e70..ce89d023 100644
--- a/include/Makefile
+++ b/include/Makefile
@@ -17,6 +17,7 @@ LIBHFILES = libxfs.h \
kmem.h \
list.h \
parent.h \
+ sema.h \
spinlock.h \
xfs_inode.h \
xfs_log_recover.h \
diff --git a/include/libxfs.h b/include/libxfs.h
index 61475347..ca5a21b0 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -20,6 +20,7 @@
#include "atomic.h"
#include "spinlock.h"
#include "completion.h"
+#include "sema.h"
#include "xfs_types.h"
#include "xfs_fs.h"
diff --git a/include/sema.h b/include/sema.h
new file mode 100644
index 00000000..bcccb156
--- /dev/null
+++ b/include/sema.h
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019-20 RedHat, Inc.
+ * All Rights Reserved.
+ */
+#ifndef __LIBXFS_SEMA_H__
+#define __LIBXFS_SEMA_H__
+
+/*
+ * This implements kernel compatible semaphore _exclusion_ semantics. It does
+ * not implement counting semaphore behaviour.
+ *
+ * This makes use of the fact that fast pthread mutexes on Linux don't check
+ * that the unlocker is the same thread that locked the mutex, and hence can be
+ * unlocked in a different thread safely.
+ *
+ * If this needs to be portable or we require counting semaphore behaviour in
+ * libxfs code, this requires re-implementation based on posix semaphores.
+ */
+struct semaphore {
+ pthread_mutex_t lock;
+};
+
+#define sema_init(l, nolock) \
+do { \
+ pthread_mutex_init(&(l)->lock, NULL); \
+ if (!nolock) \
+ pthread_mutex_lock(&(l)->lock); \
+} while (0)
+
+#define down(l) pthread_mutex_lock(&(l)->lock)
+#define down_trylock(l) pthread_mutex_trylock(&(l)->lock)
+#define up(l) pthread_mutex_unlock(&(l)->lock)
+
+#endif /* __LIBXFS_SEMA_H__ */
diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 9f28fd90..1fc243cf 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -50,6 +50,7 @@
#include "atomic.h"
#include "spinlock.h"
#include "completion.h"
+#include "sema.h"
#include "xfs_types.h"
#include "xfs_arch.h"
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread