* [PATCH 1/6] libfrog: fix overly sleep workqueues
2023-09-12 19:39 [PATCHSET 0/6] xfsprogs: minor fixes Darrick J. Wong
@ 2023-09-12 19:39 ` Darrick J. Wong
2023-09-13 12:41 ` Carlos Maiolino
2023-10-05 12:34 ` Carlos Maiolino
2023-09-12 19:39 ` [PATCH 2/6] libfrog: don't fail on XFS_FSOP_GEOM_FLAGS_NREXT64 in xfrog_bulkstat_single5 Darrick J. Wong
` (4 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Darrick J. Wong @ 2023-09-12 19:39 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
I discovered the following bad behavior in the workqueue code when I
noticed that xfs_scrub was running single-threaded despite having 4
virtual CPUs allocated to the VM. I observed this sequence:
Thread 1 WQ1 WQ2...N
workqueue_create
<start up>
pthread_cond_wait
<start up>
pthread_cond_wait
workqueue_add
next_item == NULL
pthread_cond_signal
workqueue_add
next_item != NULL
<do not pthread_cond_signal>
<receives wakeup>
<run first item>
workqueue_add
next_item != NULL
<do not pthread_cond_signal>
<run second item>
<run third item>
pthread_cond_wait
workqueue_terminate
pthread_cond_broadcast
<receives wakeup>
<nothing to do, exits>
<wakes up again>
<nothing to do, exits>
Notice how threads WQ2...N are completely idle while WQ1 ends up doing
all the work! That wasn't the point of a worker pool! Observe that
thread 1 manages to queue two work items before WQ1 pulls the first item
off the queue. When thread 1 queues the third item, it sees that
next_item is not NULL, so it doesn't wake a worker. If thread 1 queues
all the N work that it has before WQ1 empties the queue, then none of
the other thread get woken up.
Fix this by maintaining a count of the number of active threads, and
using that to wake either the sole idle thread, or all the threads if
there are many that are idle. This dramatically improves startup
behavior of the workqueue and eliminates the collapse case.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
libfrog/workqueue.c | 34 ++++++++++++++++++++++++----------
libfrog/workqueue.h | 1 +
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
index 702a53e2f3c..db5b3f68bc5 100644
--- a/libfrog/workqueue.c
+++ b/libfrog/workqueue.c
@@ -26,8 +26,8 @@ workqueue_thread(void *arg)
* Check for notification to exit after every chunk of work.
*/
rcu_register_thread();
+ pthread_mutex_lock(&wq->lock);
while (1) {
- pthread_mutex_lock(&wq->lock);
/*
* Wait for work.
@@ -36,10 +36,8 @@ workqueue_thread(void *arg)
assert(wq->item_count == 0);
pthread_cond_wait(&wq->wakeup, &wq->lock);
}
- if (wq->next_item == NULL && wq->terminate) {
- pthread_mutex_unlock(&wq->lock);
+ if (wq->next_item == NULL && wq->terminate)
break;
- }
/*
* Dequeue work from the head of the list. If the queue was
@@ -57,11 +55,16 @@ workqueue_thread(void *arg)
/* more work, wake up another worker */
pthread_cond_signal(&wq->wakeup);
}
+ wq->active_threads++;
pthread_mutex_unlock(&wq->lock);
(wi->function)(wi->queue, wi->index, wi->arg);
free(wi);
+
+ pthread_mutex_lock(&wq->lock);
+ wq->active_threads--;
}
+ pthread_mutex_unlock(&wq->lock);
rcu_unregister_thread();
return NULL;
@@ -170,12 +173,6 @@ workqueue_add(
restart:
if (wq->next_item == NULL) {
assert(wq->item_count == 0);
- ret = -pthread_cond_signal(&wq->wakeup);
- if (ret) {
- pthread_mutex_unlock(&wq->lock);
- free(wi);
- return ret;
- }
wq->next_item = wi;
} else {
/* throttle on a full queue if configured */
@@ -192,6 +189,23 @@ workqueue_add(
}
wq->last_item = wi;
wq->item_count++;
+
+ if (wq->active_threads == wq->thread_count - 1) {
+ /* One thread is idle, wake it */
+ ret = -pthread_cond_signal(&wq->wakeup);
+ if (ret) {
+ pthread_mutex_unlock(&wq->lock);
+ return ret;
+ }
+ } else if (wq->active_threads < wq->thread_count) {
+ /* Multiple threads are idle, wake everyone */
+ ret = -pthread_cond_broadcast(&wq->wakeup);
+ if (ret) {
+ pthread_mutex_unlock(&wq->lock);
+ return ret;
+ }
+ }
+
pthread_mutex_unlock(&wq->lock);
return 0;
diff --git a/libfrog/workqueue.h b/libfrog/workqueue.h
index a9c108d0e66..edbe12fabab 100644
--- a/libfrog/workqueue.h
+++ b/libfrog/workqueue.h
@@ -29,6 +29,7 @@ struct workqueue {
pthread_cond_t wakeup;
unsigned int item_count;
unsigned int thread_count;
+ unsigned int active_threads;
bool terminate;
bool terminated;
int max_queued;
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/6] libfrog: fix overly sleep workqueues
2023-09-12 19:39 ` [PATCH 1/6] libfrog: fix overly sleep workqueues Darrick J. Wong
@ 2023-09-13 12:41 ` Carlos Maiolino
2023-10-05 12:34 ` Carlos Maiolino
1 sibling, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-09-13 12:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:39:41PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> I discovered the following bad behavior in the workqueue code when I
> noticed that xfs_scrub was running single-threaded despite having 4
> virtual CPUs allocated to the VM. I observed this sequence:
>
> Thread 1 WQ1 WQ2...N
> workqueue_create
> <start up>
> pthread_cond_wait
> <start up>
> pthread_cond_wait
> workqueue_add
> next_item == NULL
> pthread_cond_signal
>
> workqueue_add
> next_item != NULL
> <do not pthread_cond_signal>
>
> <receives wakeup>
> <run first item>
>
> workqueue_add
> next_item != NULL
> <do not pthread_cond_signal>
>
> <run second item>
> <run third item>
> pthread_cond_wait
>
> workqueue_terminate
> pthread_cond_broadcast
> <receives wakeup>
> <nothing to do, exits>
> <wakes up again>
> <nothing to do, exits>
>
> Notice how threads WQ2...N are completely idle while WQ1 ends up doing
> all the work! That wasn't the point of a worker pool! Observe that
> thread 1 manages to queue two work items before WQ1 pulls the first item
> off the queue. When thread 1 queues the third item, it sees that
> next_item is not NULL, so it doesn't wake a worker. If thread 1 queues
> all the N work that it has before WQ1 empties the queue, then none of
> the other thread get woken up.
>
> Fix this by maintaining a count of the number of active threads, and
> using that to wake either the sole idle thread, or all the threads if
> there are many that are idle. This dramatically improves startup
> behavior of the workqueue and eliminates the collapse case.
>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> libfrog/workqueue.c | 34 ++++++++++++++++++++++++----------
> libfrog/workqueue.h | 1 +
> 2 files changed, 25 insertions(+), 10 deletions(-)
>
>
> diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
> index 702a53e2f3c..db5b3f68bc5 100644
> --- a/libfrog/workqueue.c
> +++ b/libfrog/workqueue.c
> @@ -26,8 +26,8 @@ workqueue_thread(void *arg)
> * Check for notification to exit after every chunk of work.
> */
> rcu_register_thread();
> + pthread_mutex_lock(&wq->lock);
> while (1) {
> - pthread_mutex_lock(&wq->lock);
>
> /*
> * Wait for work.
> @@ -36,10 +36,8 @@ workqueue_thread(void *arg)
> assert(wq->item_count == 0);
> pthread_cond_wait(&wq->wakeup, &wq->lock);
> }
> - if (wq->next_item == NULL && wq->terminate) {
> - pthread_mutex_unlock(&wq->lock);
> + if (wq->next_item == NULL && wq->terminate)
> break;
> - }
>
> /*
> * Dequeue work from the head of the list. If the queue was
> @@ -57,11 +55,16 @@ workqueue_thread(void *arg)
> /* more work, wake up another worker */
> pthread_cond_signal(&wq->wakeup);
> }
> + wq->active_threads++;
> pthread_mutex_unlock(&wq->lock);
>
> (wi->function)(wi->queue, wi->index, wi->arg);
> free(wi);
> +
> + pthread_mutex_lock(&wq->lock);
> + wq->active_threads--;
> }
> + pthread_mutex_unlock(&wq->lock);
> rcu_unregister_thread();
>
> return NULL;
> @@ -170,12 +173,6 @@ workqueue_add(
> restart:
> if (wq->next_item == NULL) {
> assert(wq->item_count == 0);
> - ret = -pthread_cond_signal(&wq->wakeup);
> - if (ret) {
> - pthread_mutex_unlock(&wq->lock);
> - free(wi);
> - return ret;
> - }
> wq->next_item = wi;
> } else {
> /* throttle on a full queue if configured */
> @@ -192,6 +189,23 @@ workqueue_add(
> }
> wq->last_item = wi;
> wq->item_count++;
> +
> + if (wq->active_threads == wq->thread_count - 1) {
> + /* One thread is idle, wake it */
> + ret = -pthread_cond_signal(&wq->wakeup);
> + if (ret) {
> + pthread_mutex_unlock(&wq->lock);
> + return ret;
> + }
> + } else if (wq->active_threads < wq->thread_count) {
> + /* Multiple threads are idle, wake everyone */
> + ret = -pthread_cond_broadcast(&wq->wakeup);
> + if (ret) {
> + pthread_mutex_unlock(&wq->lock);
> + return ret;
> + }
> + }
> +
> pthread_mutex_unlock(&wq->lock);
>
> return 0;
> diff --git a/libfrog/workqueue.h b/libfrog/workqueue.h
> index a9c108d0e66..edbe12fabab 100644
> --- a/libfrog/workqueue.h
> +++ b/libfrog/workqueue.h
> @@ -29,6 +29,7 @@ struct workqueue {
> pthread_cond_t wakeup;
> unsigned int item_count;
> unsigned int thread_count;
> + unsigned int active_threads;
> bool terminate;
> bool terminated;
> int max_queued;
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 1/6] libfrog: fix overly sleep workqueues
2023-09-12 19:39 ` [PATCH 1/6] libfrog: fix overly sleep workqueues Darrick J. Wong
2023-09-13 12:41 ` Carlos Maiolino
@ 2023-10-05 12:34 ` Carlos Maiolino
1 sibling, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-10-05 12:34 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:39:41PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> I discovered the following bad behavior in the workqueue code when I
> noticed that xfs_scrub was running single-threaded despite having 4
> virtual CPUs allocated to the VM. I observed this sequence:
For some reason I thought I had ack'ed this patch in the past... Maybe a Deja vu,
or the matrix is broken...
in any case:
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Carlos
>
> Thread 1 WQ1 WQ2...N
> workqueue_create
> <start up>
> pthread_cond_wait
> <start up>
> pthread_cond_wait
> workqueue_add
> next_item == NULL
> pthread_cond_signal
>
> workqueue_add
> next_item != NULL
> <do not pthread_cond_signal>
>
> <receives wakeup>
> <run first item>
>
> workqueue_add
> next_item != NULL
> <do not pthread_cond_signal>
>
> <run second item>
> <run third item>
> pthread_cond_wait
>
> workqueue_terminate
> pthread_cond_broadcast
> <receives wakeup>
> <nothing to do, exits>
> <wakes up again>
> <nothing to do, exits>
>
> Notice how threads WQ2...N are completely idle while WQ1 ends up doing
> all the work! That wasn't the point of a worker pool! Observe that
> thread 1 manages to queue two work items before WQ1 pulls the first item
> off the queue. When thread 1 queues the third item, it sees that
> next_item is not NULL, so it doesn't wake a worker. If thread 1 queues
> all the N work that it has before WQ1 empties the queue, then none of
> the other thread get woken up.
>
> Fix this by maintaining a count of the number of active threads, and
> using that to wake either the sole idle thread, or all the threads if
> there are many that are idle. This dramatically improves startup
> behavior of the workqueue and eliminates the collapse case.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> libfrog/workqueue.c | 34 ++++++++++++++++++++++++----------
> libfrog/workqueue.h | 1 +
> 2 files changed, 25 insertions(+), 10 deletions(-)
>
>
> diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
> index 702a53e2f3c..db5b3f68bc5 100644
> --- a/libfrog/workqueue.c
> +++ b/libfrog/workqueue.c
> @@ -26,8 +26,8 @@ workqueue_thread(void *arg)
> * Check for notification to exit after every chunk of work.
> */
> rcu_register_thread();
> + pthread_mutex_lock(&wq->lock);
> while (1) {
> - pthread_mutex_lock(&wq->lock);
>
> /*
> * Wait for work.
> @@ -36,10 +36,8 @@ workqueue_thread(void *arg)
> assert(wq->item_count == 0);
> pthread_cond_wait(&wq->wakeup, &wq->lock);
> }
> - if (wq->next_item == NULL && wq->terminate) {
> - pthread_mutex_unlock(&wq->lock);
> + if (wq->next_item == NULL && wq->terminate)
> break;
> - }
>
> /*
> * Dequeue work from the head of the list. If the queue was
> @@ -57,11 +55,16 @@ workqueue_thread(void *arg)
> /* more work, wake up another worker */
> pthread_cond_signal(&wq->wakeup);
> }
> + wq->active_threads++;
> pthread_mutex_unlock(&wq->lock);
>
> (wi->function)(wi->queue, wi->index, wi->arg);
> free(wi);
> +
> + pthread_mutex_lock(&wq->lock);
> + wq->active_threads--;
> }
> + pthread_mutex_unlock(&wq->lock);
> rcu_unregister_thread();
>
> return NULL;
> @@ -170,12 +173,6 @@ workqueue_add(
> restart:
> if (wq->next_item == NULL) {
> assert(wq->item_count == 0);
> - ret = -pthread_cond_signal(&wq->wakeup);
> - if (ret) {
> - pthread_mutex_unlock(&wq->lock);
> - free(wi);
> - return ret;
> - }
> wq->next_item = wi;
> } else {
> /* throttle on a full queue if configured */
> @@ -192,6 +189,23 @@ workqueue_add(
> }
> wq->last_item = wi;
> wq->item_count++;
> +
> + if (wq->active_threads == wq->thread_count - 1) {
> + /* One thread is idle, wake it */
> + ret = -pthread_cond_signal(&wq->wakeup);
> + if (ret) {
> + pthread_mutex_unlock(&wq->lock);
> + return ret;
> + }
> + } else if (wq->active_threads < wq->thread_count) {
> + /* Multiple threads are idle, wake everyone */
> + ret = -pthread_cond_broadcast(&wq->wakeup);
> + if (ret) {
> + pthread_mutex_unlock(&wq->lock);
> + return ret;
> + }
> + }
> +
> pthread_mutex_unlock(&wq->lock);
>
> return 0;
> diff --git a/libfrog/workqueue.h b/libfrog/workqueue.h
> index a9c108d0e66..edbe12fabab 100644
> --- a/libfrog/workqueue.h
> +++ b/libfrog/workqueue.h
> @@ -29,6 +29,7 @@ struct workqueue {
> pthread_cond_t wakeup;
> unsigned int item_count;
> unsigned int thread_count;
> + unsigned int active_threads;
> bool terminate;
> bool terminated;
> int max_queued;
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/6] libfrog: don't fail on XFS_FSOP_GEOM_FLAGS_NREXT64 in xfrog_bulkstat_single5
2023-09-12 19:39 [PATCHSET 0/6] xfsprogs: minor fixes Darrick J. Wong
2023-09-12 19:39 ` [PATCH 1/6] libfrog: fix overly sleep workqueues Darrick J. Wong
@ 2023-09-12 19:39 ` Darrick J. Wong
2023-09-13 12:50 ` Carlos Maiolino
2023-10-05 12:34 ` Carlos Maiolino
2023-09-12 19:39 ` [PATCH 3/6] libxfs: use XFS_IGET_CREATE when creating new files Darrick J. Wong
` (3 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Darrick J. Wong @ 2023-09-12 19:39 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
This flag is perfectly acceptable for bulkstatting a single file;
there's no reason not to allow it.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
libfrog/bulkstat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libfrog/bulkstat.c b/libfrog/bulkstat.c
index 0a90947fb29..c863bcb6bf8 100644
--- a/libfrog/bulkstat.c
+++ b/libfrog/bulkstat.c
@@ -53,7 +53,7 @@ xfrog_bulkstat_single5(
struct xfs_bulkstat_req *req;
int ret;
- if (flags & ~(XFS_BULK_IREQ_SPECIAL))
+ if (flags & ~(XFS_BULK_IREQ_SPECIAL | XFS_BULK_IREQ_NREXT64))
return -EINVAL;
if (xfd->fsgeom.flags & XFS_FSOP_GEOM_FLAGS_NREXT64)
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 2/6] libfrog: don't fail on XFS_FSOP_GEOM_FLAGS_NREXT64 in xfrog_bulkstat_single5
2023-09-12 19:39 ` [PATCH 2/6] libfrog: don't fail on XFS_FSOP_GEOM_FLAGS_NREXT64 in xfrog_bulkstat_single5 Darrick J. Wong
@ 2023-09-13 12:50 ` Carlos Maiolino
2023-10-05 12:34 ` Carlos Maiolino
1 sibling, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-09-13 12:50 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:39:47PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> This flag is perfectly acceptable for bulkstatting a single file;
> there's no reason not to allow it.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> libfrog/bulkstat.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> diff --git a/libfrog/bulkstat.c b/libfrog/bulkstat.c
> index 0a90947fb29..c863bcb6bf8 100644
> --- a/libfrog/bulkstat.c
> +++ b/libfrog/bulkstat.c
> @@ -53,7 +53,7 @@ xfrog_bulkstat_single5(
> struct xfs_bulkstat_req *req;
> int ret;
>
> - if (flags & ~(XFS_BULK_IREQ_SPECIAL))
> + if (flags & ~(XFS_BULK_IREQ_SPECIAL | XFS_BULK_IREQ_NREXT64))
> return -EINVAL;
>
> if (xfd->fsgeom.flags & XFS_FSOP_GEOM_FLAGS_NREXT64)
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/6] libfrog: don't fail on XFS_FSOP_GEOM_FLAGS_NREXT64 in xfrog_bulkstat_single5
2023-09-12 19:39 ` [PATCH 2/6] libfrog: don't fail on XFS_FSOP_GEOM_FLAGS_NREXT64 in xfrog_bulkstat_single5 Darrick J. Wong
2023-09-13 12:50 ` Carlos Maiolino
@ 2023-10-05 12:34 ` Carlos Maiolino
1 sibling, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-10-05 12:34 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:39:47PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> This flag is perfectly acceptable for bulkstatting a single file;
> there's no reason not to allow it.
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Carlos
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> libfrog/bulkstat.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> diff --git a/libfrog/bulkstat.c b/libfrog/bulkstat.c
> index 0a90947fb29..c863bcb6bf8 100644
> --- a/libfrog/bulkstat.c
> +++ b/libfrog/bulkstat.c
> @@ -53,7 +53,7 @@ xfrog_bulkstat_single5(
> struct xfs_bulkstat_req *req;
> int ret;
>
> - if (flags & ~(XFS_BULK_IREQ_SPECIAL))
> + if (flags & ~(XFS_BULK_IREQ_SPECIAL | XFS_BULK_IREQ_NREXT64))
> return -EINVAL;
>
> if (xfd->fsgeom.flags & XFS_FSOP_GEOM_FLAGS_NREXT64)
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/6] libxfs: use XFS_IGET_CREATE when creating new files
2023-09-12 19:39 [PATCHSET 0/6] xfsprogs: minor fixes Darrick J. Wong
2023-09-12 19:39 ` [PATCH 1/6] libfrog: fix overly sleep workqueues Darrick J. Wong
2023-09-12 19:39 ` [PATCH 2/6] libfrog: don't fail on XFS_FSOP_GEOM_FLAGS_NREXT64 in xfrog_bulkstat_single5 Darrick J. Wong
@ 2023-09-12 19:39 ` Darrick J. Wong
2023-09-13 12:58 ` Carlos Maiolino
` (2 more replies)
2023-09-12 19:39 ` [PATCH 4/6] xfs_scrub: actually return errno from check_xattr_ns_names Darrick J. Wong
` (2 subsequent siblings)
5 siblings, 3 replies; 23+ messages in thread
From: Darrick J. Wong @ 2023-09-12 19:39 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Use this flag to check that newly allocated inodes are, in fact,
unallocated. This matches the kernel, and prevents userspace programs
from making latent corruptions worse by unintentionally crosslinking
files.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
libxfs/util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libxfs/util.c b/libxfs/util.c
index e7d3497ec96..8f79b0cd17b 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -260,7 +260,7 @@ libxfs_init_new_inode(
unsigned int flags;
int error;
- error = libxfs_iget(tp->t_mountp, tp, ino, 0, &ip);
+ error = libxfs_iget(tp->t_mountp, tp, ino, XFS_IGET_CREATE, &ip);
if (error != 0)
return error;
ASSERT(ip != NULL);
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 3/6] libxfs: use XFS_IGET_CREATE when creating new files
2023-09-12 19:39 ` [PATCH 3/6] libxfs: use XFS_IGET_CREATE when creating new files Darrick J. Wong
@ 2023-09-13 12:58 ` Carlos Maiolino
2023-09-14 18:24 ` Bill O'Donnell
2023-10-05 12:35 ` Carlos Maiolino
2 siblings, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-09-13 12:58 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:39:52PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Use this flag to check that newly allocated inodes are, in fact,
> unallocated. This matches the kernel, and prevents userspace programs
> from making latent corruptions worse by unintentionally crosslinking
> files.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks good,
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> libxfs/util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> diff --git a/libxfs/util.c b/libxfs/util.c
> index e7d3497ec96..8f79b0cd17b 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -260,7 +260,7 @@ libxfs_init_new_inode(
> unsigned int flags;
> int error;
>
> - error = libxfs_iget(tp->t_mountp, tp, ino, 0, &ip);
> + error = libxfs_iget(tp->t_mountp, tp, ino, XFS_IGET_CREATE, &ip);
> if (error != 0)
> return error;
> ASSERT(ip != NULL);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/6] libxfs: use XFS_IGET_CREATE when creating new files
2023-09-12 19:39 ` [PATCH 3/6] libxfs: use XFS_IGET_CREATE when creating new files Darrick J. Wong
2023-09-13 12:58 ` Carlos Maiolino
@ 2023-09-14 18:24 ` Bill O'Donnell
2023-10-05 12:35 ` Carlos Maiolino
2 siblings, 0 replies; 23+ messages in thread
From: Bill O'Donnell @ 2023-09-14 18:24 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Tue, Sep 12, 2023 at 12:39:52PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Use this flag to check that newly allocated inodes are, in fact,
> unallocated. This matches the kernel, and prevents userspace programs
> from making latent corruptions worse by unintentionally crosslinking
> files.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>
> ---
> libxfs/util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> diff --git a/libxfs/util.c b/libxfs/util.c
> index e7d3497ec96..8f79b0cd17b 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -260,7 +260,7 @@ libxfs_init_new_inode(
> unsigned int flags;
> int error;
>
> - error = libxfs_iget(tp->t_mountp, tp, ino, 0, &ip);
> + error = libxfs_iget(tp->t_mountp, tp, ino, XFS_IGET_CREATE, &ip);
> if (error != 0)
> return error;
> ASSERT(ip != NULL);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/6] libxfs: use XFS_IGET_CREATE when creating new files
2023-09-12 19:39 ` [PATCH 3/6] libxfs: use XFS_IGET_CREATE when creating new files Darrick J. Wong
2023-09-13 12:58 ` Carlos Maiolino
2023-09-14 18:24 ` Bill O'Donnell
@ 2023-10-05 12:35 ` Carlos Maiolino
2 siblings, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-10-05 12:35 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:39:52PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Use this flag to check that newly allocated inodes are, in fact,
> unallocated. This matches the kernel, and prevents userspace programs
> from making latent corruptions worse by unintentionally crosslinking
> files.
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Carlos
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> libxfs/util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> diff --git a/libxfs/util.c b/libxfs/util.c
> index e7d3497ec96..8f79b0cd17b 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -260,7 +260,7 @@ libxfs_init_new_inode(
> unsigned int flags;
> int error;
>
> - error = libxfs_iget(tp->t_mountp, tp, ino, 0, &ip);
> + error = libxfs_iget(tp->t_mountp, tp, ino, XFS_IGET_CREATE, &ip);
> if (error != 0)
> return error;
> ASSERT(ip != NULL);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/6] xfs_scrub: actually return errno from check_xattr_ns_names
2023-09-12 19:39 [PATCHSET 0/6] xfsprogs: minor fixes Darrick J. Wong
` (2 preceding siblings ...)
2023-09-12 19:39 ` [PATCH 3/6] libxfs: use XFS_IGET_CREATE when creating new files Darrick J. Wong
@ 2023-09-12 19:39 ` Darrick J. Wong
2023-09-13 13:00 ` Carlos Maiolino
2023-10-05 12:36 ` Carlos Maiolino
2023-09-12 19:40 ` [PATCH 5/6] xfs_repair: set aformat and anextents correctly when clearing the attr fork Darrick J. Wong
2023-09-12 19:40 ` [PATCH 6/6] libxfs: fix atomic64_t detection on x86 32-bit architectures Darrick J. Wong
5 siblings, 2 replies; 23+ messages in thread
From: Darrick J. Wong @ 2023-09-12 19:39 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Actually return the error code when extended attribute checks fail.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
scrub/phase5.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/scrub/phase5.c b/scrub/phase5.c
index 1ef234bff68..31405709657 100644
--- a/scrub/phase5.c
+++ b/scrub/phase5.c
@@ -202,6 +202,7 @@ check_xattr_ns_names(
if (error) {
if (errno == ESTALE)
errno = 0;
+ error = errno;
if (errno)
str_errno(ctx, descr_render(dsc));
}
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 4/6] xfs_scrub: actually return errno from check_xattr_ns_names
2023-09-12 19:39 ` [PATCH 4/6] xfs_scrub: actually return errno from check_xattr_ns_names Darrick J. Wong
@ 2023-09-13 13:00 ` Carlos Maiolino
2023-10-05 12:36 ` Carlos Maiolino
1 sibling, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-09-13 13:00 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:39:58PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Actually return the error code when extended attribute checks fail.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> scrub/phase5.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
>
>
> diff --git a/scrub/phase5.c b/scrub/phase5.c
> index 1ef234bff68..31405709657 100644
> --- a/scrub/phase5.c
> +++ b/scrub/phase5.c
> @@ -202,6 +202,7 @@ check_xattr_ns_names(
> if (error) {
> if (errno == ESTALE)
> errno = 0;
> + error = errno;
> if (errno)
> str_errno(ctx, descr_render(dsc));
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 4/6] xfs_scrub: actually return errno from check_xattr_ns_names
2023-09-12 19:39 ` [PATCH 4/6] xfs_scrub: actually return errno from check_xattr_ns_names Darrick J. Wong
2023-09-13 13:00 ` Carlos Maiolino
@ 2023-10-05 12:36 ` Carlos Maiolino
1 sibling, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-10-05 12:36 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:39:58PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Actually return the error code when extended attribute checks fail.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Carlos
> ---
> scrub/phase5.c | 1 +
> 1 file changed, 1 insertion(+)
>
>
> diff --git a/scrub/phase5.c b/scrub/phase5.c
> index 1ef234bff68..31405709657 100644
> --- a/scrub/phase5.c
> +++ b/scrub/phase5.c
> @@ -202,6 +202,7 @@ check_xattr_ns_names(
> if (error) {
> if (errno == ESTALE)
> errno = 0;
> + error = errno;
> if (errno)
> str_errno(ctx, descr_render(dsc));
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/6] xfs_repair: set aformat and anextents correctly when clearing the attr fork
2023-09-12 19:39 [PATCHSET 0/6] xfsprogs: minor fixes Darrick J. Wong
` (3 preceding siblings ...)
2023-09-12 19:39 ` [PATCH 4/6] xfs_scrub: actually return errno from check_xattr_ns_names Darrick J. Wong
@ 2023-09-12 19:40 ` Darrick J. Wong
2023-09-13 13:02 ` Carlos Maiolino
` (2 more replies)
2023-09-12 19:40 ` [PATCH 6/6] libxfs: fix atomic64_t detection on x86 32-bit architectures Darrick J. Wong
5 siblings, 3 replies; 23+ messages in thread
From: Darrick J. Wong @ 2023-09-12 19:40 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Ever since commit b42db0860e130 ("xfs: enhance dinode verifier"), we've
required that inodes with zero di_forkoff must also have di_aformat ==
EXTENTS and di_naextents == 0. clear_dinode_attr actually does this,
but then both callers inexplicably set di_format = LOCAL. That in turn
causes a verifier failure the next time the xattrs of that file are
read by the kernel. Get rid of the bogus field write.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
repair/dinode.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/repair/dinode.c b/repair/dinode.c
index e534a01b500..c10dd1fa322 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2078,7 +2078,6 @@ process_inode_attr_fork(
if (!no_modify) {
do_warn(_(", clearing attr fork\n"));
*dirty += clear_dinode_attr(mp, dino, lino);
- dino->di_aformat = XFS_DINODE_FMT_LOCAL;
ASSERT(*dirty > 0);
} else {
do_warn(_(", would clear attr fork\n"));
@@ -2135,7 +2134,6 @@ process_inode_attr_fork(
/* clear attributes if not done already */
if (!no_modify) {
*dirty += clear_dinode_attr(mp, dino, lino);
- dino->di_aformat = XFS_DINODE_FMT_LOCAL;
} else {
do_warn(_("would clear attr fork\n"));
}
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 5/6] xfs_repair: set aformat and anextents correctly when clearing the attr fork
2023-09-12 19:40 ` [PATCH 5/6] xfs_repair: set aformat and anextents correctly when clearing the attr fork Darrick J. Wong
@ 2023-09-13 13:02 ` Carlos Maiolino
2023-09-14 18:25 ` Bill O'Donnell
2023-10-05 12:37 ` Carlos Maiolino
2 siblings, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-09-13 13:02 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:40:04PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Ever since commit b42db0860e130 ("xfs: enhance dinode verifier"), we've
> required that inodes with zero di_forkoff must also have di_aformat ==
> EXTENTS and di_naextents == 0. clear_dinode_attr actually does this,
> but then both callers inexplicably set di_format = LOCAL. That in turn
> causes a verifier failure the next time the xattrs of that file are
> read by the kernel. Get rid of the bogus field write.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> repair/dinode.c | 2 --
> 1 file changed, 2 deletions(-)
>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
>
> diff --git a/repair/dinode.c b/repair/dinode.c
> index e534a01b500..c10dd1fa322 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2078,7 +2078,6 @@ process_inode_attr_fork(
> if (!no_modify) {
> do_warn(_(", clearing attr fork\n"));
> *dirty += clear_dinode_attr(mp, dino, lino);
> - dino->di_aformat = XFS_DINODE_FMT_LOCAL;
> ASSERT(*dirty > 0);
> } else {
> do_warn(_(", would clear attr fork\n"));
> @@ -2135,7 +2134,6 @@ process_inode_attr_fork(
> /* clear attributes if not done already */
> if (!no_modify) {
> *dirty += clear_dinode_attr(mp, dino, lino);
> - dino->di_aformat = XFS_DINODE_FMT_LOCAL;
> } else {
> do_warn(_("would clear attr fork\n"));
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/6] xfs_repair: set aformat and anextents correctly when clearing the attr fork
2023-09-12 19:40 ` [PATCH 5/6] xfs_repair: set aformat and anextents correctly when clearing the attr fork Darrick J. Wong
2023-09-13 13:02 ` Carlos Maiolino
@ 2023-09-14 18:25 ` Bill O'Donnell
2023-10-05 12:37 ` Carlos Maiolino
2 siblings, 0 replies; 23+ messages in thread
From: Bill O'Donnell @ 2023-09-14 18:25 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Tue, Sep 12, 2023 at 12:40:04PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Ever since commit b42db0860e130 ("xfs: enhance dinode verifier"), we've
> required that inodes with zero di_forkoff must also have di_aformat ==
> EXTENTS and di_naextents == 0. clear_dinode_attr actually does this,
> but then both callers inexplicably set di_format = LOCAL. That in turn
> causes a verifier failure the next time the xattrs of that file are
> read by the kernel. Get rid of the bogus field write.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>
> ---
> repair/dinode.c | 2 --
> 1 file changed, 2 deletions(-)
>
>
> diff --git a/repair/dinode.c b/repair/dinode.c
> index e534a01b500..c10dd1fa322 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2078,7 +2078,6 @@ process_inode_attr_fork(
> if (!no_modify) {
> do_warn(_(", clearing attr fork\n"));
> *dirty += clear_dinode_attr(mp, dino, lino);
> - dino->di_aformat = XFS_DINODE_FMT_LOCAL;
> ASSERT(*dirty > 0);
> } else {
> do_warn(_(", would clear attr fork\n"));
> @@ -2135,7 +2134,6 @@ process_inode_attr_fork(
> /* clear attributes if not done already */
> if (!no_modify) {
> *dirty += clear_dinode_attr(mp, dino, lino);
> - dino->di_aformat = XFS_DINODE_FMT_LOCAL;
> } else {
> do_warn(_("would clear attr fork\n"));
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/6] xfs_repair: set aformat and anextents correctly when clearing the attr fork
2023-09-12 19:40 ` [PATCH 5/6] xfs_repair: set aformat and anextents correctly when clearing the attr fork Darrick J. Wong
2023-09-13 13:02 ` Carlos Maiolino
2023-09-14 18:25 ` Bill O'Donnell
@ 2023-10-05 12:37 ` Carlos Maiolino
2 siblings, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-10-05 12:37 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Tue, Sep 12, 2023 at 12:40:04PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Ever since commit b42db0860e130 ("xfs: enhance dinode verifier"), we've
> required that inodes with zero di_forkoff must also have di_aformat ==
> EXTENTS and di_naextents == 0. clear_dinode_attr actually does this,
> but then both callers inexplicably set di_format = LOCAL. That in turn
> causes a verifier failure the next time the xattrs of that file are
> read by the kernel. Get rid of the bogus field write.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Carlos
> ---
> repair/dinode.c | 2 --
> 1 file changed, 2 deletions(-)
>
>
> diff --git a/repair/dinode.c b/repair/dinode.c
> index e534a01b500..c10dd1fa322 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2078,7 +2078,6 @@ process_inode_attr_fork(
> if (!no_modify) {
> do_warn(_(", clearing attr fork\n"));
> *dirty += clear_dinode_attr(mp, dino, lino);
> - dino->di_aformat = XFS_DINODE_FMT_LOCAL;
> ASSERT(*dirty > 0);
> } else {
> do_warn(_(", would clear attr fork\n"));
> @@ -2135,7 +2134,6 @@ process_inode_attr_fork(
> /* clear attributes if not done already */
> if (!no_modify) {
> *dirty += clear_dinode_attr(mp, dino, lino);
> - dino->di_aformat = XFS_DINODE_FMT_LOCAL;
> } else {
> do_warn(_("would clear attr fork\n"));
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 6/6] libxfs: fix atomic64_t detection on x86 32-bit architectures
2023-09-12 19:39 [PATCHSET 0/6] xfsprogs: minor fixes Darrick J. Wong
` (4 preceding siblings ...)
2023-09-12 19:40 ` [PATCH 5/6] xfs_repair: set aformat and anextents correctly when clearing the attr fork Darrick J. Wong
@ 2023-09-12 19:40 ` Darrick J. Wong
2023-09-12 19:47 ` [PATCH v1.1 " Darrick J. Wong
5 siblings, 1 reply; 23+ messages in thread
From: Darrick J. Wong @ 2023-09-12 19:40 UTC (permalink / raw)
To: djwong, cem; +Cc: Anthony Iliopoulos, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
xfsprogs during compilation tries to detect if liburcu supports atomic
64-bit ops on the platform it is being compiled on, and if not it falls
back to using pthread mutex locks.
The detection logic for that fallback relies on _uatomic_link_error()
which is a link-time trick used by liburcu that will cause compilation
errors on archs that lack the required support. That only works for the
generic liburcu code though, and it is not implemented for the
x86-specific code.
In practice this means that when xfsprogs is compiled on 32-bit x86
archs will successfully link to liburcu for atomic ops, but liburcu does
not support atomic64_t on those archs. It indicates this during runtime
by generating an illegal instruction that aborts execution, and thus
causes various xfsprogs utils to be segfaulting.
Fix this by executing the liburcu atomic64_t detection code during
configure instead of only relying on the linker error, so that
compilation will properly fall back to pthread mutexes on those archs.
Fixes: 7448af588a2e ("libxfs: fix atomic64_t poorly for 32-bit architectures")
Reported-by: Anthony Iliopoulos <ailiop@suse.com>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
m4/package_urcu.m4 | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/m4/package_urcu.m4 b/m4/package_urcu.m4
index ef116e0cda7..4bb2b886f06 100644
--- a/m4/package_urcu.m4
+++ b/m4/package_urcu.m4
@@ -26,7 +26,11 @@ rcu_init();
#
# Make sure that calling uatomic_inc on a 64-bit integer doesn't cause a link
# error on _uatomic_link_error, which is how liburcu signals that it doesn't
-# support atomic operations on 64-bit data types.
+# support atomic operations on 64-bit data types for its generic
+# implementation (which relies on compiler builtins). For certain archs
+# where liburcu carries its own implementation (such as x86_32), it
+# signals lack of support during runtime by emitting an illegal
+# instruction, so we also need to check CAA_BITS_PER_LONG to detect that.
#
AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
[ AC_MSG_CHECKING([for atomic64_t support in liburcu])
@@ -34,8 +38,11 @@ AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
[ AC_LANG_PROGRAM([[
#define _GNU_SOURCE
#include <urcu.h>
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
]], [[
long long f = 3;
+
+BUILD_BUG_ON(CAA_BITS_PER_LONG < 64);
uatomic_inc(&f);
]])
], have_liburcu_atomic64=yes
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v1.1 6/6] libxfs: fix atomic64_t detection on x86 32-bit architectures
2023-09-12 19:40 ` [PATCH 6/6] libxfs: fix atomic64_t detection on x86 32-bit architectures Darrick J. Wong
@ 2023-09-12 19:47 ` Darrick J. Wong
2023-09-13 13:22 ` Carlos Maiolino
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Darrick J. Wong @ 2023-09-12 19:47 UTC (permalink / raw)
To: cem; +Cc: Anthony Iliopoulos, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
xfsprogs during compilation tries to detect if liburcu supports atomic
64-bit ops on the platform it is being compiled on, and if not it falls
back to using pthread mutex locks.
The detection logic for that fallback relies on _uatomic_link_error()
which is a link-time trick used by liburcu that will cause compilation
errors on archs that lack the required support. That only works for the
generic liburcu code though, and it is not implemented for the
x86-specific code.
In practice this means that when xfsprogs is compiled on 32-bit x86
archs will successfully link to liburcu for atomic ops, but liburcu does
not support atomic64_t on those archs. It indicates this during runtime
by generating an illegal instruction that aborts execution, and thus
causes various xfsprogs utils to be segfaulting.
Fix this by requiring that unsigned longs are at least 64 bits in size,
which /usually/ means that 64-bit atomic counters are supported. We
can't simply execute the liburcu atomic64_t detection code during
configure instead of only relying on the linker error because that
doesn't work for cross-compiled packages.
Fixes: 7448af588a2e ("libxfs: fix atomic64_t poorly for 32-bit architectures")
Reported-by: Anthony Iliopoulos <ailiop@suse.com>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v1.1: This time with correct commit message.
---
m4/package_urcu.m4 | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/m4/package_urcu.m4 b/m4/package_urcu.m4
index ef116e0cda7..4bb2b886f06 100644
--- a/m4/package_urcu.m4
+++ b/m4/package_urcu.m4
@@ -26,7 +26,11 @@ rcu_init();
#
# Make sure that calling uatomic_inc on a 64-bit integer doesn't cause a link
# error on _uatomic_link_error, which is how liburcu signals that it doesn't
-# support atomic operations on 64-bit data types.
+# support atomic operations on 64-bit data types for its generic
+# implementation (which relies on compiler builtins). For certain archs
+# where liburcu carries its own implementation (such as x86_32), it
+# signals lack of support during runtime by emitting an illegal
+# instruction, so we also need to check CAA_BITS_PER_LONG to detect that.
#
AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
[ AC_MSG_CHECKING([for atomic64_t support in liburcu])
@@ -34,8 +38,11 @@ AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
[ AC_LANG_PROGRAM([[
#define _GNU_SOURCE
#include <urcu.h>
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
]], [[
long long f = 3;
+
+BUILD_BUG_ON(CAA_BITS_PER_LONG < 64);
uatomic_inc(&f);
]])
], have_liburcu_atomic64=yes
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v1.1 6/6] libxfs: fix atomic64_t detection on x86 32-bit architectures
2023-09-12 19:47 ` [PATCH v1.1 " Darrick J. Wong
@ 2023-09-13 13:22 ` Carlos Maiolino
2023-09-14 18:26 ` Bill O'Donnell
2023-10-05 12:45 ` Carlos Maiolino
2 siblings, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-09-13 13:22 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Anthony Iliopoulos, linux-xfs
On Tue, Sep 12, 2023 at 12:47:51PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> xfsprogs during compilation tries to detect if liburcu supports atomic
> 64-bit ops on the platform it is being compiled on, and if not it falls
> back to using pthread mutex locks.
>
> The detection logic for that fallback relies on _uatomic_link_error()
> which is a link-time trick used by liburcu that will cause compilation
> errors on archs that lack the required support. That only works for the
> generic liburcu code though, and it is not implemented for the
> x86-specific code.
>
> In practice this means that when xfsprogs is compiled on 32-bit x86
> archs will successfully link to liburcu for atomic ops, but liburcu does
> not support atomic64_t on those archs. It indicates this during runtime
> by generating an illegal instruction that aborts execution, and thus
> causes various xfsprogs utils to be segfaulting.
>
> Fix this by requiring that unsigned longs are at least 64 bits in size,
> which /usually/ means that 64-bit atomic counters are supported. We
> can't simply execute the liburcu atomic64_t detection code during
> configure instead of only relying on the linker error because that
> doesn't work for cross-compiled packages.
>
> Fixes: 7448af588a2e ("libxfs: fix atomic64_t poorly for 32-bit architectures")
> Reported-by: Anthony Iliopoulos <ailiop@suse.com>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> v1.1: This time with correct commit message.
> ---
> m4/package_urcu.m4 | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/m4/package_urcu.m4 b/m4/package_urcu.m4
> index ef116e0cda7..4bb2b886f06 100644
> --- a/m4/package_urcu.m4
> +++ b/m4/package_urcu.m4
> @@ -26,7 +26,11 @@ rcu_init();
> #
> # Make sure that calling uatomic_inc on a 64-bit integer doesn't cause a link
> # error on _uatomic_link_error, which is how liburcu signals that it doesn't
> -# support atomic operations on 64-bit data types.
> +# support atomic operations on 64-bit data types for its generic
> +# implementation (which relies on compiler builtins). For certain archs
> +# where liburcu carries its own implementation (such as x86_32), it
> +# signals lack of support during runtime by emitting an illegal
> +# instruction, so we also need to check CAA_BITS_PER_LONG to detect that.
> #
> AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
> [ AC_MSG_CHECKING([for atomic64_t support in liburcu])
> @@ -34,8 +38,11 @@ AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
> [ AC_LANG_PROGRAM([[
> #define _GNU_SOURCE
> #include <urcu.h>
> +#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> ]], [[
> long long f = 3;
> +
> +BUILD_BUG_ON(CAA_BITS_PER_LONG < 64);
> uatomic_inc(&f);
> ]])
> ], have_liburcu_atomic64=yes
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v1.1 6/6] libxfs: fix atomic64_t detection on x86 32-bit architectures
2023-09-12 19:47 ` [PATCH v1.1 " Darrick J. Wong
2023-09-13 13:22 ` Carlos Maiolino
@ 2023-09-14 18:26 ` Bill O'Donnell
2023-10-05 12:45 ` Carlos Maiolino
2 siblings, 0 replies; 23+ messages in thread
From: Bill O'Donnell @ 2023-09-14 18:26 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, Anthony Iliopoulos, linux-xfs
On Tue, Sep 12, 2023 at 12:47:51PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> xfsprogs during compilation tries to detect if liburcu supports atomic
> 64-bit ops on the platform it is being compiled on, and if not it falls
> back to using pthread mutex locks.
>
> The detection logic for that fallback relies on _uatomic_link_error()
> which is a link-time trick used by liburcu that will cause compilation
> errors on archs that lack the required support. That only works for the
> generic liburcu code though, and it is not implemented for the
> x86-specific code.
>
> In practice this means that when xfsprogs is compiled on 32-bit x86
> archs will successfully link to liburcu for atomic ops, but liburcu does
> not support atomic64_t on those archs. It indicates this during runtime
> by generating an illegal instruction that aborts execution, and thus
> causes various xfsprogs utils to be segfaulting.
>
> Fix this by requiring that unsigned longs are at least 64 bits in size,
> which /usually/ means that 64-bit atomic counters are supported. We
> can't simply execute the liburcu atomic64_t detection code during
> configure instead of only relying on the linker error because that
> doesn't work for cross-compiled packages.
>
> Fixes: 7448af588a2e ("libxfs: fix atomic64_t poorly for 32-bit architectures")
> Reported-by: Anthony Iliopoulos <ailiop@suse.com>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>
> ---
> v1.1: This time with correct commit message.
> ---
> m4/package_urcu.m4 | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/m4/package_urcu.m4 b/m4/package_urcu.m4
> index ef116e0cda7..4bb2b886f06 100644
> --- a/m4/package_urcu.m4
> +++ b/m4/package_urcu.m4
> @@ -26,7 +26,11 @@ rcu_init();
> #
> # Make sure that calling uatomic_inc on a 64-bit integer doesn't cause a link
> # error on _uatomic_link_error, which is how liburcu signals that it doesn't
> -# support atomic operations on 64-bit data types.
> +# support atomic operations on 64-bit data types for its generic
> +# implementation (which relies on compiler builtins). For certain archs
> +# where liburcu carries its own implementation (such as x86_32), it
> +# signals lack of support during runtime by emitting an illegal
> +# instruction, so we also need to check CAA_BITS_PER_LONG to detect that.
> #
> AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
> [ AC_MSG_CHECKING([for atomic64_t support in liburcu])
> @@ -34,8 +38,11 @@ AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
> [ AC_LANG_PROGRAM([[
> #define _GNU_SOURCE
> #include <urcu.h>
> +#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> ]], [[
> long long f = 3;
> +
> +BUILD_BUG_ON(CAA_BITS_PER_LONG < 64);
> uatomic_inc(&f);
> ]])
> ], have_liburcu_atomic64=yes
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v1.1 6/6] libxfs: fix atomic64_t detection on x86 32-bit architectures
2023-09-12 19:47 ` [PATCH v1.1 " Darrick J. Wong
2023-09-13 13:22 ` Carlos Maiolino
2023-09-14 18:26 ` Bill O'Donnell
@ 2023-10-05 12:45 ` Carlos Maiolino
2 siblings, 0 replies; 23+ messages in thread
From: Carlos Maiolino @ 2023-10-05 12:45 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Anthony Iliopoulos, linux-xfs
On Tue, Sep 12, 2023 at 12:47:51PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> xfsprogs during compilation tries to detect if liburcu supports atomic
> 64-bit ops on the platform it is being compiled on, and if not it falls
> back to using pthread mutex locks.
>
> The detection logic for that fallback relies on _uatomic_link_error()
> which is a link-time trick used by liburcu that will cause compilation
> errors on archs that lack the required support. That only works for the
> generic liburcu code though, and it is not implemented for the
> x86-specific code.
>
> In practice this means that when xfsprogs is compiled on 32-bit x86
> archs will successfully link to liburcu for atomic ops, but liburcu does
> not support atomic64_t on those archs. It indicates this during runtime
> by generating an illegal instruction that aborts execution, and thus
> causes various xfsprogs utils to be segfaulting.
>
> Fix this by requiring that unsigned longs are at least 64 bits in size,
> which /usually/ means that 64-bit atomic counters are supported. We
> can't simply execute the liburcu atomic64_t detection code during
> configure instead of only relying on the linker error because that
> doesn't work for cross-compiled packages.
>
> Fixes: 7448af588a2e ("libxfs: fix atomic64_t poorly for 32-bit architectures")
> Reported-by: Anthony Iliopoulos <ailiop@suse.com>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v1.1: This time with correct commit message.
> ---
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Carlos
> m4/package_urcu.m4 | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/m4/package_urcu.m4 b/m4/package_urcu.m4
> index ef116e0cda7..4bb2b886f06 100644
> --- a/m4/package_urcu.m4
> +++ b/m4/package_urcu.m4
> @@ -26,7 +26,11 @@ rcu_init();
> #
> # Make sure that calling uatomic_inc on a 64-bit integer doesn't cause a link
> # error on _uatomic_link_error, which is how liburcu signals that it doesn't
> -# support atomic operations on 64-bit data types.
> +# support atomic operations on 64-bit data types for its generic
> +# implementation (which relies on compiler builtins). For certain archs
> +# where liburcu carries its own implementation (such as x86_32), it
> +# signals lack of support during runtime by emitting an illegal
> +# instruction, so we also need to check CAA_BITS_PER_LONG to detect that.
> #
> AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
> [ AC_MSG_CHECKING([for atomic64_t support in liburcu])
> @@ -34,8 +38,11 @@ AC_DEFUN([AC_HAVE_LIBURCU_ATOMIC64],
> [ AC_LANG_PROGRAM([[
> #define _GNU_SOURCE
> #include <urcu.h>
> +#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> ]], [[
> long long f = 3;
> +
> +BUILD_BUG_ON(CAA_BITS_PER_LONG < 64);
> uatomic_inc(&f);
> ]])
> ], have_liburcu_atomic64=yes
^ permalink raw reply [flat|nested] 23+ messages in thread