* [PATCH 1/5] mkfs: allow users to specify rtinherit=0
2020-10-26 23:31 [PATCH 0/5] xfsprogs: fixes for 5.10 Darrick J. Wong
@ 2020-10-26 23:31 ` Darrick J. Wong
2020-10-27 5:35 ` Allison Henderson
` (3 more replies)
2020-10-26 23:32 ` [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks Darrick J. Wong
` (3 subsequent siblings)
4 siblings, 4 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-10-26 23:31 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
mkfs has quite a few boolean options that can be specified in several
ways: "option=1" (turn it on), "option" (turn it on), or "option=0"
(turn it off). For whatever reason, rtinherit sticks out as the only
mkfs parameter that doesn't behave that way. Let's make it behave the
same as all the other boolean variables.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
mkfs/xfs_mkfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 8fe149d74b0a..908d520df909 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -349,7 +349,7 @@ static struct opt_params dopts = {
},
{ .index = D_RTINHERIT,
.conflicts = { { NULL, LAST_CONFLICT } },
- .minval = 1,
+ .minval = 0,
.maxval = 1,
.defaultval = 1,
},
@@ -1429,6 +1429,8 @@ data_opts_parser(
case D_RTINHERIT:
if (getnum(value, opts, subopt))
cli->fsx.fsx_xflags |= FS_XFLAG_RTINHERIT;
+ else
+ cli->fsx.fsx_xflags &= ~FS_XFLAG_RTINHERIT;
break;
case D_PROJINHERIT:
cli->fsx.fsx_projid = getnum(value, opts, subopt);
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 1/5] mkfs: allow users to specify rtinherit=0
2020-10-26 23:31 ` [PATCH 1/5] mkfs: allow users to specify rtinherit=0 Darrick J. Wong
@ 2020-10-27 5:35 ` Allison Henderson
2020-10-27 17:22 ` Eric Sandeen
` (2 subsequent siblings)
3 siblings, 0 replies; 26+ messages in thread
From: Allison Henderson @ 2020-10-27 5:35 UTC (permalink / raw)
To: Darrick J. Wong, sandeen; +Cc: linux-xfs
On 10/26/20 4:31 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> mkfs has quite a few boolean options that can be specified in several
> ways: "option=1" (turn it on), "option" (turn it on), or "option=0"
> (turn it off). For whatever reason, rtinherit sticks out as the only
> mkfs parameter that doesn't behave that way. Let's make it behave the
> same as all the other boolean variables.
>
Looks ok
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> mkfs/xfs_mkfs.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 8fe149d74b0a..908d520df909 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -349,7 +349,7 @@ static struct opt_params dopts = {
> },
> { .index = D_RTINHERIT,
> .conflicts = { { NULL, LAST_CONFLICT } },
> - .minval = 1,
> + .minval = 0,
> .maxval = 1,
> .defaultval = 1,
> },
> @@ -1429,6 +1429,8 @@ data_opts_parser(
> case D_RTINHERIT:
> if (getnum(value, opts, subopt))
> cli->fsx.fsx_xflags |= FS_XFLAG_RTINHERIT;
> + else
> + cli->fsx.fsx_xflags &= ~FS_XFLAG_RTINHERIT;
> break;
> case D_PROJINHERIT:
> cli->fsx.fsx_projid = getnum(value, opts, subopt);
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/5] mkfs: allow users to specify rtinherit=0
2020-10-26 23:31 ` [PATCH 1/5] mkfs: allow users to specify rtinherit=0 Darrick J. Wong
2020-10-27 5:35 ` Allison Henderson
@ 2020-10-27 17:22 ` Eric Sandeen
2020-10-27 17:24 ` [PATCH 1.5/5] mkfs: clarify valid "inherit" option values Eric Sandeen
2020-10-28 7:32 ` [PATCH 1/5] mkfs: allow users to specify rtinherit=0 Christoph Hellwig
3 siblings, 0 replies; 26+ messages in thread
From: Eric Sandeen @ 2020-10-27 17:22 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On 10/26/20 6:31 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> mkfs has quite a few boolean options that can be specified in several
> ways: "option=1" (turn it on), "option" (turn it on), or "option=0"
> (turn it off). For whatever reason, rtinherit sticks out as the only
> mkfs parameter that doesn't behave that way. Let's make it behave the
> same as all the other boolean variables.
Seems fine; tho looking over mkfs.xfs.8, I think we could clarify what all
the valid ${FOO}inherit=[value] values are, in general.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
-Eric
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> mkfs/xfs_mkfs.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 8fe149d74b0a..908d520df909 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -349,7 +349,7 @@ static struct opt_params dopts = {
> },
> { .index = D_RTINHERIT,
> .conflicts = { { NULL, LAST_CONFLICT } },
> - .minval = 1,
> + .minval = 0,
> .maxval = 1,
> .defaultval = 1,
> },
> @@ -1429,6 +1429,8 @@ data_opts_parser(
> case D_RTINHERIT:
> if (getnum(value, opts, subopt))
> cli->fsx.fsx_xflags |= FS_XFLAG_RTINHERIT;
> + else
> + cli->fsx.fsx_xflags &= ~FS_XFLAG_RTINHERIT;
> break;
> case D_PROJINHERIT:
> cli->fsx.fsx_projid = getnum(value, opts, subopt);
>
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 1.5/5] mkfs: clarify valid "inherit" option values
2020-10-26 23:31 ` [PATCH 1/5] mkfs: allow users to specify rtinherit=0 Darrick J. Wong
2020-10-27 5:35 ` Allison Henderson
2020-10-27 17:22 ` Eric Sandeen
@ 2020-10-27 17:24 ` Eric Sandeen
2020-10-27 17:40 ` Darrick J. Wong
2020-10-27 17:50 ` [PATCH 1.5/5 V2] " Eric Sandeen
2020-10-28 7:32 ` [PATCH 1/5] mkfs: allow users to specify rtinherit=0 Christoph Hellwig
3 siblings, 2 replies; 26+ messages in thread
From: Eric Sandeen @ 2020-10-27 17:24 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
Clarify which values are valid for the various *inherit= mkfs
options.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 0a785874..a2be066b 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -377,21 +377,21 @@ This option disables automatic geometry detection and creates the filesystem
without stripe geometry alignment even if the underlying storage device provides
this information.
.TP
-.BI rtinherit= value
-If set, all inodes created by
+.BI rtinherit= [0|1]
+If set to 1, all inodes created by
.B mkfs.xfs
will be created with the realtime flag set.
Directories will pass on this flag to newly created regular files and
directories.
.TP
-.BI projinherit= value
+.BI projinherit= projid
All inodes created by
.B mkfs.xfs
will be assigned this project quota id.
Directories will pass on the project id to newly created regular files and
directories.
.TP
-.BI extszinherit= value
+.BI extszinherit= extentsize
All inodes created by
.B mkfs.xfs
will have this extent size hint applied.
@@ -399,8 +399,8 @@ The value must be provided in units of filesystem blocks.
Directories will pass on this hint to newly created regular files and
directories.
.TP
-.BI daxinherit= value
-If set, all inodes created by
+.BI daxinherit= [0|1]
+If set to 1, all inodes created by
.B mkfs.xfs
will be created with the DAX flag set.
Directories will pass on this flag to newly created regular files and
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1.5/5] mkfs: clarify valid "inherit" option values
2020-10-27 17:24 ` [PATCH 1.5/5] mkfs: clarify valid "inherit" option values Eric Sandeen
@ 2020-10-27 17:40 ` Darrick J. Wong
2020-10-27 17:49 ` Eric Sandeen
2020-10-27 17:50 ` [PATCH 1.5/5 V2] " Eric Sandeen
1 sibling, 1 reply; 26+ messages in thread
From: Darrick J. Wong @ 2020-10-27 17:40 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Tue, Oct 27, 2020 at 12:24:29PM -0500, Eric Sandeen wrote:
> Clarify which values are valid for the various *inherit= mkfs
> options.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index 0a785874..a2be066b 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -377,21 +377,21 @@ This option disables automatic geometry detection and creates the filesystem
> without stripe geometry alignment even if the underlying storage device provides
> this information.
> .TP
> -.BI rtinherit= value
> -If set, all inodes created by
> +.BI rtinherit= [0|1]
> +If set to 1, all inodes created by
> .B mkfs.xfs
> will be created with the realtime flag set.
> Directories will pass on this flag to newly created regular files and
> directories.
> .TP
> -.BI projinherit= value
> +.BI projinherit= projid
> All inodes created by
> .B mkfs.xfs
> will be assigned this project quota id.
> Directories will pass on the project id to newly created regular files and
> directories.
> .TP
> -.BI extszinherit= value
> +.BI extszinherit= extentsize
Hmm... if you're going to make this change to extszinherit, you might as
well do the same for cowextsize.
With that fixed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> All inodes created by
> .B mkfs.xfs
> will have this extent size hint applied.
> @@ -399,8 +399,8 @@ The value must be provided in units of filesystem blocks.
> Directories will pass on this hint to newly created regular files and
> directories.
> .TP
> -.BI daxinherit= value
> -If set, all inodes created by
> +.BI daxinherit= [0|1]
> +If set to 1, all inodes created by
> .B mkfs.xfs
> will be created with the DAX flag set.
> Directories will pass on this flag to newly created regular files and
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1.5/5] mkfs: clarify valid "inherit" option values
2020-10-27 17:40 ` Darrick J. Wong
@ 2020-10-27 17:49 ` Eric Sandeen
2020-10-28 7:32 ` Christoph Hellwig
0 siblings, 1 reply; 26+ messages in thread
From: Eric Sandeen @ 2020-10-27 17:49 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On 10/27/20 12:40 PM, Darrick J. Wong wrote:
> On Tue, Oct 27, 2020 at 12:24:29PM -0500, Eric Sandeen wrote:
>> Clarify which values are valid for the various *inherit= mkfs
>> options.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
>> index 0a785874..a2be066b 100644
>> --- a/man/man8/mkfs.xfs.8
>> +++ b/man/man8/mkfs.xfs.8
>> @@ -377,21 +377,21 @@ This option disables automatic geometry detection and creates the filesystem
>> without stripe geometry alignment even if the underlying storage device provides
>> this information.
>> .TP
>> -.BI rtinherit= value
>> -If set, all inodes created by
>> +.BI rtinherit= [0|1]
>> +If set to 1, all inodes created by
>> .B mkfs.xfs
>> will be created with the realtime flag set.
>> Directories will pass on this flag to newly created regular files and
>> directories.
>> .TP
>> -.BI projinherit= value
>> +.BI projinherit= projid
>> All inodes created by
>> .B mkfs.xfs
>> will be assigned this project quota id.
>> Directories will pass on the project id to newly created regular files and
>> directories.
>> .TP
>> -.BI extszinherit= value
>> +.BI extszinherit= extentsize
>
> Hmm... if you're going to make this change to extszinherit, you might as
> well do the same for cowextsize.
Well, "inherit=" /sounds/ like a boolean, so I figured that one needed clarification,
whereas "size=" seems pretty clear to me, and is described already?
But perhaps it would be more consistent to keep it all as "=value" and then just
more clearly describe the valid *inherit= values in the explanatory text... let me try
a V2 ;)
> With that fixed,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> --D
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1.5/5] mkfs: clarify valid "inherit" option values
2020-10-27 17:49 ` Eric Sandeen
@ 2020-10-28 7:32 ` Christoph Hellwig
0 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-10-28 7:32 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Darrick J. Wong, linux-xfs
On Tue, Oct 27, 2020 at 12:49:08PM -0500, Eric Sandeen wrote:
> Well, "inherit=" /sounds/ like a boolean, so I figured that one needed clarification,
> whereas "size=" seems pretty clear to me, and is described already?
>
> But perhaps it would be more consistent to keep it all as "=value" and then just
> more clearly describe the valid *inherit= values in the explanatory text... let me try
> a V2 ;)
Yes, that sounds pretty sensible.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1.5/5 V2] mkfs: clarify valid "inherit" option values
2020-10-27 17:24 ` [PATCH 1.5/5] mkfs: clarify valid "inherit" option values Eric Sandeen
2020-10-27 17:40 ` Darrick J. Wong
@ 2020-10-27 17:50 ` Eric Sandeen
2020-10-27 18:37 ` Darrick J. Wong
1 sibling, 1 reply; 26+ messages in thread
From: Eric Sandeen @ 2020-10-27 17:50 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
Clarify which values are valid for the various *inherit= mkfs
options.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: Keep the same "=value" as elsewhere in the manpage but still clarify
what those values should be and what they do.
diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 0a785874..10ceea30 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -378,31 +378,38 @@ without stripe geometry alignment even if the underlying storage device provides
this information.
.TP
.BI rtinherit= value
-If set, all inodes created by
+If
+.I value
+is set to 1, all inodes created by
.B mkfs.xfs
-will be created with the realtime flag set.
+will be created with the realtime flag set. Default is 0.
Directories will pass on this flag to newly created regular files and
directories.
.TP
.BI projinherit= value
All inodes created by
.B mkfs.xfs
-will be assigned this project quota id.
+will be assigned the project quota id provided in
+.I value.
Directories will pass on the project id to newly created regular files and
directories.
.TP
.BI extszinherit= value
All inodes created by
.B mkfs.xfs
-will have this extent size hint applied.
+will have this
+.I value
+extent size hint applied.
The value must be provided in units of filesystem blocks.
Directories will pass on this hint to newly created regular files and
directories.
.TP
.BI daxinherit= value
-If set, all inodes created by
+If
+.I value
+is set to 1, all inodes created by
.B mkfs.xfs
-will be created with the DAX flag set.
+will be created with the DAX flag set. Default is 0.
Directories will pass on this flag to newly created regular files and
directories.
By default,
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1.5/5 V2] mkfs: clarify valid "inherit" option values
2020-10-27 17:50 ` [PATCH 1.5/5 V2] " Eric Sandeen
@ 2020-10-27 18:37 ` Darrick J. Wong
0 siblings, 0 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-10-27 18:37 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Tue, Oct 27, 2020 at 12:50:43PM -0500, Eric Sandeen wrote:
> Clarify which values are valid for the various *inherit= mkfs
> options.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> V2: Keep the same "=value" as elsewhere in the manpage but still clarify
> what those values should be and what they do.
>
>
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index 0a785874..10ceea30 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -378,31 +378,38 @@ without stripe geometry alignment even if the underlying storage device provides
> this information.
> .TP
> .BI rtinherit= value
> -If set, all inodes created by
> +If
> +.I value
> +is set to 1, all inodes created by
> .B mkfs.xfs
> -will be created with the realtime flag set.
> +will be created with the realtime flag set. Default is 0.
Nit: Start new sentences on a new line.
> Directories will pass on this flag to newly created regular files and
> directories.
> .TP
> .BI projinherit= value
> All inodes created by
> .B mkfs.xfs
> -will be assigned this project quota id.
> +will be assigned the project quota id provided in
Nit: Blank space at end of line.
> +.I value.
> Directories will pass on the project id to newly created regular files and
> directories.
> .TP
> .BI extszinherit= value
> All inodes created by
> .B mkfs.xfs
> -will have this extent size hint applied.
> +will have this
> +.I value
> +extent size hint applied.
> The value must be provided in units of filesystem blocks.
> Directories will pass on this hint to newly created regular files and
> directories.
> .TP
> .BI daxinherit= value
> -If set, all inodes created by
> +If
> +.I value
> +is set to 1, all inodes created by
> .B mkfs.xfs
> -will be created with the DAX flag set.
> +will be created with the DAX flag set. Default is 0.
The first nit again...
With /that/ fixed...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> Directories will pass on this flag to newly created regular files and
> directories.
> By default,
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] mkfs: allow users to specify rtinherit=0
2020-10-26 23:31 ` [PATCH 1/5] mkfs: allow users to specify rtinherit=0 Darrick J. Wong
` (2 preceding siblings ...)
2020-10-27 17:24 ` [PATCH 1.5/5] mkfs: clarify valid "inherit" option values Eric Sandeen
@ 2020-10-28 7:32 ` Christoph Hellwig
3 siblings, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-10-28 7:32 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks
2020-10-26 23:31 [PATCH 0/5] xfsprogs: fixes for 5.10 Darrick J. Wong
2020-10-26 23:31 ` [PATCH 1/5] mkfs: allow users to specify rtinherit=0 Darrick J. Wong
@ 2020-10-26 23:32 ` Darrick J. Wong
2020-10-27 5:35 ` Allison Henderson
2020-10-28 7:33 ` Christoph Hellwig
2020-10-26 23:32 ` [PATCH 3/5] xfs_db: report ranges of invalid rt blocks Darrick J. Wong
` (2 subsequent siblings)
4 siblings, 2 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-10-26 23:32 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
The only caller that cares about the file counts uses it to compute the
number of files used, so return that and save a parameter.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
scrub/fscounters.c | 8 +++-----
scrub/fscounters.h | 2 +-
scrub/phase6.c | 7 +++----
scrub/phase7.c | 5 +----
4 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/scrub/fscounters.c b/scrub/fscounters.c
index e9901fcdf6df..9a240d49477b 100644
--- a/scrub/fscounters.c
+++ b/scrub/fscounters.c
@@ -116,7 +116,7 @@ scrub_count_all_inodes(
}
/*
- * Estimate the number of blocks and inodes in the filesystem. Returns 0
+ * Estimate the number of blocks and used inodes in the filesystem. Returns 0
* or a positive error number.
*/
int
@@ -126,8 +126,7 @@ scrub_scan_estimate_blocks(
unsigned long long *d_bfree,
unsigned long long *r_blocks,
unsigned long long *r_bfree,
- unsigned long long *f_files,
- unsigned long long *f_free)
+ unsigned long long *f_files_used)
{
struct xfs_fsop_counts fc;
int error;
@@ -141,8 +140,7 @@ scrub_scan_estimate_blocks(
*d_bfree = fc.freedata;
*r_blocks = ctx->mnt.fsgeom.rtblocks;
*r_bfree = fc.freertx;
- *f_files = fc.allocino;
- *f_free = fc.freeino;
+ *f_files_used = fc.allocino - fc.freeino;
return 0;
}
diff --git a/scrub/fscounters.h b/scrub/fscounters.h
index 1fae58a6b287..13bd9967f004 100644
--- a/scrub/fscounters.h
+++ b/scrub/fscounters.h
@@ -9,7 +9,7 @@
int scrub_scan_estimate_blocks(struct scrub_ctx *ctx,
unsigned long long *d_blocks, unsigned long long *d_bfree,
unsigned long long *r_blocks, unsigned long long *r_bfree,
- unsigned long long *f_files, unsigned long long *f_free);
+ unsigned long long *f_files_used);
int scrub_count_all_inodes(struct scrub_ctx *ctx, uint64_t *count);
#endif /* XFS_SCRUB_FSCOUNTERS_H_ */
diff --git a/scrub/phase6.c b/scrub/phase6.c
index 8d976732d8e1..87828b60fbed 100644
--- a/scrub/phase6.c
+++ b/scrub/phase6.c
@@ -719,12 +719,11 @@ phase6_estimate(
unsigned long long d_bfree;
unsigned long long r_blocks;
unsigned long long r_bfree;
- unsigned long long f_files;
- unsigned long long f_free;
+ unsigned long long dontcare;
int ret;
- ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree,
- &r_blocks, &r_bfree, &f_files, &f_free);
+ ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
+ &r_bfree, &dontcare);
if (ret) {
str_liberror(ctx, ret, _("estimating verify work"));
return ret;
diff --git a/scrub/phase7.c b/scrub/phase7.c
index 96876f7c0596..bc652ab6f44a 100644
--- a/scrub/phase7.c
+++ b/scrub/phase7.c
@@ -111,8 +111,6 @@ phase7_func(
unsigned long long d_bfree;
unsigned long long r_blocks;
unsigned long long r_bfree;
- unsigned long long f_files;
- unsigned long long f_free;
bool complain;
int ip;
int error;
@@ -160,7 +158,7 @@ phase7_func(
}
error = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
- &r_bfree, &f_files, &f_free);
+ &r_bfree, &used_files);
if (error) {
str_liberror(ctx, error, _("estimating verify work"));
return error;
@@ -177,7 +175,6 @@ phase7_func(
/* Report on what we found. */
used_data = cvt_off_fsb_to_b(&ctx->mnt, d_blocks - d_bfree);
used_rt = cvt_off_fsb_to_b(&ctx->mnt, r_blocks - r_bfree);
- used_files = f_files - f_free;
stat_data = totalcount.dbytes;
stat_rt = totalcount.rbytes;
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks
2020-10-26 23:32 ` [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks Darrick J. Wong
@ 2020-10-27 5:35 ` Allison Henderson
2020-10-27 15:33 ` Darrick J. Wong
2020-10-28 7:33 ` Christoph Hellwig
1 sibling, 1 reply; 26+ messages in thread
From: Allison Henderson @ 2020-10-27 5:35 UTC (permalink / raw)
To: Darrick J. Wong, sandeen; +Cc: linux-xfs
On 10/26/20 4:32 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> The only caller that cares about the file counts uses it to compute the
> number of files used, so return that and save a parameter.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> scrub/fscounters.c | 8 +++-----
> scrub/fscounters.h | 2 +-
> scrub/phase6.c | 7 +++----
> scrub/phase7.c | 5 +----
> 4 files changed, 8 insertions(+), 14 deletions(-)
>
>
> diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> index e9901fcdf6df..9a240d49477b 100644
> --- a/scrub/fscounters.c
> +++ b/scrub/fscounters.c
> @@ -116,7 +116,7 @@ scrub_count_all_inodes(
> }
>
> /*
> - * Estimate the number of blocks and inodes in the filesystem. Returns 0
> + * Estimate the number of blocks and used inodes in the filesystem. Returns 0
> * or a positive error number.
> */
> int
> @@ -126,8 +126,7 @@ scrub_scan_estimate_blocks(
> unsigned long long *d_bfree,
> unsigned long long *r_blocks,
> unsigned long long *r_bfree,
> - unsigned long long *f_files,
> - unsigned long long *f_free)
> + unsigned long long *f_files_used)
> {
> struct xfs_fsop_counts fc;
> int error;
> @@ -141,8 +140,7 @@ scrub_scan_estimate_blocks(
> *d_bfree = fc.freedata;
> *r_blocks = ctx->mnt.fsgeom.rtblocks;
> *r_bfree = fc.freertx;
> - *f_files = fc.allocino;
> - *f_free = fc.freeino;
> + *f_files_used = fc.allocino - fc.freeino;
Just a nit, I think I might have put in:
if(f_files_used)
*f_files_used = fc.allocino - fc.freeino;
That way calling functions that don't care can just pass NULL, instead
of declaring a "dontcare" variable that has no other use. Though I
suppose none of the other variables do that.
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Allison
>
> return 0;
> }
> diff --git a/scrub/fscounters.h b/scrub/fscounters.h
> index 1fae58a6b287..13bd9967f004 100644
> --- a/scrub/fscounters.h
> +++ b/scrub/fscounters.h
> @@ -9,7 +9,7 @@
> int scrub_scan_estimate_blocks(struct scrub_ctx *ctx,
> unsigned long long *d_blocks, unsigned long long *d_bfree,
> unsigned long long *r_blocks, unsigned long long *r_bfree,
> - unsigned long long *f_files, unsigned long long *f_free);
> + unsigned long long *f_files_used);
> int scrub_count_all_inodes(struct scrub_ctx *ctx, uint64_t *count);
>
> #endif /* XFS_SCRUB_FSCOUNTERS_H_ */
> diff --git a/scrub/phase6.c b/scrub/phase6.c
> index 8d976732d8e1..87828b60fbed 100644
> --- a/scrub/phase6.c
> +++ b/scrub/phase6.c
> @@ -719,12 +719,11 @@ phase6_estimate(
> unsigned long long d_bfree;
> unsigned long long r_blocks;
> unsigned long long r_bfree;
> - unsigned long long f_files;
> - unsigned long long f_free;
> + unsigned long long dontcare;
> int ret;
>
> - ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree,
> - &r_blocks, &r_bfree, &f_files, &f_free);
> + ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
> + &r_bfree, &dontcare);
> if (ret) {
> str_liberror(ctx, ret, _("estimating verify work"));
> return ret;
> diff --git a/scrub/phase7.c b/scrub/phase7.c
> index 96876f7c0596..bc652ab6f44a 100644
> --- a/scrub/phase7.c
> +++ b/scrub/phase7.c
> @@ -111,8 +111,6 @@ phase7_func(
> unsigned long long d_bfree;
> unsigned long long r_blocks;
> unsigned long long r_bfree;
> - unsigned long long f_files;
> - unsigned long long f_free;
> bool complain;
> int ip;
> int error;
> @@ -160,7 +158,7 @@ phase7_func(
> }
>
> error = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
> - &r_bfree, &f_files, &f_free);
> + &r_bfree, &used_files);
> if (error) {
> str_liberror(ctx, error, _("estimating verify work"));
> return error;
> @@ -177,7 +175,6 @@ phase7_func(
> /* Report on what we found. */
> used_data = cvt_off_fsb_to_b(&ctx->mnt, d_blocks - d_bfree);
> used_rt = cvt_off_fsb_to_b(&ctx->mnt, r_blocks - r_bfree);
> - used_files = f_files - f_free;
> stat_data = totalcount.dbytes;
> stat_rt = totalcount.rbytes;
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks
2020-10-27 5:35 ` Allison Henderson
@ 2020-10-27 15:33 ` Darrick J. Wong
2020-10-29 18:33 ` Darrick J. Wong
0 siblings, 1 reply; 26+ messages in thread
From: Darrick J. Wong @ 2020-10-27 15:33 UTC (permalink / raw)
To: Allison Henderson; +Cc: sandeen, linux-xfs
On Mon, Oct 26, 2020 at 10:35:46PM -0700, Allison Henderson wrote:
>
>
> On 10/26/20 4:32 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > The only caller that cares about the file counts uses it to compute the
> > number of files used, so return that and save a parameter.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > scrub/fscounters.c | 8 +++-----
> > scrub/fscounters.h | 2 +-
> > scrub/phase6.c | 7 +++----
> > scrub/phase7.c | 5 +----
> > 4 files changed, 8 insertions(+), 14 deletions(-)
> >
> >
> > diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> > index e9901fcdf6df..9a240d49477b 100644
> > --- a/scrub/fscounters.c
> > +++ b/scrub/fscounters.c
> > @@ -116,7 +116,7 @@ scrub_count_all_inodes(
> > }
> > /*
> > - * Estimate the number of blocks and inodes in the filesystem. Returns 0
> > + * Estimate the number of blocks and used inodes in the filesystem. Returns 0
> > * or a positive error number.
> > */
> > int
> > @@ -126,8 +126,7 @@ scrub_scan_estimate_blocks(
> > unsigned long long *d_bfree,
> > unsigned long long *r_blocks,
> > unsigned long long *r_bfree,
> > - unsigned long long *f_files,
> > - unsigned long long *f_free)
> > + unsigned long long *f_files_used)
> > {
> > struct xfs_fsop_counts fc;
> > int error;
> > @@ -141,8 +140,7 @@ scrub_scan_estimate_blocks(
> > *d_bfree = fc.freedata;
> > *r_blocks = ctx->mnt.fsgeom.rtblocks;
> > *r_bfree = fc.freertx;
> > - *f_files = fc.allocino;
> > - *f_free = fc.freeino;
> > + *f_files_used = fc.allocino - fc.freeino;
> Just a nit, I think I might have put in:
> if(f_files_used)
> *f_files_used = fc.allocino - fc.freeino;
>
> That way calling functions that don't care can just pass NULL, instead of
> declaring a "dontcare" variable that has no other use. Though I suppose
> none of the other variables do that.
<shrug> There's only two callers, and they both pass a pointer, so I
didn't bother...
--D
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> Allison
> > return 0;
> > }
> > diff --git a/scrub/fscounters.h b/scrub/fscounters.h
> > index 1fae58a6b287..13bd9967f004 100644
> > --- a/scrub/fscounters.h
> > +++ b/scrub/fscounters.h
> > @@ -9,7 +9,7 @@
> > int scrub_scan_estimate_blocks(struct scrub_ctx *ctx,
> > unsigned long long *d_blocks, unsigned long long *d_bfree,
> > unsigned long long *r_blocks, unsigned long long *r_bfree,
> > - unsigned long long *f_files, unsigned long long *f_free);
> > + unsigned long long *f_files_used);
> > int scrub_count_all_inodes(struct scrub_ctx *ctx, uint64_t *count);
> > #endif /* XFS_SCRUB_FSCOUNTERS_H_ */
> > diff --git a/scrub/phase6.c b/scrub/phase6.c
> > index 8d976732d8e1..87828b60fbed 100644
> > --- a/scrub/phase6.c
> > +++ b/scrub/phase6.c
> > @@ -719,12 +719,11 @@ phase6_estimate(
> > unsigned long long d_bfree;
> > unsigned long long r_blocks;
> > unsigned long long r_bfree;
> > - unsigned long long f_files;
> > - unsigned long long f_free;
> > + unsigned long long dontcare;
> > int ret;
> > - ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree,
> > - &r_blocks, &r_bfree, &f_files, &f_free);
> > + ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
> > + &r_bfree, &dontcare);
> > if (ret) {
> > str_liberror(ctx, ret, _("estimating verify work"));
> > return ret;
> > diff --git a/scrub/phase7.c b/scrub/phase7.c
> > index 96876f7c0596..bc652ab6f44a 100644
> > --- a/scrub/phase7.c
> > +++ b/scrub/phase7.c
> > @@ -111,8 +111,6 @@ phase7_func(
> > unsigned long long d_bfree;
> > unsigned long long r_blocks;
> > unsigned long long r_bfree;
> > - unsigned long long f_files;
> > - unsigned long long f_free;
> > bool complain;
> > int ip;
> > int error;
> > @@ -160,7 +158,7 @@ phase7_func(
> > }
> > error = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
> > - &r_bfree, &f_files, &f_free);
> > + &r_bfree, &used_files);
> > if (error) {
> > str_liberror(ctx, error, _("estimating verify work"));
> > return error;
> > @@ -177,7 +175,6 @@ phase7_func(
> > /* Report on what we found. */
> > used_data = cvt_off_fsb_to_b(&ctx->mnt, d_blocks - d_bfree);
> > used_rt = cvt_off_fsb_to_b(&ctx->mnt, r_blocks - r_bfree);
> > - used_files = f_files - f_free;
> > stat_data = totalcount.dbytes;
> > stat_rt = totalcount.rbytes;
> >
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks
2020-10-27 15:33 ` Darrick J. Wong
@ 2020-10-29 18:33 ` Darrick J. Wong
2020-10-29 18:38 ` Allison Henderson
0 siblings, 1 reply; 26+ messages in thread
From: Darrick J. Wong @ 2020-10-29 18:33 UTC (permalink / raw)
To: Allison Henderson; +Cc: sandeen, linux-xfs
On Tue, Oct 27, 2020 at 08:33:50AM -0700, Darrick J. Wong wrote:
> On Mon, Oct 26, 2020 at 10:35:46PM -0700, Allison Henderson wrote:
> >
> >
> > On 10/26/20 4:32 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > The only caller that cares about the file counts uses it to compute the
> > > number of files used, so return that and save a parameter.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > > scrub/fscounters.c | 8 +++-----
> > > scrub/fscounters.h | 2 +-
> > > scrub/phase6.c | 7 +++----
> > > scrub/phase7.c | 5 +----
> > > 4 files changed, 8 insertions(+), 14 deletions(-)
> > >
> > >
> > > diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> > > index e9901fcdf6df..9a240d49477b 100644
> > > --- a/scrub/fscounters.c
> > > +++ b/scrub/fscounters.c
> > > @@ -116,7 +116,7 @@ scrub_count_all_inodes(
> > > }
> > > /*
> > > - * Estimate the number of blocks and inodes in the filesystem. Returns 0
> > > + * Estimate the number of blocks and used inodes in the filesystem. Returns 0
> > > * or a positive error number.
> > > */
> > > int
> > > @@ -126,8 +126,7 @@ scrub_scan_estimate_blocks(
> > > unsigned long long *d_bfree,
> > > unsigned long long *r_blocks,
> > > unsigned long long *r_bfree,
> > > - unsigned long long *f_files,
> > > - unsigned long long *f_free)
> > > + unsigned long long *f_files_used)
> > > {
> > > struct xfs_fsop_counts fc;
> > > int error;
> > > @@ -141,8 +140,7 @@ scrub_scan_estimate_blocks(
> > > *d_bfree = fc.freedata;
> > > *r_blocks = ctx->mnt.fsgeom.rtblocks;
> > > *r_bfree = fc.freertx;
> > > - *f_files = fc.allocino;
> > > - *f_free = fc.freeino;
> > > + *f_files_used = fc.allocino - fc.freeino;
> > Just a nit, I think I might have put in:
> > if(f_files_used)
> > *f_files_used = fc.allocino - fc.freeino;
> >
> > That way calling functions that don't care can just pass NULL, instead of
> > declaring a "dontcare" variable that has no other use. Though I suppose
> > none of the other variables do that.
>
> <shrug> There's only two callers, and they both pass a pointer, so I
> didn't bother...
...and one of those callers is to a dontcare variable; and that's what
you were talking about.
Er... I don't mind changing it, but I'll leave that to Eric's
discretion.
--D
> --D
>
> > Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> > Allison
> > > return 0;
> > > }
> > > diff --git a/scrub/fscounters.h b/scrub/fscounters.h
> > > index 1fae58a6b287..13bd9967f004 100644
> > > --- a/scrub/fscounters.h
> > > +++ b/scrub/fscounters.h
> > > @@ -9,7 +9,7 @@
> > > int scrub_scan_estimate_blocks(struct scrub_ctx *ctx,
> > > unsigned long long *d_blocks, unsigned long long *d_bfree,
> > > unsigned long long *r_blocks, unsigned long long *r_bfree,
> > > - unsigned long long *f_files, unsigned long long *f_free);
> > > + unsigned long long *f_files_used);
> > > int scrub_count_all_inodes(struct scrub_ctx *ctx, uint64_t *count);
> > > #endif /* XFS_SCRUB_FSCOUNTERS_H_ */
> > > diff --git a/scrub/phase6.c b/scrub/phase6.c
> > > index 8d976732d8e1..87828b60fbed 100644
> > > --- a/scrub/phase6.c
> > > +++ b/scrub/phase6.c
> > > @@ -719,12 +719,11 @@ phase6_estimate(
> > > unsigned long long d_bfree;
> > > unsigned long long r_blocks;
> > > unsigned long long r_bfree;
> > > - unsigned long long f_files;
> > > - unsigned long long f_free;
> > > + unsigned long long dontcare;
> > > int ret;
> > > - ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree,
> > > - &r_blocks, &r_bfree, &f_files, &f_free);
> > > + ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
> > > + &r_bfree, &dontcare);
> > > if (ret) {
> > > str_liberror(ctx, ret, _("estimating verify work"));
> > > return ret;
> > > diff --git a/scrub/phase7.c b/scrub/phase7.c
> > > index 96876f7c0596..bc652ab6f44a 100644
> > > --- a/scrub/phase7.c
> > > +++ b/scrub/phase7.c
> > > @@ -111,8 +111,6 @@ phase7_func(
> > > unsigned long long d_bfree;
> > > unsigned long long r_blocks;
> > > unsigned long long r_bfree;
> > > - unsigned long long f_files;
> > > - unsigned long long f_free;
> > > bool complain;
> > > int ip;
> > > int error;
> > > @@ -160,7 +158,7 @@ phase7_func(
> > > }
> > > error = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
> > > - &r_bfree, &f_files, &f_free);
> > > + &r_bfree, &used_files);
> > > if (error) {
> > > str_liberror(ctx, error, _("estimating verify work"));
> > > return error;
> > > @@ -177,7 +175,6 @@ phase7_func(
> > > /* Report on what we found. */
> > > used_data = cvt_off_fsb_to_b(&ctx->mnt, d_blocks - d_bfree);
> > > used_rt = cvt_off_fsb_to_b(&ctx->mnt, r_blocks - r_bfree);
> > > - used_files = f_files - f_free;
> > > stat_data = totalcount.dbytes;
> > > stat_rt = totalcount.rbytes;
> > >
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks
2020-10-29 18:33 ` Darrick J. Wong
@ 2020-10-29 18:38 ` Allison Henderson
0 siblings, 0 replies; 26+ messages in thread
From: Allison Henderson @ 2020-10-29 18:38 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
On 10/29/20 11:33 AM, Darrick J. Wong wrote:
> On Tue, Oct 27, 2020 at 08:33:50AM -0700, Darrick J. Wong wrote:
>> On Mon, Oct 26, 2020 at 10:35:46PM -0700, Allison Henderson wrote:
>>>
>>>
>>> On 10/26/20 4:32 PM, Darrick J. Wong wrote:
>>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>>
>>>> The only caller that cares about the file counts uses it to compute the
>>>> number of files used, so return that and save a parameter.
>>>>
>>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>>> ---
>>>> scrub/fscounters.c | 8 +++-----
>>>> scrub/fscounters.h | 2 +-
>>>> scrub/phase6.c | 7 +++----
>>>> scrub/phase7.c | 5 +----
>>>> 4 files changed, 8 insertions(+), 14 deletions(-)
>>>>
>>>>
>>>> diff --git a/scrub/fscounters.c b/scrub/fscounters.c
>>>> index e9901fcdf6df..9a240d49477b 100644
>>>> --- a/scrub/fscounters.c
>>>> +++ b/scrub/fscounters.c
>>>> @@ -116,7 +116,7 @@ scrub_count_all_inodes(
>>>> }
>>>> /*
>>>> - * Estimate the number of blocks and inodes in the filesystem. Returns 0
>>>> + * Estimate the number of blocks and used inodes in the filesystem. Returns 0
>>>> * or a positive error number.
>>>> */
>>>> int
>>>> @@ -126,8 +126,7 @@ scrub_scan_estimate_blocks(
>>>> unsigned long long *d_bfree,
>>>> unsigned long long *r_blocks,
>>>> unsigned long long *r_bfree,
>>>> - unsigned long long *f_files,
>>>> - unsigned long long *f_free)
>>>> + unsigned long long *f_files_used)
>>>> {
>>>> struct xfs_fsop_counts fc;
>>>> int error;
>>>> @@ -141,8 +140,7 @@ scrub_scan_estimate_blocks(
>>>> *d_bfree = fc.freedata;
>>>> *r_blocks = ctx->mnt.fsgeom.rtblocks;
>>>> *r_bfree = fc.freertx;
>>>> - *f_files = fc.allocino;
>>>> - *f_free = fc.freeino;
>>>> + *f_files_used = fc.allocino - fc.freeino;
>>> Just a nit, I think I might have put in:
>>> if(f_files_used)
>>> *f_files_used = fc.allocino - fc.freeino;
>>>
>>> That way calling functions that don't care can just pass NULL, instead of
>>> declaring a "dontcare" variable that has no other use. Though I suppose
>>> none of the other variables do that.
>>
>> <shrug> There's only two callers, and they both pass a pointer, so I
>> didn't bother...
>
> ...and one of those callers is to a dontcare variable; and that's what
> you were talking about.
>
> Er... I don't mind changing it, but I'll leave that to Eric's
> discretion.
>
> --D
No worries, it's not a huge detail, I just thought I'd throw the
suggestion out is all :-)
Allison
>
>> --D
>>
>>> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
>>> Allison
>>>> return 0;
>>>> }
>>>> diff --git a/scrub/fscounters.h b/scrub/fscounters.h
>>>> index 1fae58a6b287..13bd9967f004 100644
>>>> --- a/scrub/fscounters.h
>>>> +++ b/scrub/fscounters.h
>>>> @@ -9,7 +9,7 @@
>>>> int scrub_scan_estimate_blocks(struct scrub_ctx *ctx,
>>>> unsigned long long *d_blocks, unsigned long long *d_bfree,
>>>> unsigned long long *r_blocks, unsigned long long *r_bfree,
>>>> - unsigned long long *f_files, unsigned long long *f_free);
>>>> + unsigned long long *f_files_used);
>>>> int scrub_count_all_inodes(struct scrub_ctx *ctx, uint64_t *count);
>>>> #endif /* XFS_SCRUB_FSCOUNTERS_H_ */
>>>> diff --git a/scrub/phase6.c b/scrub/phase6.c
>>>> index 8d976732d8e1..87828b60fbed 100644
>>>> --- a/scrub/phase6.c
>>>> +++ b/scrub/phase6.c
>>>> @@ -719,12 +719,11 @@ phase6_estimate(
>>>> unsigned long long d_bfree;
>>>> unsigned long long r_blocks;
>>>> unsigned long long r_bfree;
>>>> - unsigned long long f_files;
>>>> - unsigned long long f_free;
>>>> + unsigned long long dontcare;
>>>> int ret;
>>>> - ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree,
>>>> - &r_blocks, &r_bfree, &f_files, &f_free);
>>>> + ret = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
>>>> + &r_bfree, &dontcare);
>>>> if (ret) {
>>>> str_liberror(ctx, ret, _("estimating verify work"));
>>>> return ret;
>>>> diff --git a/scrub/phase7.c b/scrub/phase7.c
>>>> index 96876f7c0596..bc652ab6f44a 100644
>>>> --- a/scrub/phase7.c
>>>> +++ b/scrub/phase7.c
>>>> @@ -111,8 +111,6 @@ phase7_func(
>>>> unsigned long long d_bfree;
>>>> unsigned long long r_blocks;
>>>> unsigned long long r_bfree;
>>>> - unsigned long long f_files;
>>>> - unsigned long long f_free;
>>>> bool complain;
>>>> int ip;
>>>> int error;
>>>> @@ -160,7 +158,7 @@ phase7_func(
>>>> }
>>>> error = scrub_scan_estimate_blocks(ctx, &d_blocks, &d_bfree, &r_blocks,
>>>> - &r_bfree, &f_files, &f_free);
>>>> + &r_bfree, &used_files);
>>>> if (error) {
>>>> str_liberror(ctx, error, _("estimating verify work"));
>>>> return error;
>>>> @@ -177,7 +175,6 @@ phase7_func(
>>>> /* Report on what we found. */
>>>> used_data = cvt_off_fsb_to_b(&ctx->mnt, d_blocks - d_bfree);
>>>> used_rt = cvt_off_fsb_to_b(&ctx->mnt, r_blocks - r_bfree);
>>>> - used_files = f_files - f_free;
>>>> stat_data = totalcount.dbytes;
>>>> stat_rt = totalcount.rbytes;
>>>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks
2020-10-26 23:32 ` [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks Darrick J. Wong
2020-10-27 5:35 ` Allison Henderson
@ 2020-10-28 7:33 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-10-28 7:33 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
On Mon, Oct 26, 2020 at 04:32:05PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> The only caller that cares about the file counts uses it to compute the
> number of files used, so return that and save a parameter.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/5] xfs_db: report ranges of invalid rt blocks
2020-10-26 23:31 [PATCH 0/5] xfsprogs: fixes for 5.10 Darrick J. Wong
2020-10-26 23:31 ` [PATCH 1/5] mkfs: allow users to specify rtinherit=0 Darrick J. Wong
2020-10-26 23:32 ` [PATCH 2/5] xfs: remove unnecessary parameter from scrub_scan_estimate_blocks Darrick J. Wong
@ 2020-10-26 23:32 ` Darrick J. Wong
2020-10-27 5:35 ` Allison Henderson
2020-10-28 7:33 ` Christoph Hellwig
2020-10-26 23:32 ` [PATCH 4/5] xfs_repair: skip the rmap and refcount btree checks when the levels are garbage Darrick J. Wong
2020-10-26 23:32 ` [PATCH 5/5] xfs_repair: correctly detect partially written extents Darrick J. Wong
4 siblings, 2 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-10-26 23:32 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
Copy-pasta the block range reporting code from check_range into
check_rrange so that we don't flood stdout with a ton of low value
messages when a bit flips somewhere in rt metadata.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
db/check.c | 33 ++++++++++++++++++++++++++++++---
1 file changed, 30 insertions(+), 3 deletions(-)
diff --git a/db/check.c b/db/check.c
index 553249dc9a41..5aede6cca15c 100644
--- a/db/check.c
+++ b/db/check.c
@@ -1569,19 +1569,46 @@ check_rootdir(void)
}
}
+static inline void
+report_rrange(
+ xfs_rfsblock_t low,
+ xfs_rfsblock_t high)
+{
+ if (low == high)
+ dbprintf(_("rtblock %llu out of range\n"), low);
+ else
+ dbprintf(_("rtblocks %llu..%llu out of range\n"), low, high);
+}
+
static int
check_rrange(
xfs_rfsblock_t bno,
xfs_extlen_t len)
{
xfs_extlen_t i;
+ xfs_rfsblock_t low = 0;
+ xfs_rfsblock_t high = 0;
+ bool valid_range = false;
+ int cur, prev = 0;
if (bno + len - 1 >= mp->m_sb.sb_rblocks) {
for (i = 0; i < len; i++) {
- if (!sflag || CHECK_BLIST(bno + i))
- dbprintf(_("rtblock %llu out of range\n"),
- bno + i);
+ cur = !sflag || CHECK_BLIST(bno + i) ? 1 : 0;
+ if (cur == 1 && prev == 0) {
+ low = high = bno + i;
+ valid_range = true;
+ } else if (cur == 0 && prev == 0) {
+ /* Do nothing */
+ } else if (cur == 0 && prev == 1) {
+ report_rrange(low, high);
+ valid_range = false;
+ } else if (cur == 1 && prev == 1) {
+ high = bno + i;
+ }
+ prev = cur;
}
+ if (valid_range)
+ report_rrange(low, high);
error++;
return 0;
}
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 3/5] xfs_db: report ranges of invalid rt blocks
2020-10-26 23:32 ` [PATCH 3/5] xfs_db: report ranges of invalid rt blocks Darrick J. Wong
@ 2020-10-27 5:35 ` Allison Henderson
2020-10-28 7:33 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Allison Henderson @ 2020-10-27 5:35 UTC (permalink / raw)
To: Darrick J. Wong, sandeen; +Cc: linux-xfs
On 10/26/20 4:32 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Copy-pasta the block range reporting code from check_range into
> check_rrange so that we don't flood stdout with a ton of low value
> messages when a bit flips somewhere in rt metadata.
>
Ok, makes sense
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> db/check.c | 33 ++++++++++++++++++++++++++++++---
> 1 file changed, 30 insertions(+), 3 deletions(-)
>
>
> diff --git a/db/check.c b/db/check.c
> index 553249dc9a41..5aede6cca15c 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -1569,19 +1569,46 @@ check_rootdir(void)
> }
> }
>
> +static inline void
> +report_rrange(
> + xfs_rfsblock_t low,
> + xfs_rfsblock_t high)
> +{
> + if (low == high)
> + dbprintf(_("rtblock %llu out of range\n"), low);
> + else
> + dbprintf(_("rtblocks %llu..%llu out of range\n"), low, high);
> +}
> +
> static int
> check_rrange(
> xfs_rfsblock_t bno,
> xfs_extlen_t len)
> {
> xfs_extlen_t i;
> + xfs_rfsblock_t low = 0;
> + xfs_rfsblock_t high = 0;
> + bool valid_range = false;
> + int cur, prev = 0;
>
> if (bno + len - 1 >= mp->m_sb.sb_rblocks) {
> for (i = 0; i < len; i++) {
> - if (!sflag || CHECK_BLIST(bno + i))
> - dbprintf(_("rtblock %llu out of range\n"),
> - bno + i);
> + cur = !sflag || CHECK_BLIST(bno + i) ? 1 : 0;
> + if (cur == 1 && prev == 0) {
> + low = high = bno + i;
> + valid_range = true;
> + } else if (cur == 0 && prev == 0) {
> + /* Do nothing */
> + } else if (cur == 0 && prev == 1) {
> + report_rrange(low, high);
> + valid_range = false;
> + } else if (cur == 1 && prev == 1) {
> + high = bno + i;
> + }
> + prev = cur;
> }
> + if (valid_range)
> + report_rrange(low, high);
> error++;
> return 0;
> }
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 3/5] xfs_db: report ranges of invalid rt blocks
2020-10-26 23:32 ` [PATCH 3/5] xfs_db: report ranges of invalid rt blocks Darrick J. Wong
2020-10-27 5:35 ` Allison Henderson
@ 2020-10-28 7:33 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-10-28 7:33 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
On Mon, Oct 26, 2020 at 04:32:12PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Copy-pasta the block range reporting code from check_range into
> check_rrange so that we don't flood stdout with a ton of low value
> messages when a bit flips somewhere in rt metadata.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/5] xfs_repair: skip the rmap and refcount btree checks when the levels are garbage
2020-10-26 23:31 [PATCH 0/5] xfsprogs: fixes for 5.10 Darrick J. Wong
` (2 preceding siblings ...)
2020-10-26 23:32 ` [PATCH 3/5] xfs_db: report ranges of invalid rt blocks Darrick J. Wong
@ 2020-10-26 23:32 ` Darrick J. Wong
2020-10-27 5:35 ` Allison Henderson
2020-10-28 7:34 ` Christoph Hellwig
2020-10-26 23:32 ` [PATCH 5/5] xfs_repair: correctly detect partially written extents Darrick J. Wong
4 siblings, 2 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-10-26 23:32 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
In validate_ag[fi], we should check that the levels of the rmap and
refcount btrees are valid. If they aren't, we need to tell phase4 to
skip the comparison between the existing and incore rmap and refcount
data. The comparison routines use libxfs btree cursors, which assume
that the caller validated bc_nlevels and will corrupt memory if we load
a btree cursor with a garbage level count.
This was found by examing a core dump from a failed xfs/086 invocation.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
repair/scan.c | 36 ++++++++++++++++++++++++++----------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/repair/scan.c b/repair/scan.c
index 42b299f75067..2a38ae5197c6 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -2279,23 +2279,31 @@ validate_agf(
if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
struct rmap_priv priv;
+ unsigned int levels;
memset(&priv.high_key, 0xFF, sizeof(priv.high_key));
priv.high_key.rm_blockcount = 0;
priv.agcnts = agcnts;
priv.last_rec.rm_owner = XFS_RMAP_OWN_UNKNOWN;
priv.nr_blocks = 0;
+
+ levels = be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]);
+ if (levels >= XFS_BTREE_MAXLEVELS) {
+ do_warn(_("bad levels %u for rmapbt root, agno %d\n"),
+ levels, agno);
+ rmap_avoid_check();
+ }
+
bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_RMAP]);
if (libxfs_verify_agbno(mp, agno, bno)) {
- scan_sbtree(bno,
- be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]),
- agno, 0, scan_rmapbt, 1, XFS_RMAP_CRC_MAGIC,
- &priv, &xfs_rmapbt_buf_ops);
+ scan_sbtree(bno, levels, agno, 0, scan_rmapbt, 1,
+ XFS_RMAP_CRC_MAGIC, &priv,
+ &xfs_rmapbt_buf_ops);
if (be32_to_cpu(agf->agf_rmap_blocks) != priv.nr_blocks)
do_warn(_("bad rmapbt block count %u, saw %u\n"),
priv.nr_blocks,
be32_to_cpu(agf->agf_rmap_blocks));
- } else {
+ } else {
do_warn(_("bad agbno %u for rmapbt root, agno %d\n"),
bno, agno);
rmap_avoid_check();
@@ -2303,20 +2311,28 @@ validate_agf(
}
if (xfs_sb_version_hasreflink(&mp->m_sb)) {
+ unsigned int levels;
+
+ levels = be32_to_cpu(agf->agf_refcount_level);
+ if (levels >= XFS_BTREE_MAXLEVELS) {
+ do_warn(_("bad levels %u for refcountbt root, agno %d\n"),
+ levels, agno);
+ refcount_avoid_check();
+ }
+
bno = be32_to_cpu(agf->agf_refcount_root);
if (libxfs_verify_agbno(mp, agno, bno)) {
struct refc_priv priv;
memset(&priv, 0, sizeof(priv));
- scan_sbtree(bno,
- be32_to_cpu(agf->agf_refcount_level),
- agno, 0, scan_refcbt, 1, XFS_REFC_CRC_MAGIC,
- &priv, &xfs_refcountbt_buf_ops);
+ scan_sbtree(bno, levels, agno, 0, scan_refcbt, 1,
+ XFS_REFC_CRC_MAGIC, &priv,
+ &xfs_refcountbt_buf_ops);
if (be32_to_cpu(agf->agf_refcount_blocks) != priv.nr_blocks)
do_warn(_("bad refcountbt block count %u, saw %u\n"),
priv.nr_blocks,
be32_to_cpu(agf->agf_refcount_blocks));
- } else {
+ } else {
do_warn(_("bad agbno %u for refcntbt root, agno %d\n"),
bno, agno);
refcount_avoid_check();
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 4/5] xfs_repair: skip the rmap and refcount btree checks when the levels are garbage
2020-10-26 23:32 ` [PATCH 4/5] xfs_repair: skip the rmap and refcount btree checks when the levels are garbage Darrick J. Wong
@ 2020-10-27 5:35 ` Allison Henderson
2020-10-28 7:34 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Allison Henderson @ 2020-10-27 5:35 UTC (permalink / raw)
To: Darrick J. Wong, sandeen; +Cc: linux-xfs
On 10/26/20 4:32 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> In validate_ag[fi], we should check that the levels of the rmap and
> refcount btrees are valid. If they aren't, we need to tell phase4 to
> skip the comparison between the existing and incore rmap and refcount
> data. The comparison routines use libxfs btree cursors, which assume
> that the caller validated bc_nlevels and will corrupt memory if we load
> a btree cursor with a garbage level count.
>
> This was found by examing a core dump from a failed xfs/086 invocation.
>
Ok, looks ok
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> repair/scan.c | 36 ++++++++++++++++++++++++++----------
> 1 file changed, 26 insertions(+), 10 deletions(-)
>
>
> diff --git a/repair/scan.c b/repair/scan.c
> index 42b299f75067..2a38ae5197c6 100644
> --- a/repair/scan.c
> +++ b/repair/scan.c
> @@ -2279,23 +2279,31 @@ validate_agf(
>
> if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
> struct rmap_priv priv;
> + unsigned int levels;
>
> memset(&priv.high_key, 0xFF, sizeof(priv.high_key));
> priv.high_key.rm_blockcount = 0;
> priv.agcnts = agcnts;
> priv.last_rec.rm_owner = XFS_RMAP_OWN_UNKNOWN;
> priv.nr_blocks = 0;
> +
> + levels = be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]);
> + if (levels >= XFS_BTREE_MAXLEVELS) {
> + do_warn(_("bad levels %u for rmapbt root, agno %d\n"),
> + levels, agno);
> + rmap_avoid_check();
> + }
> +
> bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_RMAP]);
> if (libxfs_verify_agbno(mp, agno, bno)) {
> - scan_sbtree(bno,
> - be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]),
> - agno, 0, scan_rmapbt, 1, XFS_RMAP_CRC_MAGIC,
> - &priv, &xfs_rmapbt_buf_ops);
> + scan_sbtree(bno, levels, agno, 0, scan_rmapbt, 1,
> + XFS_RMAP_CRC_MAGIC, &priv,
> + &xfs_rmapbt_buf_ops);
> if (be32_to_cpu(agf->agf_rmap_blocks) != priv.nr_blocks)
> do_warn(_("bad rmapbt block count %u, saw %u\n"),
> priv.nr_blocks,
> be32_to_cpu(agf->agf_rmap_blocks));
> - } else {
> + } else {
> do_warn(_("bad agbno %u for rmapbt root, agno %d\n"),
> bno, agno);
> rmap_avoid_check();
> @@ -2303,20 +2311,28 @@ validate_agf(
> }
>
> if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> + unsigned int levels;
> +
> + levels = be32_to_cpu(agf->agf_refcount_level);
> + if (levels >= XFS_BTREE_MAXLEVELS) {
> + do_warn(_("bad levels %u for refcountbt root, agno %d\n"),
> + levels, agno);
> + refcount_avoid_check();
> + }
> +
> bno = be32_to_cpu(agf->agf_refcount_root);
> if (libxfs_verify_agbno(mp, agno, bno)) {
> struct refc_priv priv;
>
> memset(&priv, 0, sizeof(priv));
> - scan_sbtree(bno,
> - be32_to_cpu(agf->agf_refcount_level),
> - agno, 0, scan_refcbt, 1, XFS_REFC_CRC_MAGIC,
> - &priv, &xfs_refcountbt_buf_ops);
> + scan_sbtree(bno, levels, agno, 0, scan_refcbt, 1,
> + XFS_REFC_CRC_MAGIC, &priv,
> + &xfs_refcountbt_buf_ops);
> if (be32_to_cpu(agf->agf_refcount_blocks) != priv.nr_blocks)
> do_warn(_("bad refcountbt block count %u, saw %u\n"),
> priv.nr_blocks,
> be32_to_cpu(agf->agf_refcount_blocks));
> - } else {
> + } else {
> do_warn(_("bad agbno %u for refcntbt root, agno %d\n"),
> bno, agno);
> refcount_avoid_check();
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 4/5] xfs_repair: skip the rmap and refcount btree checks when the levels are garbage
2020-10-26 23:32 ` [PATCH 4/5] xfs_repair: skip the rmap and refcount btree checks when the levels are garbage Darrick J. Wong
2020-10-27 5:35 ` Allison Henderson
@ 2020-10-28 7:34 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-10-28 7:34 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
On Mon, Oct 26, 2020 at 04:32:18PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> In validate_ag[fi], we should check that the levels of the rmap and
> refcount btrees are valid. If they aren't, we need to tell phase4 to
> skip the comparison between the existing and incore rmap and refcount
> data. The comparison routines use libxfs btree cursors, which assume
> that the caller validated bc_nlevels and will corrupt memory if we load
> a btree cursor with a garbage level count.
>
> This was found by examing a core dump from a failed xfs/086 invocation.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/5] xfs_repair: correctly detect partially written extents
2020-10-26 23:31 [PATCH 0/5] xfsprogs: fixes for 5.10 Darrick J. Wong
` (3 preceding siblings ...)
2020-10-26 23:32 ` [PATCH 4/5] xfs_repair: skip the rmap and refcount btree checks when the levels are garbage Darrick J. Wong
@ 2020-10-26 23:32 ` Darrick J. Wong
2020-10-27 5:52 ` Allison Henderson
2020-10-28 7:35 ` Christoph Hellwig
4 siblings, 2 replies; 26+ messages in thread
From: Darrick J. Wong @ 2020-10-26 23:32 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
Recently, I was able to create a realtime file with a 16b extent size
and the following data fork mapping:
data offset 0 startblock 144 (0/144) count 3 flag 0
data offset 3 startblock 147 (0/147) count 3 flag 1
data offset 6 startblock 150 (0/150) count 10 flag 0
Notice how we have a written extent, then an unwritten extent, and then
another written extent. The current code in process_rt_rec trips over
that third extent, because repair only knows not to complain about inuse
extents if the mapping was unwritten.
This loop logic is confusing, because it tries to do too many things.
Move the phase3 and phase4 code to separate helper functions, then
isolate the code that handles a mapping that starts in the middle of an
rt extent so that it's clearer what's going on.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
repair/dinode.c | 180 ++++++++++++++++++++++++++++++++++---------------------
1 file changed, 112 insertions(+), 68 deletions(-)
diff --git a/repair/dinode.c b/repair/dinode.c
index c89f21e08373..028a23cd5c8c 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -176,76 +176,69 @@ verify_dfsbno_range(
return XR_DFSBNORANGE_VALID;
}
+
static int
-process_rt_rec(
+process_rt_rec_dups(
struct xfs_mount *mp,
- struct xfs_bmbt_irec *irec,
xfs_ino_t ino,
- xfs_rfsblock_t *tot,
- int check_dups)
+ struct xfs_bmbt_irec *irec)
{
- xfs_fsblock_t b, lastb;
+ xfs_fsblock_t b;
xfs_rtblock_t ext;
- int state;
- int pwe; /* partially-written extent */
- /*
- * check numeric validity of the extent
- */
- if (!libxfs_verify_rtbno(mp, irec->br_startblock)) {
- do_warn(
-_("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" PRIu64 "\n"),
- ino,
- irec->br_startblock,
- irec->br_startoff);
- return 1;
- }
-
- lastb = irec->br_startblock + irec->br_blockcount - 1;
- if (!libxfs_verify_rtbno(mp, lastb)) {
- do_warn(
-_("inode %" PRIu64 " - bad rt extent last block number %" PRIu64 ", offset %" PRIu64 "\n"),
- ino,
- lastb,
- irec->br_startoff);
- return 1;
- }
- if (lastb < irec->br_startblock) {
- do_warn(
-_("inode %" PRIu64 " - bad rt extent overflows - start %" PRIu64 ", "
- "end %" PRIu64 ", offset %" PRIu64 "\n"),
- ino,
- irec->br_startblock,
- lastb,
- irec->br_startoff);
- return 1;
- }
-
- /*
- * set the appropriate number of extents
- * this iterates block by block, this can be optimised using extents
- */
- for (b = irec->br_startblock; b < irec->br_startblock +
- irec->br_blockcount; b += mp->m_sb.sb_rextsize) {
+ for (b = rounddown(irec->br_startblock, mp->m_sb.sb_rextsize);
+ b < irec->br_startblock + irec->br_blockcount;
+ b += mp->m_sb.sb_rextsize) {
ext = (xfs_rtblock_t) b / mp->m_sb.sb_rextsize;
- pwe = irec->br_state == XFS_EXT_UNWRITTEN &&
- (b % mp->m_sb.sb_rextsize != 0);
-
- if (check_dups == 1) {
- if (search_rt_dup_extent(mp, ext) && !pwe) {
- do_warn(
+ if (search_rt_dup_extent(mp, ext)) {
+ do_warn(
_("data fork in rt ino %" PRIu64 " claims dup rt extent,"
- "off - %" PRIu64 ", start - %" PRIu64 ", count %" PRIu64 "\n"),
- ino,
- irec->br_startoff,
- irec->br_startblock,
- irec->br_blockcount);
- return 1;
- }
- continue;
+"off - %" PRIu64 ", start - %" PRIu64 ", count %" PRIu64 "\n"),
+ ino,
+ irec->br_startoff,
+ irec->br_startblock,
+ irec->br_blockcount);
+ return 1;
}
+ }
+ return 0;
+}
+
+static int
+process_rt_rec_state(
+ struct xfs_mount *mp,
+ xfs_ino_t ino,
+ struct xfs_bmbt_irec *irec)
+{
+ xfs_fsblock_t b = irec->br_startblock;
+ xfs_rtblock_t ext;
+ int state;
+
+ do {
+ ext = (xfs_rtblock_t)b / mp->m_sb.sb_rextsize;
state = get_rtbmap(ext);
+
+ if ((b % mp->m_sb.sb_rextsize) != 0) {
+ /*
+ * We are midway through a partially written extent.
+ * If we don't find the state that gets set in the
+ * other clause of this loop body, then we have a
+ * partially *mapped* rt extent and should complain.
+ */
+ if (state != XR_E_INUSE)
+ do_error(
+_("data fork in rt inode %" PRIu64 " found invalid rt extent %"PRIu64" state %d at rt block %"PRIu64"\n"),
+ ino, ext, state, b);
+ b = roundup(b, mp->m_sb.sb_rextsize);
+ continue;
+ }
+
+ /*
+ * This is the start of an rt extent. Set the extent state if
+ * nobody else has claimed the extent, or complain if there are
+ * conflicting states.
+ */
switch (state) {
case XR_E_FREE:
case XR_E_UNKNOWN:
@@ -253,32 +246,83 @@ _("data fork in rt ino %" PRIu64 " claims dup rt extent,"
break;
case XR_E_BAD_STATE:
do_error(
-_("bad state in rt block map %" PRIu64 "\n"),
+_("bad state in rt extent map %" PRIu64 "\n"),
ext);
case XR_E_FS_MAP:
case XR_E_INO:
case XR_E_INUSE_FS:
do_error(
-_("data fork in rt inode %" PRIu64 " found metadata block %" PRIu64 " in rt bmap\n"),
+_("data fork in rt inode %" PRIu64 " found rt metadata extent %" PRIu64 " in rt bmap\n"),
ino, ext);
case XR_E_INUSE:
- if (pwe)
- break;
- /* fall through */
case XR_E_MULT:
set_rtbmap(ext, XR_E_MULT);
do_warn(
-_("data fork in rt inode %" PRIu64 " claims used rt block %" PRIu64 "\n"),
- ino, ext);
+_("data fork in rt inode %" PRIu64 " claims used rt extent %" PRIu64 "\n"),
+ ino, b);
return 1;
case XR_E_FREE1:
default:
do_error(
-_("illegal state %d in rt block map %" PRIu64 "\n"),
- state, b);
+_("illegal state %d in rt extent %" PRIu64 "\n"),
+ state, ext);
}
+ b += mp->m_sb.sb_rextsize;
+ } while (b < irec->br_startblock + irec->br_blockcount);
+
+ return 0;
+}
+
+static int
+process_rt_rec(
+ struct xfs_mount *mp,
+ struct xfs_bmbt_irec *irec,
+ xfs_ino_t ino,
+ xfs_rfsblock_t *tot,
+ int check_dups)
+{
+ xfs_fsblock_t lastb;
+ int bad;
+
+ /*
+ * check numeric validity of the extent
+ */
+ if (!libxfs_verify_rtbno(mp, irec->br_startblock)) {
+ do_warn(
+_("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" PRIu64 "\n"),
+ ino,
+ irec->br_startblock,
+ irec->br_startoff);
+ return 1;
}
+ lastb = irec->br_startblock + irec->br_blockcount - 1;
+ if (!libxfs_verify_rtbno(mp, lastb)) {
+ do_warn(
+_("inode %" PRIu64 " - bad rt extent last block number %" PRIu64 ", offset %" PRIu64 "\n"),
+ ino,
+ lastb,
+ irec->br_startoff);
+ return 1;
+ }
+ if (lastb < irec->br_startblock) {
+ do_warn(
+_("inode %" PRIu64 " - bad rt extent overflows - start %" PRIu64 ", "
+ "end %" PRIu64 ", offset %" PRIu64 "\n"),
+ ino,
+ irec->br_startblock,
+ lastb,
+ irec->br_startoff);
+ return 1;
+ }
+
+ if (check_dups)
+ bad = process_rt_rec_dups(mp, ino, irec);
+ else
+ bad = process_rt_rec_state(mp, ino, irec);
+ if (bad)
+ return bad;
+
/*
* bump up the block counter
*/
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 5/5] xfs_repair: correctly detect partially written extents
2020-10-26 23:32 ` [PATCH 5/5] xfs_repair: correctly detect partially written extents Darrick J. Wong
@ 2020-10-27 5:52 ` Allison Henderson
2020-10-28 7:35 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Allison Henderson @ 2020-10-27 5:52 UTC (permalink / raw)
To: Darrick J. Wong, sandeen; +Cc: linux-xfs
On 10/26/20 4:32 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Recently, I was able to create a realtime file with a 16b extent size
> and the following data fork mapping:
>
> data offset 0 startblock 144 (0/144) count 3 flag 0
> data offset 3 startblock 147 (0/147) count 3 flag 1
> data offset 6 startblock 150 (0/150) count 10 flag 0
>
> Notice how we have a written extent, then an unwritten extent, and then
> another written extent. The current code in process_rt_rec trips over
> that third extent, because repair only knows not to complain about inuse
> extents if the mapping was unwritten.
>
> This loop logic is confusing, because it tries to do too many things.
> Move the phase3 and phase4 code to separate helper functions, then
> isolate the code that handles a mapping that starts in the middle of an
> rt extent so that it's clearer what's going on.
>
Ok, seems reasonable, though I think I might have hoisted one helper
separately to make that a little easier to read through.
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> repair/dinode.c | 180 ++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 112 insertions(+), 68 deletions(-)
>
>
> diff --git a/repair/dinode.c b/repair/dinode.c
> index c89f21e08373..028a23cd5c8c 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -176,76 +176,69 @@ verify_dfsbno_range(
>
> return XR_DFSBNORANGE_VALID;
> }
> +
> static int
> -process_rt_rec(
> +process_rt_rec_dups(
> struct xfs_mount *mp,
> - struct xfs_bmbt_irec *irec,
> xfs_ino_t ino,
> - xfs_rfsblock_t *tot,
> - int check_dups)
> + struct xfs_bmbt_irec *irec)
> {
> - xfs_fsblock_t b, lastb;
> + xfs_fsblock_t b;
> xfs_rtblock_t ext;
> - int state;
> - int pwe; /* partially-written extent */
>
> - /*
> - * check numeric validity of the extent
> - */
> - if (!libxfs_verify_rtbno(mp, irec->br_startblock)) {
> - do_warn(
> -_("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" PRIu64 "\n"),
> - ino,
> - irec->br_startblock,
> - irec->br_startoff);
> - return 1;
> - }
> -
> - lastb = irec->br_startblock + irec->br_blockcount - 1;
> - if (!libxfs_verify_rtbno(mp, lastb)) {
> - do_warn(
> -_("inode %" PRIu64 " - bad rt extent last block number %" PRIu64 ", offset %" PRIu64 "\n"),
> - ino,
> - lastb,
> - irec->br_startoff);
> - return 1;
> - }
> - if (lastb < irec->br_startblock) {
> - do_warn(
> -_("inode %" PRIu64 " - bad rt extent overflows - start %" PRIu64 ", "
> - "end %" PRIu64 ", offset %" PRIu64 "\n"),
> - ino,
> - irec->br_startblock,
> - lastb,
> - irec->br_startoff);
> - return 1;
> - }
> -
> - /*
> - * set the appropriate number of extents
> - * this iterates block by block, this can be optimised using extents
> - */
> - for (b = irec->br_startblock; b < irec->br_startblock +
> - irec->br_blockcount; b += mp->m_sb.sb_rextsize) {
> + for (b = rounddown(irec->br_startblock, mp->m_sb.sb_rextsize);
> + b < irec->br_startblock + irec->br_blockcount;
> + b += mp->m_sb.sb_rextsize) {
> ext = (xfs_rtblock_t) b / mp->m_sb.sb_rextsize;
> - pwe = irec->br_state == XFS_EXT_UNWRITTEN &&
> - (b % mp->m_sb.sb_rextsize != 0);
> -
> - if (check_dups == 1) {
> - if (search_rt_dup_extent(mp, ext) && !pwe) {
> - do_warn(
> + if (search_rt_dup_extent(mp, ext)) {
> + do_warn(
> _("data fork in rt ino %" PRIu64 " claims dup rt extent,"
> - "off - %" PRIu64 ", start - %" PRIu64 ", count %" PRIu64 "\n"),
> - ino,
> - irec->br_startoff,
> - irec->br_startblock,
> - irec->br_blockcount);
> - return 1;
> - }
> - continue;
> +"off - %" PRIu64 ", start - %" PRIu64 ", count %" PRIu64 "\n"),
> + ino,
> + irec->br_startoff,
> + irec->br_startblock,
> + irec->br_blockcount);
> + return 1;
> }
> + }
>
> + return 0;
> +}
> +
> +static int
> +process_rt_rec_state(
> + struct xfs_mount *mp,
> + xfs_ino_t ino,
> + struct xfs_bmbt_irec *irec)
> +{
> + xfs_fsblock_t b = irec->br_startblock;
> + xfs_rtblock_t ext;
> + int state;
> +
> + do {
> + ext = (xfs_rtblock_t)b / mp->m_sb.sb_rextsize;
> state = get_rtbmap(ext);
> +
> + if ((b % mp->m_sb.sb_rextsize) != 0) {
> + /*
> + * We are midway through a partially written extent.
> + * If we don't find the state that gets set in the
> + * other clause of this loop body, then we have a
> + * partially *mapped* rt extent and should complain.
> + */
> + if (state != XR_E_INUSE)
> + do_error(
> +_("data fork in rt inode %" PRIu64 " found invalid rt extent %"PRIu64" state %d at rt block %"PRIu64"\n"),
> + ino, ext, state, b);
> + b = roundup(b, mp->m_sb.sb_rextsize);
> + continue;
> + }
> +
> + /*
> + * This is the start of an rt extent. Set the extent state if
> + * nobody else has claimed the extent, or complain if there are
> + * conflicting states.
> + */
> switch (state) {
> case XR_E_FREE:
> case XR_E_UNKNOWN:
> @@ -253,32 +246,83 @@ _("data fork in rt ino %" PRIu64 " claims dup rt extent,"
> break;
> case XR_E_BAD_STATE:
> do_error(
> -_("bad state in rt block map %" PRIu64 "\n"),
> +_("bad state in rt extent map %" PRIu64 "\n"),
> ext);
> case XR_E_FS_MAP:
> case XR_E_INO:
> case XR_E_INUSE_FS:
> do_error(
> -_("data fork in rt inode %" PRIu64 " found metadata block %" PRIu64 " in rt bmap\n"),
> +_("data fork in rt inode %" PRIu64 " found rt metadata extent %" PRIu64 " in rt bmap\n"),
> ino, ext);
> case XR_E_INUSE:
> - if (pwe)
> - break;
> - /* fall through */
> case XR_E_MULT:
> set_rtbmap(ext, XR_E_MULT);
> do_warn(
> -_("data fork in rt inode %" PRIu64 " claims used rt block %" PRIu64 "\n"),
> - ino, ext);
> +_("data fork in rt inode %" PRIu64 " claims used rt extent %" PRIu64 "\n"),
> + ino, b);
> return 1;
> case XR_E_FREE1:
> default:
> do_error(
> -_("illegal state %d in rt block map %" PRIu64 "\n"),
> - state, b);
> +_("illegal state %d in rt extent %" PRIu64 "\n"),
> + state, ext);
> }
> + b += mp->m_sb.sb_rextsize;
> + } while (b < irec->br_startblock + irec->br_blockcount);
> +
> + return 0;
> +}
> +
> +static int
> +process_rt_rec(
> + struct xfs_mount *mp,
> + struct xfs_bmbt_irec *irec,
> + xfs_ino_t ino,
> + xfs_rfsblock_t *tot,
> + int check_dups)
> +{
> + xfs_fsblock_t lastb;
> + int bad;
> +
> + /*
> + * check numeric validity of the extent
> + */
> + if (!libxfs_verify_rtbno(mp, irec->br_startblock)) {
> + do_warn(
> +_("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" PRIu64 "\n"),
> + ino,
> + irec->br_startblock,
> + irec->br_startoff);
> + return 1;
> }
>
> + lastb = irec->br_startblock + irec->br_blockcount - 1;
> + if (!libxfs_verify_rtbno(mp, lastb)) {
> + do_warn(
> +_("inode %" PRIu64 " - bad rt extent last block number %" PRIu64 ", offset %" PRIu64 "\n"),
> + ino,
> + lastb,
> + irec->br_startoff);
> + return 1;
> + }
> + if (lastb < irec->br_startblock) {
> + do_warn(
> +_("inode %" PRIu64 " - bad rt extent overflows - start %" PRIu64 ", "
> + "end %" PRIu64 ", offset %" PRIu64 "\n"),
> + ino,
> + irec->br_startblock,
> + lastb,
> + irec->br_startoff);
> + return 1;
> + }
> +
> + if (check_dups)
> + bad = process_rt_rec_dups(mp, ino, irec);
> + else
> + bad = process_rt_rec_state(mp, ino, irec);
> + if (bad)
> + return bad;
> +
> /*
> * bump up the block counter
> */
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 5/5] xfs_repair: correctly detect partially written extents
2020-10-26 23:32 ` [PATCH 5/5] xfs_repair: correctly detect partially written extents Darrick J. Wong
2020-10-27 5:52 ` Allison Henderson
@ 2020-10-28 7:35 ` Christoph Hellwig
1 sibling, 0 replies; 26+ messages in thread
From: Christoph Hellwig @ 2020-10-28 7:35 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
On Mon, Oct 26, 2020 at 04:32:24PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Recently, I was able to create a realtime file with a 16b extent size
> and the following data fork mapping:
>
> data offset 0 startblock 144 (0/144) count 3 flag 0
> data offset 3 startblock 147 (0/147) count 3 flag 1
> data offset 6 startblock 150 (0/150) count 10 flag 0
>
> Notice how we have a written extent, then an unwritten extent, and then
> another written extent. The current code in process_rt_rec trips over
> that third extent, because repair only knows not to complain about inuse
> extents if the mapping was unwritten.
>
> This loop logic is confusing, because it tries to do too many things.
> Move the phase3 and phase4 code to separate helper functions, then
> isolate the code that handles a mapping that starts in the middle of an
> rt extent so that it's clearer what's going on.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 26+ messages in thread