* [PATCH 1/9] libfrog: move 64-bit division wrappers to libfrog
2023-11-22 23:06 [PATCHSET 0/9] xfsprogs: minor fixes for 6.6 Darrick J. Wong
@ 2023-11-22 23:06 ` Darrick J. Wong
2023-11-23 6:34 ` Christoph Hellwig
2023-11-24 9:09 ` Chandan Babu R
2023-11-22 23:06 ` [PATCH 2/9] libxfs: don't UAF a requeued EFI Darrick J. Wong
` (7 subsequent siblings)
8 siblings, 2 replies; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-22 23:06 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
We want to keep the rtgroup unit conversion functions as static inlines,
so share the div64 functions via libfrog instead of libxfs_priv.h.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
include/libxfs.h | 1 +
libfrog/Makefile | 1 +
libfrog/div64.h | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
libxfs/libxfs_priv.h | 77 +---------------------------------------
4 files changed, 99 insertions(+), 76 deletions(-)
create mode 100644 libfrog/div64.h
diff --git a/include/libxfs.h b/include/libxfs.h
index b28781d19d3..a6a5f66f28d 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -18,6 +18,7 @@
#include "kmem.h"
#include "libfrog/radix-tree.h"
#include "libfrog/bitmask.h"
+#include "libfrog/div64.h"
#include "atomic.h"
#include "spinlock.h"
diff --git a/libfrog/Makefile b/libfrog/Makefile
index 8cde97d418f..dcfd1fb8a93 100644
--- a/libfrog/Makefile
+++ b/libfrog/Makefile
@@ -41,6 +41,7 @@ crc32cselftest.h \
crc32defs.h \
crc32table.h \
dahashselftest.h \
+div64.h \
fsgeom.h \
logging.h \
paths.h \
diff --git a/libfrog/div64.h b/libfrog/div64.h
new file mode 100644
index 00000000000..673b01cbab3
--- /dev/null
+++ b/libfrog/div64.h
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2000-2005 Silicon Graphics, Inc.
+ * All Rights Reserved.
+ */
+#ifndef LIBFROG_DIV64_H_
+#define LIBFROG_DIV64_H_
+
+static inline int __do_div(unsigned long long *n, unsigned base)
+{
+ int __res;
+ __res = (int)(((unsigned long) *n) % (unsigned) base);
+ *n = ((unsigned long) *n) / (unsigned) base;
+ return __res;
+}
+
+#define do_div(n,base) (__do_div((unsigned long long *)&(n), (base)))
+#define do_mod(a, b) ((a) % (b))
+#define rol32(x,y) (((x) << (y)) | ((x) >> (32 - (y))))
+
+/**
+ * div_u64_rem - unsigned 64bit divide with 32bit divisor with remainder
+ * @dividend: unsigned 64bit dividend
+ * @divisor: unsigned 32bit divisor
+ * @remainder: pointer to unsigned 32bit remainder
+ *
+ * Return: sets ``*remainder``, then returns dividend / divisor
+ *
+ * This is commonly provided by 32bit archs to provide an optimized 64bit
+ * divide.
+ */
+static inline uint64_t
+div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder)
+{
+ *remainder = dividend % divisor;
+ return dividend / divisor;
+}
+
+/**
+ * div_u64 - unsigned 64bit divide with 32bit divisor
+ * @dividend: unsigned 64bit dividend
+ * @divisor: unsigned 32bit divisor
+ *
+ * This is the most common 64bit divide and should be used if possible,
+ * as many 32bit archs can optimize this variant better than a full 64bit
+ * divide.
+ */
+static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor)
+{
+ uint32_t remainder;
+ return div_u64_rem(dividend, divisor, &remainder);
+}
+
+/**
+ * div64_u64_rem - unsigned 64bit divide with 64bit divisor and remainder
+ * @dividend: unsigned 64bit dividend
+ * @divisor: unsigned 64bit divisor
+ * @remainder: pointer to unsigned 64bit remainder
+ *
+ * Return: sets ``*remainder``, then returns dividend / divisor
+ */
+static inline uint64_t
+div64_u64_rem(uint64_t dividend, uint64_t divisor, uint64_t *remainder)
+{
+ *remainder = dividend % divisor;
+ return dividend / divisor;
+}
+
+static inline uint64_t rounddown_64(uint64_t x, uint32_t y)
+{
+ do_div(x, y);
+ return x * y;
+}
+
+static inline bool isaligned_64(uint64_t x, uint32_t y)
+{
+ return do_div(x, y) == 0;
+}
+
+static inline uint64_t
+roundup_64(uint64_t x, uint32_t y)
+{
+ x += y - 1;
+ do_div(x, y);
+ return x * y;
+}
+
+static inline uint64_t
+howmany_64(uint64_t x, uint32_t y)
+{
+ x += y - 1;
+ do_div(x, y);
+ return x;
+}
+
+#endif /* LIBFROG_DIV64_H_ */
diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 2729241bdaa..5a7decf970e 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -48,6 +48,7 @@
#include "kmem.h"
#include "libfrog/radix-tree.h"
#include "libfrog/bitmask.h"
+#include "libfrog/div64.h"
#include "atomic.h"
#include "spinlock.h"
#include "linux-err.h"
@@ -215,66 +216,6 @@ static inline bool WARN_ON(bool expr) {
(inode)->i_version = (version); \
} while (0)
-static inline int __do_div(unsigned long long *n, unsigned base)
-{
- int __res;
- __res = (int)(((unsigned long) *n) % (unsigned) base);
- *n = ((unsigned long) *n) / (unsigned) base;
- return __res;
-}
-
-#define do_div(n,base) (__do_div((unsigned long long *)&(n), (base)))
-#define do_mod(a, b) ((a) % (b))
-#define rol32(x,y) (((x) << (y)) | ((x) >> (32 - (y))))
-
-/**
- * div_u64_rem - unsigned 64bit divide with 32bit divisor with remainder
- * @dividend: unsigned 64bit dividend
- * @divisor: unsigned 32bit divisor
- * @remainder: pointer to unsigned 32bit remainder
- *
- * Return: sets ``*remainder``, then returns dividend / divisor
- *
- * This is commonly provided by 32bit archs to provide an optimized 64bit
- * divide.
- */
-static inline uint64_t
-div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder)
-{
- *remainder = dividend % divisor;
- return dividend / divisor;
-}
-
-/**
- * div_u64 - unsigned 64bit divide with 32bit divisor
- * @dividend: unsigned 64bit dividend
- * @divisor: unsigned 32bit divisor
- *
- * This is the most common 64bit divide and should be used if possible,
- * as many 32bit archs can optimize this variant better than a full 64bit
- * divide.
- */
-static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor)
-{
- uint32_t remainder;
- return div_u64_rem(dividend, divisor, &remainder);
-}
-
-/**
- * div64_u64_rem - unsigned 64bit divide with 64bit divisor and remainder
- * @dividend: unsigned 64bit dividend
- * @divisor: unsigned 64bit divisor
- * @remainder: pointer to unsigned 64bit remainder
- *
- * Return: sets ``*remainder``, then returns dividend / divisor
- */
-static inline uint64_t
-div64_u64_rem(uint64_t dividend, uint64_t divisor, uint64_t *remainder)
-{
- *remainder = dividend % divisor;
- return dividend / divisor;
-}
-
#define min_t(type,x,y) \
({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
#define max_t(type,x,y) \
@@ -380,22 +321,6 @@ roundup_pow_of_two(uint v)
return 0;
}
-static inline uint64_t
-roundup_64(uint64_t x, uint32_t y)
-{
- x += y - 1;
- do_div(x, y);
- return x * y;
-}
-
-static inline uint64_t
-howmany_64(uint64_t x, uint32_t y)
-{
- x += y - 1;
- do_div(x, y);
- return x;
-}
-
/* buffer management */
#define XBF_TRYLOCK 0
#define XBF_UNMAPPED 0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 1/9] libfrog: move 64-bit division wrappers to libfrog
2023-11-22 23:06 ` [PATCH 1/9] libfrog: move 64-bit division wrappers to libfrog Darrick J. Wong
@ 2023-11-23 6:34 ` Christoph Hellwig
2023-11-24 9:09 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:34 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
.. I so wish we get rid of the stupid 64-bit division helpers in the
kernel..
But until then:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/9] libfrog: move 64-bit division wrappers to libfrog
2023-11-22 23:06 ` [PATCH 1/9] libfrog: move 64-bit division wrappers to libfrog Darrick J. Wong
2023-11-23 6:34 ` Christoph Hellwig
@ 2023-11-24 9:09 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Chandan Babu R @ 2023-11-24 9:09 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:06:54 PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> We want to keep the rtgroup unit conversion functions as static inlines,
> so share the div64 functions via libfrog instead of libxfs_priv.h.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks good to me.
Reviewed-by: Chandan Babu R <chandanbabu@kernel.org>
> ---
> include/libxfs.h | 1 +
> libfrog/Makefile | 1 +
> libfrog/div64.h | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
> libxfs/libxfs_priv.h | 77 +---------------------------------------
> 4 files changed, 99 insertions(+), 76 deletions(-)
> create mode 100644 libfrog/div64.h
>
>
> diff --git a/include/libxfs.h b/include/libxfs.h
> index b28781d19d3..a6a5f66f28d 100644
> --- a/include/libxfs.h
> +++ b/include/libxfs.h
> @@ -18,6 +18,7 @@
> #include "kmem.h"
> #include "libfrog/radix-tree.h"
> #include "libfrog/bitmask.h"
> +#include "libfrog/div64.h"
> #include "atomic.h"
> #include "spinlock.h"
>
> diff --git a/libfrog/Makefile b/libfrog/Makefile
> index 8cde97d418f..dcfd1fb8a93 100644
> --- a/libfrog/Makefile
> +++ b/libfrog/Makefile
> @@ -41,6 +41,7 @@ crc32cselftest.h \
> crc32defs.h \
> crc32table.h \
> dahashselftest.h \
> +div64.h \
> fsgeom.h \
> logging.h \
> paths.h \
> diff --git a/libfrog/div64.h b/libfrog/div64.h
> new file mode 100644
> index 00000000000..673b01cbab3
> --- /dev/null
> +++ b/libfrog/div64.h
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2000-2005 Silicon Graphics, Inc.
> + * All Rights Reserved.
> + */
> +#ifndef LIBFROG_DIV64_H_
> +#define LIBFROG_DIV64_H_
> +
> +static inline int __do_div(unsigned long long *n, unsigned base)
> +{
> + int __res;
> + __res = (int)(((unsigned long) *n) % (unsigned) base);
> + *n = ((unsigned long) *n) / (unsigned) base;
> + return __res;
> +}
> +
> +#define do_div(n,base) (__do_div((unsigned long long *)&(n), (base)))
> +#define do_mod(a, b) ((a) % (b))
> +#define rol32(x,y) (((x) << (y)) | ((x) >> (32 - (y))))
> +
> +/**
> + * div_u64_rem - unsigned 64bit divide with 32bit divisor with remainder
> + * @dividend: unsigned 64bit dividend
> + * @divisor: unsigned 32bit divisor
> + * @remainder: pointer to unsigned 32bit remainder
> + *
> + * Return: sets ``*remainder``, then returns dividend / divisor
> + *
> + * This is commonly provided by 32bit archs to provide an optimized 64bit
> + * divide.
> + */
> +static inline uint64_t
> +div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder)
> +{
> + *remainder = dividend % divisor;
> + return dividend / divisor;
> +}
> +
> +/**
> + * div_u64 - unsigned 64bit divide with 32bit divisor
> + * @dividend: unsigned 64bit dividend
> + * @divisor: unsigned 32bit divisor
> + *
> + * This is the most common 64bit divide and should be used if possible,
> + * as many 32bit archs can optimize this variant better than a full 64bit
> + * divide.
> + */
> +static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor)
> +{
> + uint32_t remainder;
> + return div_u64_rem(dividend, divisor, &remainder);
> +}
> +
> +/**
> + * div64_u64_rem - unsigned 64bit divide with 64bit divisor and remainder
> + * @dividend: unsigned 64bit dividend
> + * @divisor: unsigned 64bit divisor
> + * @remainder: pointer to unsigned 64bit remainder
> + *
> + * Return: sets ``*remainder``, then returns dividend / divisor
> + */
> +static inline uint64_t
> +div64_u64_rem(uint64_t dividend, uint64_t divisor, uint64_t *remainder)
> +{
> + *remainder = dividend % divisor;
> + return dividend / divisor;
> +}
> +
> +static inline uint64_t rounddown_64(uint64_t x, uint32_t y)
> +{
> + do_div(x, y);
> + return x * y;
> +}
> +
> +static inline bool isaligned_64(uint64_t x, uint32_t y)
> +{
> + return do_div(x, y) == 0;
> +}
> +
> +static inline uint64_t
> +roundup_64(uint64_t x, uint32_t y)
> +{
> + x += y - 1;
> + do_div(x, y);
> + return x * y;
> +}
> +
> +static inline uint64_t
> +howmany_64(uint64_t x, uint32_t y)
> +{
> + x += y - 1;
> + do_div(x, y);
> + return x;
> +}
> +
> +#endif /* LIBFROG_DIV64_H_ */
> diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
> index 2729241bdaa..5a7decf970e 100644
> --- a/libxfs/libxfs_priv.h
> +++ b/libxfs/libxfs_priv.h
> @@ -48,6 +48,7 @@
> #include "kmem.h"
> #include "libfrog/radix-tree.h"
> #include "libfrog/bitmask.h"
> +#include "libfrog/div64.h"
> #include "atomic.h"
> #include "spinlock.h"
> #include "linux-err.h"
> @@ -215,66 +216,6 @@ static inline bool WARN_ON(bool expr) {
> (inode)->i_version = (version); \
> } while (0)
>
> -static inline int __do_div(unsigned long long *n, unsigned base)
> -{
> - int __res;
> - __res = (int)(((unsigned long) *n) % (unsigned) base);
> - *n = ((unsigned long) *n) / (unsigned) base;
> - return __res;
> -}
> -
> -#define do_div(n,base) (__do_div((unsigned long long *)&(n), (base)))
> -#define do_mod(a, b) ((a) % (b))
> -#define rol32(x,y) (((x) << (y)) | ((x) >> (32 - (y))))
> -
> -/**
> - * div_u64_rem - unsigned 64bit divide with 32bit divisor with remainder
> - * @dividend: unsigned 64bit dividend
> - * @divisor: unsigned 32bit divisor
> - * @remainder: pointer to unsigned 32bit remainder
> - *
> - * Return: sets ``*remainder``, then returns dividend / divisor
> - *
> - * This is commonly provided by 32bit archs to provide an optimized 64bit
> - * divide.
> - */
> -static inline uint64_t
> -div_u64_rem(uint64_t dividend, uint32_t divisor, uint32_t *remainder)
> -{
> - *remainder = dividend % divisor;
> - return dividend / divisor;
> -}
> -
> -/**
> - * div_u64 - unsigned 64bit divide with 32bit divisor
> - * @dividend: unsigned 64bit dividend
> - * @divisor: unsigned 32bit divisor
> - *
> - * This is the most common 64bit divide and should be used if possible,
> - * as many 32bit archs can optimize this variant better than a full 64bit
> - * divide.
> - */
> -static inline uint64_t div_u64(uint64_t dividend, uint32_t divisor)
> -{
> - uint32_t remainder;
> - return div_u64_rem(dividend, divisor, &remainder);
> -}
> -
> -/**
> - * div64_u64_rem - unsigned 64bit divide with 64bit divisor and remainder
> - * @dividend: unsigned 64bit dividend
> - * @divisor: unsigned 64bit divisor
> - * @remainder: pointer to unsigned 64bit remainder
> - *
> - * Return: sets ``*remainder``, then returns dividend / divisor
> - */
> -static inline uint64_t
> -div64_u64_rem(uint64_t dividend, uint64_t divisor, uint64_t *remainder)
> -{
> - *remainder = dividend % divisor;
> - return dividend / divisor;
> -}
> -
> #define min_t(type,x,y) \
> ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
> #define max_t(type,x,y) \
> @@ -380,22 +321,6 @@ roundup_pow_of_two(uint v)
> return 0;
> }
>
> -static inline uint64_t
> -roundup_64(uint64_t x, uint32_t y)
> -{
> - x += y - 1;
> - do_div(x, y);
> - return x * y;
> -}
> -
> -static inline uint64_t
> -howmany_64(uint64_t x, uint32_t y)
> -{
> - x += y - 1;
> - do_div(x, y);
> - return x;
> -}
> -
> /* buffer management */
> #define XBF_TRYLOCK 0
> #define XBF_UNMAPPED 0
--
Chandan
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/9] libxfs: don't UAF a requeued EFI
2023-11-22 23:06 [PATCHSET 0/9] xfsprogs: minor fixes for 6.6 Darrick J. Wong
2023-11-22 23:06 ` [PATCH 1/9] libfrog: move 64-bit division wrappers to libfrog Darrick J. Wong
@ 2023-11-22 23:06 ` Darrick J. Wong
2023-11-23 6:36 ` Christoph Hellwig
2023-11-24 9:10 ` Chandan Babu R
2023-11-22 23:07 ` [PATCH 3/9] xfs_copy: actually do directio writes to block devices Darrick J. Wong
` (6 subsequent siblings)
8 siblings, 2 replies; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-22 23:06 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
In the kernel, commit 8ebbf262d4684 ("xfs: don't block in busy flushing
when freeing extents") changed the allocator behavior such that AGFL
fixing can return -EAGAIN in response to detection of a deadlock with
the transaction busy extent list. If this happens, we're supposed to
requeue the EFI so that we can roll the transaction and try the item
again.
If a requeue happens, we should not free the xefi pointer in
xfs_extent_free_finish_item or else the retry will walk off a dangling
pointer. There is no extent busy list in userspace so this should
never happen, but let's fix the logic bomb anyway.
We should have ported kernel commit 0853b5de42b47 ("xfs: allow extent
free intents to be retried") to userspace, but neither Carlos nor I
noticed this fine detail. :(
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
libxfs/defer_item.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/libxfs/defer_item.c b/libxfs/defer_item.c
index 3f519252046..8731d1834be 100644
--- a/libxfs/defer_item.c
+++ b/libxfs/defer_item.c
@@ -115,6 +115,13 @@ xfs_extent_free_finish_item(
error = xfs_free_extent(tp, xefi->xefi_pag, agbno,
xefi->xefi_blockcount, &oinfo, XFS_AG_RESV_NONE);
+ /*
+ * Don't free the XEFI if we need a new transaction to complete
+ * processing of it.
+ */
+ if (error == -EAGAIN)
+ return error;
+
xfs_extent_free_put_group(xefi);
kmem_cache_free(xfs_extfree_item_cache, xefi);
return error;
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 2/9] libxfs: don't UAF a requeued EFI
2023-11-22 23:06 ` [PATCH 2/9] libxfs: don't UAF a requeued EFI Darrick J. Wong
@ 2023-11-23 6:36 ` Christoph Hellwig
2023-11-27 18:10 ` Darrick J. Wong
2023-11-24 9:10 ` Chandan Babu R
1 sibling, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:36 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:06:59PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> In the kernel, commit 8ebbf262d4684 ("xfs: don't block in busy flushing
> when freeing extents") changed the allocator behavior such that AGFL
> fixing can return -EAGAIN in response to detection of a deadlock with
> the transaction busy extent list. If this happens, we're supposed to
> requeue the EFI so that we can roll the transaction and try the item
> again.
>
> If a requeue happens, we should not free the xefi pointer in
> xfs_extent_free_finish_item or else the retry will walk off a dangling
> pointer. There is no extent busy list in userspace so this should
> never happen, but let's fix the logic bomb anyway.
>
> We should have ported kernel commit 0853b5de42b47 ("xfs: allow extent
> free intents to be retried") to userspace, but neither Carlos nor I
> noticed this fine detail. :(
It might be time to move this code into shared files?
Btw, I still keep getting annoyed a bit about minor difference
in some of the libxfs file due to header inclusion. Maybe we also
need to be able to automate this more and require libxfs to only
include libxfs headers and xfs.h?
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 2/9] libxfs: don't UAF a requeued EFI
2023-11-23 6:36 ` Christoph Hellwig
@ 2023-11-27 18:10 ` Darrick J. Wong
2023-11-28 5:37 ` Christoph Hellwig
0 siblings, 1 reply; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-27 18:10 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 10:36:57PM -0800, Christoph Hellwig wrote:
> On Wed, Nov 22, 2023 at 03:06:59PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > In the kernel, commit 8ebbf262d4684 ("xfs: don't block in busy flushing
> > when freeing extents") changed the allocator behavior such that AGFL
> > fixing can return -EAGAIN in response to detection of a deadlock with
> > the transaction busy extent list. If this happens, we're supposed to
> > requeue the EFI so that we can roll the transaction and try the item
> > again.
> >
> > If a requeue happens, we should not free the xefi pointer in
> > xfs_extent_free_finish_item or else the retry will walk off a dangling
> > pointer. There is no extent busy list in userspace so this should
> > never happen, but let's fix the logic bomb anyway.
> >
> > We should have ported kernel commit 0853b5de42b47 ("xfs: allow extent
> > free intents to be retried") to userspace, but neither Carlos nor I
> > noticed this fine detail. :(
>
> It might be time to move this code into shared files?
I think Chandan started looking into what it would take to port the log
code from the kernel into userspace. Then xfs_trans_commit in userspace
could actually write transactions to the log and write them back
atomically; and xfs_repair could finally lose the -L switch.
> Btw, I still keep getting annoyed a bit about minor difference
> in some of the libxfs file due to header inclusion. Maybe we also
> need to be able to automate this more and require libxfs to only
> include libxfs headers and xfs.h?
<shrug> Ages ago Dave had a sketch to make xfs_{log,mount,inode}.h pull
in the relevant headers so that all the other source files only had to
#include at most three files.
I wish the kernel had precompiled headers, then it would make sense to
have one xfs.h file that pulled in the world and took advantage of
caching.
--D
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 2/9] libxfs: don't UAF a requeued EFI
2023-11-27 18:10 ` Darrick J. Wong
@ 2023-11-28 5:37 ` Christoph Hellwig
2023-11-28 17:01 ` Darrick J. Wong
0 siblings, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-28 5:37 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, cem, linux-xfs
On Mon, Nov 27, 2023 at 10:10:24AM -0800, Darrick J. Wong wrote:
> > It might be time to move this code into shared files?
>
> I think Chandan started looking into what it would take to port the log
> code from the kernel into userspace. Then xfs_trans_commit in userspace
> could actually write transactions to the log and write them back
> atomically; and xfs_repair could finally lose the -L switch.
While that does sound like a really good idea, it's now what I meant
here. I think if we moved the actual defer ops instances out of the
_item.c files into libxfs, I think we could reuse them for the current
way of operating in userspace quite easily with strategic stubs for
some functionality.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/9] libxfs: don't UAF a requeued EFI
2023-11-28 5:37 ` Christoph Hellwig
@ 2023-11-28 17:01 ` Darrick J. Wong
2023-11-28 17:09 ` Christoph Hellwig
0 siblings, 1 reply; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-28 17:01 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: cem, linux-xfs
On Mon, Nov 27, 2023 at 09:37:15PM -0800, Christoph Hellwig wrote:
> On Mon, Nov 27, 2023 at 10:10:24AM -0800, Darrick J. Wong wrote:
> > > It might be time to move this code into shared files?
> >
> > I think Chandan started looking into what it would take to port the log
> > code from the kernel into userspace. Then xfs_trans_commit in userspace
> > could actually write transactions to the log and write them back
> > atomically; and xfs_repair could finally lose the -L switch.
>
> While that does sound like a really good idea, it's now what I meant
> here. I think if we moved the actual defer ops instances out of the
> _item.c files into libxfs, I think we could reuse them for the current
> way of operating in userspace quite easily with strategic stubs for
> some functionality.
Oh! You're talking about moving xfs_rmap_update_defer_type and the
functions it points to into xfs_rmap.c, then? Hmm. I just moved
->iop_recover into xfs_defer_op_type, let me send an RFC for that.
(You and I might have hit critical mass for log item cleanups... ;))
--D
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/9] libxfs: don't UAF a requeued EFI
2023-11-28 17:01 ` Darrick J. Wong
@ 2023-11-28 17:09 ` Christoph Hellwig
0 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-28 17:09 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, cem, linux-xfs
On Tue, Nov 28, 2023 at 09:01:21AM -0800, Darrick J. Wong wrote:
> Oh! You're talking about moving xfs_rmap_update_defer_type and the
> functions it points to into xfs_rmap.c, then?
Yes.
> Hmm. I just moved
> ->iop_recover into xfs_defer_op_type, let me send an RFC for that.
>
> (You and I might have hit critical mass for log item cleanups... ;))
Yeah..
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/9] libxfs: don't UAF a requeued EFI
2023-11-22 23:06 ` [PATCH 2/9] libxfs: don't UAF a requeued EFI Darrick J. Wong
2023-11-23 6:36 ` Christoph Hellwig
@ 2023-11-24 9:10 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Chandan Babu R @ 2023-11-24 9:10 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:06:59 PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> In the kernel, commit 8ebbf262d4684 ("xfs: don't block in busy flushing
> when freeing extents") changed the allocator behavior such that AGFL
> fixing can return -EAGAIN in response to detection of a deadlock with
> the transaction busy extent list. If this happens, we're supposed to
> requeue the EFI so that we can roll the transaction and try the item
> again.
>
> If a requeue happens, we should not free the xefi pointer in
> xfs_extent_free_finish_item or else the retry will walk off a dangling
> pointer. There is no extent busy list in userspace so this should
> never happen, but let's fix the logic bomb anyway.
>
> We should have ported kernel commit 0853b5de42b47 ("xfs: allow extent
> free intents to be retried") to userspace, but neither Carlos nor I
> noticed this fine detail. :(
>
Looks good to me.
Reviewed-by: Chandan Babu R <chandanbabu@kernel.org>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> libxfs/defer_item.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
>
> diff --git a/libxfs/defer_item.c b/libxfs/defer_item.c
> index 3f519252046..8731d1834be 100644
> --- a/libxfs/defer_item.c
> +++ b/libxfs/defer_item.c
> @@ -115,6 +115,13 @@ xfs_extent_free_finish_item(
> error = xfs_free_extent(tp, xefi->xefi_pag, agbno,
> xefi->xefi_blockcount, &oinfo, XFS_AG_RESV_NONE);
>
> + /*
> + * Don't free the XEFI if we need a new transaction to complete
> + * processing of it.
> + */
> + if (error == -EAGAIN)
> + return error;
> +
> xfs_extent_free_put_group(xefi);
> kmem_cache_free(xfs_extfree_item_cache, xefi);
> return error;
--
Chandan
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 3/9] xfs_copy: actually do directio writes to block devices
2023-11-22 23:06 [PATCHSET 0/9] xfsprogs: minor fixes for 6.6 Darrick J. Wong
2023-11-22 23:06 ` [PATCH 1/9] libfrog: move 64-bit division wrappers to libfrog Darrick J. Wong
2023-11-22 23:06 ` [PATCH 2/9] libxfs: don't UAF a requeued EFI Darrick J. Wong
@ 2023-11-22 23:07 ` Darrick J. Wong
2023-11-23 6:40 ` Christoph Hellwig
2023-11-22 23:07 ` [PATCH 4/9] xfs_db: report the device associated with each io cursor Darrick J. Wong
` (5 subsequent siblings)
8 siblings, 1 reply; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-22 23:07 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Not sure why block device targets don't get O_DIRECT in !buffered mode,
but it's misleading when the copy completes instantly only to stall
forever due to fsync-on-close. Adjust the "write last sector" code to
allocate a properly aligned buffer.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
copy/xfs_copy.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index 79f65946709..26de6b2ee1c 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -854,6 +854,8 @@ main(int argc, char **argv)
progname, target[i].name, progname);
exit(1);
}
+ if (!buffered_output)
+ open_flags |= O_DIRECT;
}
target[i].fd = open(target[i].name, open_flags, 0644);
@@ -887,20 +889,22 @@ main(int argc, char **argv)
}
}
} else {
- char *lb[XFS_MAX_SECTORSIZE] = { NULL };
+ char *lb = memalign(wbuf_align, XFS_MAX_SECTORSIZE);
off64_t off;
/* ensure device files are sufficiently large */
+ memset(lb, 0, XFS_MAX_SECTORSIZE);
off = mp->m_sb.sb_dblocks * source_blocksize;
- off -= sizeof(lb);
- if (pwrite(target[i].fd, lb, sizeof(lb), off) < 0) {
+ off -= XFS_MAX_SECTORSIZE;
+ if (pwrite(target[i].fd, lb, XFS_MAX_SECTORSIZE, off) < 0) {
do_log(_("%s: failed to write last block\n"),
progname);
do_log(_("\tIs target \"%s\" too small?\n"),
target[i].name);
die_perror();
}
+ free(lb);
}
}
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 3/9] xfs_copy: actually do directio writes to block devices
2023-11-22 23:07 ` [PATCH 3/9] xfs_copy: actually do directio writes to block devices Darrick J. Wong
@ 2023-11-23 6:40 ` Christoph Hellwig
2023-11-27 18:14 ` Darrick J. Wong
0 siblings, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:40 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
> diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> index 79f65946709..26de6b2ee1c 100644
> --- a/copy/xfs_copy.c
> +++ b/copy/xfs_copy.c
> @@ -854,6 +854,8 @@ main(int argc, char **argv)
> progname, target[i].name, progname);
> exit(1);
> }
> + if (!buffered_output)
> + open_flags |= O_DIRECT;
> }
I'd just move this outside of the if/else if chain and do the
assignment once.
> @@ -887,20 +889,22 @@ main(int argc, char **argv)
> }
> }
> } else {
> - char *lb[XFS_MAX_SECTORSIZE] = { NULL };
> + char *lb = memalign(wbuf_align, XFS_MAX_SECTORSIZE);
> off64_t off;
>
> /* ensure device files are sufficiently large */
> + memset(lb, 0, XFS_MAX_SECTORSIZE);
>
> off = mp->m_sb.sb_dblocks * source_blocksize;
> + off -= XFS_MAX_SECTORSIZE;
> + if (pwrite(target[i].fd, lb, XFS_MAX_SECTORSIZE, off) < 0) {
We should probably check for a short write as well?
Also this line is a bit long.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 3/9] xfs_copy: actually do directio writes to block devices
2023-11-23 6:40 ` Christoph Hellwig
@ 2023-11-27 18:14 ` Darrick J. Wong
0 siblings, 0 replies; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-27 18:14 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 10:40:32PM -0800, Christoph Hellwig wrote:
> > diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
> > index 79f65946709..26de6b2ee1c 100644
> > --- a/copy/xfs_copy.c
> > +++ b/copy/xfs_copy.c
> > @@ -854,6 +854,8 @@ main(int argc, char **argv)
> > progname, target[i].name, progname);
> > exit(1);
> > }
> > + if (!buffered_output)
> > + open_flags |= O_DIRECT;
> > }
>
> I'd just move this outside of the if/else if chain and do the
> assignment once.
Fixed.
> > @@ -887,20 +889,22 @@ main(int argc, char **argv)
> > }
> > }
> > } else {
> > - char *lb[XFS_MAX_SECTORSIZE] = { NULL };
> > + char *lb = memalign(wbuf_align, XFS_MAX_SECTORSIZE);
> > off64_t off;
> >
> > /* ensure device files are sufficiently large */
> > + memset(lb, 0, XFS_MAX_SECTORSIZE);
> >
> > off = mp->m_sb.sb_dblocks * source_blocksize;
> > + off -= XFS_MAX_SECTORSIZE;
> > + if (pwrite(target[i].fd, lb, XFS_MAX_SECTORSIZE, off) < 0) {
>
> We should probably check for a short write as well?
> Also this line is a bit long.
Ok, I'll check for short writes and split the error messaging so that it
no longer says "Is target too small?" on EIO.
--D
>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 4/9] xfs_db: report the device associated with each io cursor
2023-11-22 23:06 [PATCHSET 0/9] xfsprogs: minor fixes for 6.6 Darrick J. Wong
` (2 preceding siblings ...)
2023-11-22 23:07 ` [PATCH 3/9] xfs_copy: actually do directio writes to block devices Darrick J. Wong
@ 2023-11-22 23:07 ` Darrick J. Wong
2023-11-23 6:41 ` Christoph Hellwig
2023-11-22 23:07 ` [PATCH 5/9] xfs_metadump.8: update for external log device options Darrick J. Wong
` (4 subsequent siblings)
8 siblings, 1 reply; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-22 23:07 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
When db is reporting on an io cursor, have it print out the device
that the cursor is pointing to.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
db/block.c | 14 +++++++++++++-
db/io.c | 35 ++++++++++++++++++++++++++++++++---
db/io.h | 3 +++
3 files changed, 48 insertions(+), 4 deletions(-)
diff --git a/db/block.c b/db/block.c
index 788337d3709..d730c779671 100644
--- a/db/block.c
+++ b/db/block.c
@@ -126,7 +126,15 @@ daddr_f(
char *p;
if (argc == 1) {
- dbprintf(_("current daddr is %lld\n"), iocur_top->off >> BBSHIFT);
+ xfs_daddr_t daddr = iocur_top->off >> BBSHIFT;
+
+ if (iocur_is_ddev(iocur_top))
+ dbprintf(_("datadev daddr is %lld\n"), daddr);
+ else if (iocur_is_extlogdev(iocur_top))
+ dbprintf(_("logdev daddr is %lld\n"), daddr);
+ else
+ dbprintf(_("current daddr is %lld\n"), daddr);
+
return 0;
}
d = (int64_t)strtoull(argv[1], &p, 0);
@@ -220,6 +228,10 @@ fsblock_f(
char *p;
if (argc == 1) {
+ if (!iocur_is_ddev(iocur_top)) {
+ dbprintf(_("cursor does not point to data device\n"));
+ return 0;
+ }
dbprintf(_("current fsblock is %lld\n"),
XFS_DADDR_TO_FSB(mp, iocur_top->off >> BBSHIFT));
return 0;
diff --git a/db/io.c b/db/io.c
index 5ccfe3b536a..590dd1f82f7 100644
--- a/db/io.c
+++ b/db/io.c
@@ -137,18 +137,47 @@ pop_help(void)
));
}
+bool
+iocur_is_ddev(const struct iocur *ioc)
+{
+ if (!ioc->bp)
+ return false;
+
+ return ioc->bp->b_target == ioc->bp->b_mount->m_ddev_targp;
+}
+
+bool
+iocur_is_extlogdev(const struct iocur *ioc)
+{
+ struct xfs_buf *bp = ioc->bp;
+
+ if (!bp)
+ return false;
+ if (bp->b_mount->m_logdev_targp == bp->b_mount->m_ddev_targp)
+ return false;
+
+ return bp->b_target == bp->b_mount->m_logdev_targp;
+}
+
void
print_iocur(
char *tag,
iocur_t *ioc)
{
+ const char *block_unit = "fsbno?";
int i;
+ if (iocur_is_ddev(ioc))
+ block_unit = "fsbno";
+ else if (iocur_is_extlogdev(ioc))
+ block_unit = "logbno";
+
dbprintf("%s\n", tag);
dbprintf(_("\tbyte offset %lld, length %d\n"), ioc->off, ioc->len);
- dbprintf(_("\tbuffer block %lld (fsbno %lld), %d bb%s\n"), ioc->bb,
- (xfs_fsblock_t)XFS_DADDR_TO_FSB(mp, ioc->bb), ioc->blen,
- ioc->blen == 1 ? "" : "s");
+ dbprintf(_("\tbuffer block %lld (%s %lld), %d bb%s\n"), ioc->bb,
+ block_unit,
+ (xfs_fsblock_t)XFS_DADDR_TO_FSB(mp, ioc->bb),
+ ioc->blen, ioc->blen == 1 ? "" : "s");
if (ioc->bbmap) {
dbprintf(_("\tblock map"));
for (i = 0; i < ioc->bbmap->nmaps; i++)
diff --git a/db/io.h b/db/io.h
index bd86c31f67e..f48b67b47a2 100644
--- a/db/io.h
+++ b/db/io.h
@@ -56,6 +56,9 @@ extern void set_iocur_type(const struct typ *type);
extern void xfs_dummy_verify(struct xfs_buf *bp);
extern void xfs_verify_recalc_crc(struct xfs_buf *bp);
+bool iocur_is_ddev(const struct iocur *ioc);
+bool iocur_is_extlogdev(const struct iocur *ioc);
+
/*
* returns -1 for unchecked, 0 for bad and 1 for good
*/
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 4/9] xfs_db: report the device associated with each io cursor
2023-11-22 23:07 ` [PATCH 4/9] xfs_db: report the device associated with each io cursor Darrick J. Wong
@ 2023-11-23 6:41 ` Christoph Hellwig
2023-11-27 18:24 ` Darrick J. Wong
0 siblings, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:07:10PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> When db is reporting on an io cursor, have it print out the device
> that the cursor is pointing to.
This looks very useful. But I wonder if it risks breaking a lot
of scripts?
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 4/9] xfs_db: report the device associated with each io cursor
2023-11-23 6:41 ` Christoph Hellwig
@ 2023-11-27 18:24 ` Darrick J. Wong
2023-11-28 5:37 ` Christoph Hellwig
0 siblings, 1 reply; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-27 18:24 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 10:41:09PM -0800, Christoph Hellwig wrote:
> On Wed, Nov 22, 2023 at 03:07:10PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > When db is reporting on an io cursor, have it print out the device
> > that the cursor is pointing to.
>
> This looks very useful. But I wonder if it risks breaking a lot
> of scripts?
<shrug> There's nothing in fstests that depends on the output of the
'stack' command, and debian code search didn't come up with any hits.
--D
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 4/9] xfs_db: report the device associated with each io cursor
2023-11-27 18:24 ` Darrick J. Wong
@ 2023-11-28 5:37 ` Christoph Hellwig
0 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-28 5:37 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, cem, linux-xfs
On Mon, Nov 27, 2023 at 10:24:14AM -0800, Darrick J. Wong wrote:
> On Wed, Nov 22, 2023 at 10:41:09PM -0800, Christoph Hellwig wrote:
> > On Wed, Nov 22, 2023 at 03:07:10PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > When db is reporting on an io cursor, have it print out the device
> > > that the cursor is pointing to.
> >
> > This looks very useful. But I wonder if it risks breaking a lot
> > of scripts?
>
> <shrug> There's nothing in fstests that depends on the output of the
> 'stack' command, and debian code search didn't come up with any hits.
I guess that's about as good as it gets..
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 5/9] xfs_metadump.8: update for external log device options
2023-11-22 23:06 [PATCHSET 0/9] xfsprogs: minor fixes for 6.6 Darrick J. Wong
` (3 preceding siblings ...)
2023-11-22 23:07 ` [PATCH 4/9] xfs_db: report the device associated with each io cursor Darrick J. Wong
@ 2023-11-22 23:07 ` Darrick J. Wong
2023-11-23 6:41 ` Christoph Hellwig
2023-11-24 9:11 ` Chandan Babu R
2023-11-22 23:07 ` [PATCH 6/9] xfs_mdrestore: fix uninitialized variables in mdrestore main Darrick J. Wong
` (3 subsequent siblings)
8 siblings, 2 replies; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-22 23:07 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Update the documentation to reflect that we can metadump external log
device contents.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
man/man8/xfs_mdrestore.8 | 6 +++++-
man/man8/xfs_metadump.8 | 7 +++++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/man/man8/xfs_mdrestore.8 b/man/man8/xfs_mdrestore.8
index 6e7457c0445..f60e7b56ebf 100644
--- a/man/man8/xfs_mdrestore.8
+++ b/man/man8/xfs_mdrestore.8
@@ -14,6 +14,10 @@ xfs_mdrestore \- restores an XFS metadump image to a filesystem image
.br
.B xfs_mdrestore
.B \-i
+[
+.B \-l
+.I logdev
+]
.I source
.br
.B xfs_mdrestore \-V
@@ -52,7 +56,7 @@ Shows metadump information on stdout. If no
is specified, exits after displaying information. Older metadumps man not
include any descriptive information.
.TP
-.B \-l " logdev"
+.BI \-l " logdev"
Metadump in v2 format can contain metadata dumped from an external log.
In such a scenario, the user has to provide a device to which the log device
contents from the metadump file are copied.
diff --git a/man/man8/xfs_metadump.8 b/man/man8/xfs_metadump.8
index 1732012cd0c..496b5926603 100644
--- a/man/man8/xfs_metadump.8
+++ b/man/man8/xfs_metadump.8
@@ -132,8 +132,11 @@ is stdout.
.TP
.BI \-l " logdev"
For filesystems which use an external log, this specifies the device where the
-external log resides. The external log is not copied, only internal logs are
-copied.
+external log resides.
+If the v2 metadump format is selected, the contents of the external log will be
+copied to the metadump.
+The v2 metadump format will be selected automatically if this option is
+specified.
.TP
.B \-m
Set the maximum size of an allowed metadata extent. Extremely large metadata
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 5/9] xfs_metadump.8: update for external log device options
2023-11-22 23:07 ` [PATCH 5/9] xfs_metadump.8: update for external log device options Darrick J. Wong
@ 2023-11-23 6:41 ` Christoph Hellwig
2023-11-24 9:11 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 5/9] xfs_metadump.8: update for external log device options
2023-11-22 23:07 ` [PATCH 5/9] xfs_metadump.8: update for external log device options Darrick J. Wong
2023-11-23 6:41 ` Christoph Hellwig
@ 2023-11-24 9:11 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Chandan Babu R @ 2023-11-24 9:11 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:07:16 PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Update the documentation to reflect that we can metadump external log
> device contents.
>
Looks good to me.
Reviewed-by: Chandan Babu R <chandanbabu@kernel.org>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> man/man8/xfs_mdrestore.8 | 6 +++++-
> man/man8/xfs_metadump.8 | 7 +++++--
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
>
> diff --git a/man/man8/xfs_mdrestore.8 b/man/man8/xfs_mdrestore.8
> index 6e7457c0445..f60e7b56ebf 100644
> --- a/man/man8/xfs_mdrestore.8
> +++ b/man/man8/xfs_mdrestore.8
> @@ -14,6 +14,10 @@ xfs_mdrestore \- restores an XFS metadump image to a filesystem image
> .br
> .B xfs_mdrestore
> .B \-i
> +[
> +.B \-l
> +.I logdev
> +]
> .I source
> .br
> .B xfs_mdrestore \-V
> @@ -52,7 +56,7 @@ Shows metadump information on stdout. If no
> is specified, exits after displaying information. Older metadumps man not
> include any descriptive information.
> .TP
> -.B \-l " logdev"
> +.BI \-l " logdev"
> Metadump in v2 format can contain metadata dumped from an external log.
> In such a scenario, the user has to provide a device to which the log device
> contents from the metadump file are copied.
> diff --git a/man/man8/xfs_metadump.8 b/man/man8/xfs_metadump.8
> index 1732012cd0c..496b5926603 100644
> --- a/man/man8/xfs_metadump.8
> +++ b/man/man8/xfs_metadump.8
> @@ -132,8 +132,11 @@ is stdout.
> .TP
> .BI \-l " logdev"
> For filesystems which use an external log, this specifies the device where the
> -external log resides. The external log is not copied, only internal logs are
> -copied.
> +external log resides.
> +If the v2 metadump format is selected, the contents of the external log will be
> +copied to the metadump.
> +The v2 metadump format will be selected automatically if this option is
> +specified.
> .TP
> .B \-m
> Set the maximum size of an allowed metadata extent. Extremely large metadata
--
Chandan
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 6/9] xfs_mdrestore: fix uninitialized variables in mdrestore main
2023-11-22 23:06 [PATCHSET 0/9] xfsprogs: minor fixes for 6.6 Darrick J. Wong
` (4 preceding siblings ...)
2023-11-22 23:07 ` [PATCH 5/9] xfs_metadump.8: update for external log device options Darrick J. Wong
@ 2023-11-22 23:07 ` Darrick J. Wong
2023-11-23 6:41 ` Christoph Hellwig
2023-11-24 9:11 ` Chandan Babu R
2023-11-22 23:07 ` [PATCH 7/9] xfs_mdrestore: emit newlines for fatal errors Darrick J. Wong
` (2 subsequent siblings)
8 siblings, 2 replies; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-22 23:07 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Coverity complained about the "is fd a file?" flags being uninitialized.
Clean this up.
Coverity-id: 1554270
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
mdrestore/xfs_mdrestore.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index 2de177c6e3f..5dfc423493e 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -472,11 +472,11 @@ main(
union mdrestore_headers headers;
FILE *src_f;
char *logdev = NULL;
- int data_dev_fd;
- int log_dev_fd;
+ int data_dev_fd = -1;
+ int log_dev_fd = -1;
int c;
- bool is_data_dev_file;
- bool is_log_dev_file;
+ bool is_data_dev_file = false;
+ bool is_log_dev_file = false;
mdrestore.show_progress = false;
mdrestore.show_info = false;
@@ -561,7 +561,6 @@ main(
/* check and open data device */
data_dev_fd = open_device(argv[optind], &is_data_dev_file);
- log_dev_fd = -1;
if (mdrestore.external_log)
/* check and open log device */
log_dev_fd = open_device(logdev, &is_log_dev_file);
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 6/9] xfs_mdrestore: fix uninitialized variables in mdrestore main
2023-11-22 23:07 ` [PATCH 6/9] xfs_mdrestore: fix uninitialized variables in mdrestore main Darrick J. Wong
@ 2023-11-23 6:41 ` Christoph Hellwig
2023-11-24 9:11 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 6/9] xfs_mdrestore: fix uninitialized variables in mdrestore main
2023-11-22 23:07 ` [PATCH 6/9] xfs_mdrestore: fix uninitialized variables in mdrestore main Darrick J. Wong
2023-11-23 6:41 ` Christoph Hellwig
@ 2023-11-24 9:11 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Chandan Babu R @ 2023-11-24 9:11 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:07:22 PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Coverity complained about the "is fd a file?" flags being uninitialized.
> Clean this up.
>
Looks good to me.
Reviewed-by: Chandan Babu R <chandanbabu@kernel.org>
> Coverity-id: 1554270
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> mdrestore/xfs_mdrestore.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
>
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 2de177c6e3f..5dfc423493e 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -472,11 +472,11 @@ main(
> union mdrestore_headers headers;
> FILE *src_f;
> char *logdev = NULL;
> - int data_dev_fd;
> - int log_dev_fd;
> + int data_dev_fd = -1;
> + int log_dev_fd = -1;
> int c;
> - bool is_data_dev_file;
> - bool is_log_dev_file;
> + bool is_data_dev_file = false;
> + bool is_log_dev_file = false;
>
> mdrestore.show_progress = false;
> mdrestore.show_info = false;
> @@ -561,7 +561,6 @@ main(
> /* check and open data device */
> data_dev_fd = open_device(argv[optind], &is_data_dev_file);
>
> - log_dev_fd = -1;
> if (mdrestore.external_log)
> /* check and open log device */
> log_dev_fd = open_device(logdev, &is_log_dev_file);
--
Chandan
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 7/9] xfs_mdrestore: emit newlines for fatal errors
2023-11-22 23:06 [PATCHSET 0/9] xfsprogs: minor fixes for 6.6 Darrick J. Wong
` (5 preceding siblings ...)
2023-11-22 23:07 ` [PATCH 6/9] xfs_mdrestore: fix uninitialized variables in mdrestore main Darrick J. Wong
@ 2023-11-22 23:07 ` Darrick J. Wong
2023-11-23 6:41 ` Christoph Hellwig
2023-11-24 9:12 ` Chandan Babu R
2023-11-22 23:07 ` [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat Darrick J. Wong
2023-11-22 23:07 ` [PATCH 9/9] xfs_mdrestore: fix missed progress reporting Darrick J. Wong
8 siblings, 2 replies; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-22 23:07 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Spit out a newline after a fatal error message.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
mdrestore/xfs_mdrestore.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index 5dfc423493e..3190e07e478 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -275,10 +275,10 @@ read_header_v2(
fatal("error reading from metadump file\n");
if (h->v2.xmh_incompat_flags != 0)
- fatal("Metadump header has unknown incompat flags set");
+ fatal("Metadump header has unknown incompat flags set\n");
if (h->v2.xmh_reserved != 0)
- fatal("Metadump header's reserved field has a non-zero value");
+ fatal("Metadump header's reserved field has a non-zero value\n");
want_external_log = !!(be32_to_cpu(h->v2.xmh_incompat_flags) &
XFS_MD2_COMPAT_EXTERNALLOG);
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 7/9] xfs_mdrestore: emit newlines for fatal errors
2023-11-22 23:07 ` [PATCH 7/9] xfs_mdrestore: emit newlines for fatal errors Darrick J. Wong
@ 2023-11-23 6:41 ` Christoph Hellwig
2023-11-24 9:12 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 7/9] xfs_mdrestore: emit newlines for fatal errors
2023-11-22 23:07 ` [PATCH 7/9] xfs_mdrestore: emit newlines for fatal errors Darrick J. Wong
2023-11-23 6:41 ` Christoph Hellwig
@ 2023-11-24 9:12 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Chandan Babu R @ 2023-11-24 9:12 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:07:28 PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Spit out a newline after a fatal error message.
>
Looks good to me.
Reviewed-by: Chandan Babu R <chandanbabu@kernel.org>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> mdrestore/xfs_mdrestore.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 5dfc423493e..3190e07e478 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -275,10 +275,10 @@ read_header_v2(
> fatal("error reading from metadump file\n");
>
> if (h->v2.xmh_incompat_flags != 0)
> - fatal("Metadump header has unknown incompat flags set");
> + fatal("Metadump header has unknown incompat flags set\n");
>
> if (h->v2.xmh_reserved != 0)
> - fatal("Metadump header's reserved field has a non-zero value");
> + fatal("Metadump header's reserved field has a non-zero value\n");
>
> want_external_log = !!(be32_to_cpu(h->v2.xmh_incompat_flags) &
> XFS_MD2_COMPAT_EXTERNALLOG);
--
Chandan
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat
2023-11-22 23:06 [PATCHSET 0/9] xfsprogs: minor fixes for 6.6 Darrick J. Wong
` (6 preceding siblings ...)
2023-11-22 23:07 ` [PATCH 7/9] xfs_mdrestore: emit newlines for fatal errors Darrick J. Wong
@ 2023-11-22 23:07 ` Darrick J. Wong
2023-11-23 6:42 ` Christoph Hellwig
2023-11-24 9:14 ` Chandan Babu R
2023-11-22 23:07 ` [PATCH 9/9] xfs_mdrestore: fix missed progress reporting Darrick J. Wong
8 siblings, 2 replies; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-22 23:07 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Fix this check to look at the correct header field.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
mdrestore/xfs_mdrestore.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index 3190e07e478..3f761e8fe8d 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -268,8 +268,6 @@ read_header_v2(
union mdrestore_headers *h,
FILE *md_fp)
{
- bool want_external_log;
-
if (fread((uint8_t *)&(h->v2) + sizeof(h->v2.xmh_magic),
sizeof(h->v2) - sizeof(h->v2.xmh_magic), 1, md_fp) != 1)
fatal("error reading from metadump file\n");
@@ -280,10 +278,8 @@ read_header_v2(
if (h->v2.xmh_reserved != 0)
fatal("Metadump header's reserved field has a non-zero value\n");
- want_external_log = !!(be32_to_cpu(h->v2.xmh_incompat_flags) &
- XFS_MD2_COMPAT_EXTERNALLOG);
-
- if (want_external_log && !mdrestore.external_log)
+ if ((h->v2.xmh_compat_flags & cpu_to_be32(XFS_MD2_COMPAT_EXTERNALLOG)) &&
+ !mdrestore.external_log)
fatal("External Log device is required\n");
}
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat
2023-11-22 23:07 ` [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat Darrick J. Wong
@ 2023-11-23 6:42 ` Christoph Hellwig
2023-11-23 6:42 ` Christoph Hellwig
2023-11-27 18:27 ` Darrick J. Wong
2023-11-24 9:14 ` Chandan Babu R
1 sibling, 2 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:42 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:07:33PM -0800, Darrick J. Wong wrote:
> @@ -280,10 +278,8 @@ read_header_v2(
> if (h->v2.xmh_reserved != 0)
> fatal("Metadump header's reserved field has a non-zero value\n");
>
> - want_external_log = !!(be32_to_cpu(h->v2.xmh_incompat_flags) &
> - XFS_MD2_COMPAT_EXTERNALLOG);
> -
> - if (want_external_log && !mdrestore.external_log)
> + if ((h->v2.xmh_compat_flags & cpu_to_be32(XFS_MD2_COMPAT_EXTERNALLOG)) &&
> + !mdrestore.external_log)
Nit: overly long line. Trivially fixable by just inverting the
conditions :)
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat
2023-11-23 6:42 ` Christoph Hellwig
@ 2023-11-23 6:42 ` Christoph Hellwig
2023-11-27 18:27 ` Darrick J. Wong
1 sibling, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:42 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 10:42:40PM -0800, Christoph Hellwig wrote:
> Nit: overly long line. Trivially fixable by just inverting the
> conditions :)
s/invert/reorder/
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat
2023-11-23 6:42 ` Christoph Hellwig
2023-11-23 6:42 ` Christoph Hellwig
@ 2023-11-27 18:27 ` Darrick J. Wong
2023-11-28 5:38 ` Christoph Hellwig
1 sibling, 1 reply; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-27 18:27 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 10:42:40PM -0800, Christoph Hellwig wrote:
> On Wed, Nov 22, 2023 at 03:07:33PM -0800, Darrick J. Wong wrote:
> > @@ -280,10 +278,8 @@ read_header_v2(
> > if (h->v2.xmh_reserved != 0)
> > fatal("Metadump header's reserved field has a non-zero value\n");
> >
> > - want_external_log = !!(be32_to_cpu(h->v2.xmh_incompat_flags) &
> > - XFS_MD2_COMPAT_EXTERNALLOG);
> > -
> > - if (want_external_log && !mdrestore.external_log)
> > + if ((h->v2.xmh_compat_flags & cpu_to_be32(XFS_MD2_COMPAT_EXTERNALLOG)) &&
> > + !mdrestore.external_log)
>
> Nit: overly long line. Trivially fixable by just inverting the
> conditions :)
Hmm. Or I could decode the ondisk field into a stack variable so that
future flags don't have to deal with that:
compat = be32_to_cpu(h->v2.xmh_compat_flags);
if (!mdrestore.external_log && (compat & XFS_MD2_COMPAT_EXTERNALLOG))
fatal("External Log device is required\n");
--D
>
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat
2023-11-27 18:27 ` Darrick J. Wong
@ 2023-11-28 5:38 ` Christoph Hellwig
0 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-28 5:38 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, cem, linux-xfs
On Mon, Nov 27, 2023 at 10:27:38AM -0800, Darrick J. Wong wrote:
> Hmm. Or I could decode the ondisk field into a stack variable so that
> future flags don't have to deal with that:
>
> compat = be32_to_cpu(h->v2.xmh_compat_flags);
>
> if (!mdrestore.external_log && (compat & XFS_MD2_COMPAT_EXTERNALLOG))
> fatal("External Log device is required\n");
That looks even better.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat
2023-11-22 23:07 ` [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat Darrick J. Wong
2023-11-23 6:42 ` Christoph Hellwig
@ 2023-11-24 9:14 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Chandan Babu R @ 2023-11-24 9:14 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:07:33 PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Fix this check to look at the correct header field.
Looks good to me.
Reviewed-by: Chandan Babu R <chandanbabu@kernel.org>
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> mdrestore/xfs_mdrestore.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
>
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 3190e07e478..3f761e8fe8d 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -268,8 +268,6 @@ read_header_v2(
> union mdrestore_headers *h,
> FILE *md_fp)
> {
> - bool want_external_log;
> -
> if (fread((uint8_t *)&(h->v2) + sizeof(h->v2.xmh_magic),
> sizeof(h->v2) - sizeof(h->v2.xmh_magic), 1, md_fp) != 1)
> fatal("error reading from metadump file\n");
> @@ -280,10 +278,8 @@ read_header_v2(
> if (h->v2.xmh_reserved != 0)
> fatal("Metadump header's reserved field has a non-zero value\n");
>
> - want_external_log = !!(be32_to_cpu(h->v2.xmh_incompat_flags) &
> - XFS_MD2_COMPAT_EXTERNALLOG);
> -
> - if (want_external_log && !mdrestore.external_log)
> + if ((h->v2.xmh_compat_flags & cpu_to_be32(XFS_MD2_COMPAT_EXTERNALLOG)) &&
> + !mdrestore.external_log)
> fatal("External Log device is required\n");
> }
>
--
Chandan
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 9/9] xfs_mdrestore: fix missed progress reporting
2023-11-22 23:06 [PATCHSET 0/9] xfsprogs: minor fixes for 6.6 Darrick J. Wong
` (7 preceding siblings ...)
2023-11-22 23:07 ` [PATCH 8/9] xfs_mdrestore: EXTERNALLOG is a compat value, not incompat Darrick J. Wong
@ 2023-11-22 23:07 ` Darrick J. Wong
2023-11-23 6:44 ` Christoph Hellwig
2023-11-24 9:14 ` Chandan Babu R
8 siblings, 2 replies; 36+ messages in thread
From: Darrick J. Wong @ 2023-11-22 23:07 UTC (permalink / raw)
To: djwong, cem; +Cc: linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Currently, the progress reporting only triggers when the number of bytes
read is exactly a multiple of a megabyte. This isn't always guaranteed,
since AG headers can be 512 bytes in size. Fix the algorithm by
recording the number of megabytes we've reported as being read, and emit
a new report any time the bytes_read count, once converted to megabytes,
doesn't match.
Fix the v2 code to emit one final status message in case the last
extent restored is more than a megabyte.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
mdrestore/xfs_mdrestore.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
index 3f761e8fe8d..ab9a44d2118 100644
--- a/mdrestore/xfs_mdrestore.c
+++ b/mdrestore/xfs_mdrestore.c
@@ -7,6 +7,7 @@
#include "libxfs.h"
#include "xfs_metadump.h"
#include <libfrog/platform.h>
+#include "libfrog/div64.h"
union mdrestore_headers {
__be32 magic;
@@ -160,6 +161,7 @@ restore_v1(
int mb_count;
xfs_sb_t sb;
int64_t bytes_read;
+ int64_t mb_read = 0;
block_size = 1 << h->v1.mb_blocklog;
max_indices = (block_size - sizeof(xfs_metablock_t)) / sizeof(__be64);
@@ -208,9 +210,14 @@ restore_v1(
bytes_read = 0;
for (;;) {
- if (mdrestore.show_progress &&
- (bytes_read & ((1 << 20) - 1)) == 0)
- print_progress("%lld MB read", bytes_read >> 20);
+ if (mdrestore.show_progress) {
+ int64_t mb_now = bytes_read >> 20;
+
+ if (mb_now != mb_read) {
+ print_progress("%lld MB read", mb_now);
+ mb_read = mb_now;
+ }
+ }
for (cur_index = 0; cur_index < mb_count; cur_index++) {
if (pwrite(ddev_fd, &block_buffer[cur_index <<
@@ -240,6 +247,9 @@ restore_v1(
bytes_read += block_size + (mb_count << h->v1.mb_blocklog);
}
+ if (mdrestore.show_progress && bytes_read > (mb_read << 20))
+ print_progress("%lld MB read", howmany_64(bytes_read, 1U << 20));
+
if (mdrestore.progress_since_warning)
putchar('\n');
@@ -340,6 +350,7 @@ restore_v2(
struct xfs_sb sb;
struct xfs_meta_extent xme;
char *block_buffer;
+ int64_t mb_read = 0;
int64_t bytes_read;
uint64_t offset;
int len;
@@ -416,16 +427,18 @@ restore_v2(
bytes_read += len;
if (mdrestore.show_progress) {
- static int64_t mb_read;
- int64_t mb_now = bytes_read >> 20;
+ int64_t mb_now = bytes_read >> 20;
if (mb_now != mb_read) {
- print_progress("%lld MB read", mb_now);
+ print_progress("%lld mb read", mb_now);
mb_read = mb_now;
}
}
} while (1);
+ if (mdrestore.show_progress && bytes_read > (mb_read << 20))
+ print_progress("%lld mb read", howmany_64(bytes_read, 1U << 20));
+
if (mdrestore.progress_since_warning)
putchar('\n');
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 9/9] xfs_mdrestore: fix missed progress reporting
2023-11-22 23:07 ` [PATCH 9/9] xfs_mdrestore: fix missed progress reporting Darrick J. Wong
@ 2023-11-23 6:44 ` Christoph Hellwig
2023-11-24 9:14 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2023-11-23 6:44 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
> + if (mdrestore.show_progress) {
> + int64_t mb_now = bytes_read >> 20;
> +
> + if (mb_now != mb_read) {
> + print_progress("%lld MB read", mb_now);
> + mb_read = mb_now;
> + }
> + }
> if (mdrestore.show_progress) {
> + int64_t mb_now = bytes_read >> 20;
>
> if (mb_now != mb_read) {
> + print_progress("%lld mb read", mb_now);
> mb_read = mb_now;
> }
> }
Not sure if it's worth the effort, but to my pattern matching eyes
this screams for a little helper.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 9/9] xfs_mdrestore: fix missed progress reporting
2023-11-22 23:07 ` [PATCH 9/9] xfs_mdrestore: fix missed progress reporting Darrick J. Wong
2023-11-23 6:44 ` Christoph Hellwig
@ 2023-11-24 9:14 ` Chandan Babu R
1 sibling, 0 replies; 36+ messages in thread
From: Chandan Babu R @ 2023-11-24 9:14 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs
On Wed, Nov 22, 2023 at 03:07:39 PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Currently, the progress reporting only triggers when the number of bytes
> read is exactly a multiple of a megabyte. This isn't always guaranteed,
> since AG headers can be 512 bytes in size. Fix the algorithm by
> recording the number of megabytes we've reported as being read, and emit
> a new report any time the bytes_read count, once converted to megabytes,
> doesn't match.
>
> Fix the v2 code to emit one final status message in case the last
> extent restored is more than a megabyte.
Looks good to me.
Reviewed-by: Chandan Babu R <chandanbabu@kernel.org>
Thanks especially for fixing all the bugs in metadump & mdrestore.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> mdrestore/xfs_mdrestore.c | 25 +++++++++++++++++++------
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
>
> diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c
> index 3f761e8fe8d..ab9a44d2118 100644
> --- a/mdrestore/xfs_mdrestore.c
> +++ b/mdrestore/xfs_mdrestore.c
> @@ -7,6 +7,7 @@
> #include "libxfs.h"
> #include "xfs_metadump.h"
> #include <libfrog/platform.h>
> +#include "libfrog/div64.h"
>
> union mdrestore_headers {
> __be32 magic;
> @@ -160,6 +161,7 @@ restore_v1(
> int mb_count;
> xfs_sb_t sb;
> int64_t bytes_read;
> + int64_t mb_read = 0;
>
> block_size = 1 << h->v1.mb_blocklog;
> max_indices = (block_size - sizeof(xfs_metablock_t)) / sizeof(__be64);
> @@ -208,9 +210,14 @@ restore_v1(
> bytes_read = 0;
>
> for (;;) {
> - if (mdrestore.show_progress &&
> - (bytes_read & ((1 << 20) - 1)) == 0)
> - print_progress("%lld MB read", bytes_read >> 20);
> + if (mdrestore.show_progress) {
> + int64_t mb_now = bytes_read >> 20;
> +
> + if (mb_now != mb_read) {
> + print_progress("%lld MB read", mb_now);
> + mb_read = mb_now;
> + }
> + }
>
> for (cur_index = 0; cur_index < mb_count; cur_index++) {
> if (pwrite(ddev_fd, &block_buffer[cur_index <<
> @@ -240,6 +247,9 @@ restore_v1(
> bytes_read += block_size + (mb_count << h->v1.mb_blocklog);
> }
>
> + if (mdrestore.show_progress && bytes_read > (mb_read << 20))
> + print_progress("%lld MB read", howmany_64(bytes_read, 1U << 20));
> +
> if (mdrestore.progress_since_warning)
> putchar('\n');
>
> @@ -340,6 +350,7 @@ restore_v2(
> struct xfs_sb sb;
> struct xfs_meta_extent xme;
> char *block_buffer;
> + int64_t mb_read = 0;
> int64_t bytes_read;
> uint64_t offset;
> int len;
> @@ -416,16 +427,18 @@ restore_v2(
> bytes_read += len;
>
> if (mdrestore.show_progress) {
> - static int64_t mb_read;
> - int64_t mb_now = bytes_read >> 20;
> + int64_t mb_now = bytes_read >> 20;
>
> if (mb_now != mb_read) {
> - print_progress("%lld MB read", mb_now);
> + print_progress("%lld mb read", mb_now);
> mb_read = mb_now;
> }
> }
> } while (1);
>
> + if (mdrestore.show_progress && bytes_read > (mb_read << 20))
> + print_progress("%lld mb read", howmany_64(bytes_read, 1U << 20));
> +
> if (mdrestore.progress_since_warning)
> putchar('\n');
>
--
Chandan
^ permalink raw reply [flat|nested] 36+ messages in thread