[PATCHSET 0/4] xfsprogs: random fixes

[PATCHSET 0/4] xfsprogs: random fixes

[Darrick J. Wong]

Hi all,

This is a rollup of all the random fixes I've collected for xfsprogs
5.19.  At this point it's just an assorted collection, no particular
theme.  Some of them are leftovers from last week's posting.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
 mkfs/xfs_mkfs.c      |    9 ++++++++-
 repair/attr_repair.c |   20 ++++++++++++++++++++
 repair/dino_chunks.c |    3 +--
 3 files changed, 29 insertions(+), 3 deletions(-)

[PATCH 1/4] xfs_repair: ignore empty xattr leaf blocks

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

As detailed in the previous commit, empty xattr leaf blocks can be the
benign byproduct of the system going down during the multi-step process
of adding a large xattr to a file that has no xattrs.  If we find one at
attr fork offset 0, we should clear it, but this isn't a corruption.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/attr_repair.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)


diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 2055d96e..c3a6d502 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -579,6 +579,26 @@ process_leaf_attr_block(
 	firstb = mp->m_sb.sb_blocksize;
 	stop = xfs_attr3_leaf_hdr_size(leaf);
 
+	/*
+	 * Empty leaf blocks at offset zero can occur as a race between
+	 * setxattr and the system going down, so we only take action if we're
+	 * running in modify mode.  See xfs_attr3_leaf_verify for details of
+	 * how we've screwed this up many times.
+	 */
+	if (!leafhdr.count && da_bno == 0) {
+		if (no_modify) {
+			do_log(
+	_("would clear empty leaf attr block 0, inode %" PRIu64 "\n"),
+					ino);
+			return 0;
+		}
+
+		do_warn(
+	_("will clear empty leaf attr block 0, inode %" PRIu64 "\n"),
+				ino);
+		return 1;
+	}
+
 	/* does the count look sorta valid? */
 	if (!leafhdr.count ||
 	    leafhdr.count * sizeof(xfs_attr_leaf_entry_t) + stop >

[PATCH 2/4] xfs_repair: Search for conflicts in inode_tree_ptrs[] when processing uncertain inodes

[Darrick J. Wong]

From: Chandan Babu R <chandan.babu@oracle.com>

When processing an uncertain inode chunk record, if we lose 2 blocks worth of
inodes or 25% of the chunk, xfs_repair decides to ignore the chunk. Otherwise,
xfs_repair adds a new chunk record to inode_tree_ptrs[agno], marking each
inode as either free or used. However, before adding the new chunk record,
xfs_repair has to check for the existance of a conflicting record.

The existing code incorrectly checks for the conflicting record in
inode_uncertain_tree_ptrs[agno]. This check will succeed since the inode chunk
record being processed was originally obtained from
inode_uncertain_tree_ptrs[agno].

This commit fixes the bug by changing xfs_repair to search
inode_tree_ptrs[agno] for conflicts.

Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/dino_chunks.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 11b0eb5f..80c52a43 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -229,8 +229,7 @@ verify_inode_chunk(xfs_mount_t		*mp,
 		/*
 		 * ok, put the record into the tree, if no conflict.
 		 */
-		if (find_uncertain_inode_rec(agno,
-				XFS_AGB_TO_AGINO(mp, start_agbno)))
+		if (find_inode_rec(mp, agno, XFS_AGB_TO_AGINO(mp, start_agbno)))
 			return(0);
 
 		start_agino = XFS_AGB_TO_AGINO(mp, start_agbno);

[PATCH 3/4] mkfs: complain about impossible log size constraints

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

xfs/042 trips over an impossible fs geometry when nrext64 is enabled.
The minimum log size calculation comes out to 4287 blocks, but the mkfs
parameters specify an AG size of 4096 blocks.  This eventually causes
mkfs to complain that the autoselected log size doesn't meet the minimum
size, but we could be a little more explicit in pointing out that the
two size constraints make for an impossible geometry.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |    7 +++++++
 1 file changed, 7 insertions(+)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index db322b3a..61ac1a4a 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3401,6 +3401,13 @@ _("external log device size %lld blocks too small, must be at least %lld blocks\
 	 * an AG.
 	 */
 	max_logblocks = libxfs_alloc_ag_max_usable(mp) - 1;
+	if (max_logblocks < min_logblocks) {
+		fprintf(stderr,
+_("max log size %d smaller than min log size %d\n"),
+				max_logblocks,
+				min_logblocks);
+		usage();
+	}
 
 	/* internal log - if no size specified, calculate automatically */
 	if (!cfg->logblocks) {

[PATCH 4/4] mkfs: terminate getsubopt arrays properly

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Having not drank any (or maybe too much) coffee this morning, I typed:

$ mkfs.xfs -d agcount=3 -d nrext64=0
Segmentation fault

I traced this down to getsubopt walking off the end of the dopts.subopts
array.  The manpage says you're supposed to terminate the suboptions
string array with a NULL entry, but the structure definition uses
MAX_SUBOPTS/D_MAX_OPTS directly, which means there is no terminator.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 61ac1a4a..9a58ff8b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -141,7 +141,7 @@ enum {
 };
 
 /* Just define the max options array size manually right now */
-#define MAX_SUBOPTS	D_MAX_OPTS
+#define MAX_SUBOPTS	(D_MAX_OPTS + 1)
 
 #define SUBOPT_NEEDS_VAL	(-1LL)
 #define MAX_CONFLICTS	8

Re: [PATCH 3/4] mkfs: complain about impossible log size constraints

[Eric Sandeen]

On 7/12/22 8:09 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> xfs/042 trips over an impossible fs geometry when nrext64 is enabled.
> The minimum log size calculation comes out to 4287 blocks, but the mkfs
> parameters specify an AG size of 4096 blocks.  This eventually causes
> mkfs to complain that the autoselected log size doesn't meet the minimum
> size, but we could be a little more explicit in pointing out that the
> two size constraints make for an impossible geometry.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  mkfs/xfs_mkfs.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index db322b3a..61ac1a4a 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3401,6 +3401,13 @@ _("external log device size %lld blocks too small, must be at least %lld blocks\
>  	 * an AG.
>  	 */
>  	max_logblocks = libxfs_alloc_ag_max_usable(mp) - 1;
> +	if (max_logblocks < min_logblocks) {
> +		fprintf(stderr,
> +_("max log size %d smaller than min log size %d\n"),

And when the user sees this, they will know that they should ___________ ?

> +				max_logblocks,
> +				min_logblocks);
> +		usage();
> +	}
>  
>  	/* internal log - if no size specified, calculate automatically */
>  	if (!cfg->logblocks) {
> 

Re: [PATCH 4/4] mkfs: terminate getsubopt arrays properly

[Eric Sandeen]

On 7/12/22 8:09 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Having not drank any (or maybe too much) coffee this morning, I typed:
> 
> $ mkfs.xfs -d agcount=3 -d nrext64=0
> Segmentation fault
> 
> I traced this down to getsubopt walking off the end of the dopts.subopts
> array.  The manpage says you're supposed to terminate the suboptions

(the getsubopt(3) manpage for those following along at home)

> string array with a NULL entry, but the structure definition uses
> MAX_SUBOPTS/D_MAX_OPTS directly, which means there is no terminator.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  mkfs/xfs_mkfs.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 61ac1a4a..9a58ff8b 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -141,7 +141,7 @@ enum {
>  };
>  
>  /* Just define the max options array size manually right now */
> -#define MAX_SUBOPTS	D_MAX_OPTS
> +#define MAX_SUBOPTS	(D_MAX_OPTS + 1)

Hah, I had not noticed this before. So this relies on there being more
suboptions for -d than anything else, I guess. What could go wrong?

OK, so this fixes it because opt_params is a global, and it contains 
subopt_params[MAX_SUBOPTS];, so the last array entry will be null
(by virtue of globals being zeroed) and that's all perfectly clear :D

Well, it fixes it for now.  I'd like to add i.e.

@@ -251,6 +251,7 @@ static struct opt_params bopts = {
        .ini_section = "block",
        .subopts = {
                [B_SIZE] = "size",
+               [B_MAX_OPTS] = NULL,
        },

etc to each suboption array to be explicit about it, sound ok? I can do
that on commit if it seems ok.

Reviewed-by: Eric Sandeen <sandeen@sandeen.net>

Thanks,
-Eric

>  
>  #define SUBOPT_NEEDS_VAL	(-1LL)
>  #define MAX_CONFLICTS	8
> 

Re: [PATCH 4/4] mkfs: terminate getsubopt arrays properly

[Darrick J. Wong]

On Wed, Jul 13, 2022 at 08:39:24PM -0500, Eric Sandeen wrote:
> On 7/12/22 8:09 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Having not drank any (or maybe too much) coffee this morning, I typed:
> > 
> > $ mkfs.xfs -d agcount=3 -d nrext64=0
> > Segmentation fault
> > 
> > I traced this down to getsubopt walking off the end of the dopts.subopts
> > array.  The manpage says you're supposed to terminate the suboptions
> 
> (the getsubopt(3) manpage for those following along at home)
> 
> > string array with a NULL entry, but the structure definition uses
> > MAX_SUBOPTS/D_MAX_OPTS directly, which means there is no terminator.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  mkfs/xfs_mkfs.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 61ac1a4a..9a58ff8b 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -141,7 +141,7 @@ enum {
> >  };
> >  
> >  /* Just define the max options array size manually right now */
> > -#define MAX_SUBOPTS	D_MAX_OPTS
> > +#define MAX_SUBOPTS	(D_MAX_OPTS + 1)
> 
> Hah, I had not noticed this before. So this relies on there being more
> suboptions for -d than anything else, I guess. What could go wrong?
> 
> OK, so this fixes it because opt_params is a global, and it contains 
> subopt_params[MAX_SUBOPTS];, so the last array entry will be null
> (by virtue of globals being zeroed) and that's all perfectly clear :D

<nod>

> Well, it fixes it for now.  I'd like to add i.e.
> 
> @@ -251,6 +251,7 @@ static struct opt_params bopts = {
>         .ini_section = "block",
>         .subopts = {
>                 [B_SIZE] = "size",
> +               [B_MAX_OPTS] = NULL,
>         },
> 
> etc to each suboption array to be explicit about it, sound ok? I can do
> that on commit if it seems ok.

Oh, that /is/ a good idea, in case B_MAX_OPTS > D_MAX_OPTS ever happens.

--D

> Reviewed-by: Eric Sandeen <sandeen@sandeen.net>
> 
> Thanks,
> -Eric
> 
> >  
> >  #define SUBOPT_NEEDS_VAL	(-1LL)
> >  #define MAX_CONFLICTS	8
> > 

Re: [PATCH 4/4] mkfs: terminate getsubopt arrays properly

[Eric Sandeen]

On 7/13/22 8:59 PM, Darrick J. Wong wrote:
> On Wed, Jul 13, 2022 at 08:39:24PM -0500, Eric Sandeen wrote:
>> On 7/12/22 8:09 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <djwong@kernel.org>
>>>
>>> Having not drank any (or maybe too much) coffee this morning, I typed:
>>>
>>> $ mkfs.xfs -d agcount=3 -d nrext64=0
>>> Segmentation fault
>>>
>>> I traced this down to getsubopt walking off the end of the dopts.subopts
>>> array.  The manpage says you're supposed to terminate the suboptions
>>
>> (the getsubopt(3) manpage for those following along at home)
>>
>>> string array with a NULL entry, but the structure definition uses
>>> MAX_SUBOPTS/D_MAX_OPTS directly, which means there is no terminator.
>>>
>>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>>> ---
>>>  mkfs/xfs_mkfs.c |    2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>
>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>> index 61ac1a4a..9a58ff8b 100644
>>> --- a/mkfs/xfs_mkfs.c
>>> +++ b/mkfs/xfs_mkfs.c
>>> @@ -141,7 +141,7 @@ enum {
>>>  };
>>>  
>>>  /* Just define the max options array size manually right now */
>>> -#define MAX_SUBOPTS	D_MAX_OPTS
>>> +#define MAX_SUBOPTS	(D_MAX_OPTS + 1)
>>
>> Hah, I had not noticed this before. So this relies on there being more
>> suboptions for -d than anything else, I guess. What could go wrong?
>>
>> OK, so this fixes it because opt_params is a global, and it contains 
>> subopt_params[MAX_SUBOPTS];, so the last array entry will be null
>> (by virtue of globals being zeroed) and that's all perfectly clear :D
> 
> <nod>
> 
>> Well, it fixes it for now.  I'd like to add i.e.
>>
>> @@ -251,6 +251,7 @@ static struct opt_params bopts = {
>>         .ini_section = "block",
>>         .subopts = {
>>                 [B_SIZE] = "size",
>> +               [B_MAX_OPTS] = NULL,
>>         },
>>
>> etc to each suboption array to be explicit about it, sound ok? I can do
>> that on commit if it seems ok.
> 
> Oh, that /is/ a good idea, in case B_MAX_OPTS > D_MAX_OPTS ever happens.

I, uh, think that in that case, gcc will barf out with something like:

xfs_mkfs.c:311:3: error: array index in initializer exceeds array bounds
   [D_MAX_OPTS] = NULL,
   ^
xfs_mkfs.c:311:3: error: (near initialization for ‘dopts.subopts’)
xfs_mkfs.c:311:3: warning: excess elements in array initializer [enabled by default]
xfs_mkfs.c:311:3: warning: (near initialization for ‘dopts.subopts’) [enabled by default]
cc1: warning: unrecognized command line option "-Wno-address-of-packed-member" [enabled by default]
gmake[2]: *** [xfs_mkfs.o] Error 1
gmake[1]: *** [mkfs] Error 2
make: *** [default] Error 2

(with s/dopts/bopts/ in your case)

-Eric

Re: [PATCH 3/4] mkfs: complain about impossible log size constraints

[Darrick J. Wong]

On Wed, Jul 13, 2022 at 08:17:22PM -0500, Eric Sandeen wrote:
> On 7/12/22 8:09 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > xfs/042 trips over an impossible fs geometry when nrext64 is enabled.
> > The minimum log size calculation comes out to 4287 blocks, but the mkfs
> > parameters specify an AG size of 4096 blocks.  This eventually causes
> > mkfs to complain that the autoselected log size doesn't meet the minimum
> > size, but we could be a little more explicit in pointing out that the
> > two size constraints make for an impossible geometry.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  mkfs/xfs_mkfs.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index db322b3a..61ac1a4a 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3401,6 +3401,13 @@ _("external log device size %lld blocks too small, must be at least %lld blocks\
> >  	 * an AG.
> >  	 */
> >  	max_logblocks = libxfs_alloc_ag_max_usable(mp) - 1;
> > +	if (max_logblocks < min_logblocks) {
> > +		fprintf(stderr,
> > +_("max log size %d smaller than min log size %d\n"),
> 
> And when the user sees this, they will know that they should ___________ ?

I dunno, ask for creating a bigger filesystem?

It's better than "log size 4083 blocks too small, minimum size is 4287
blocks", which hides the part where we autoselected 4083 blocks because
that's max_logblocks.

/me would suggest pulling in the no tiny fs patch, which will at least
fail the cases where the user wants tiny AGs but the featureset wants a
big log with "your fs is too small".

I haven't gotten around to playing with raid stripe size variations yet
though, so I don't know if this problem will come back with (say) a 301M
filesystem and a giant RAID stripe.

--D

> > +				max_logblocks,
> > +				min_logblocks);
> > +		usage();
> > +	}
> >  
> >  	/* internal log - if no size specified, calculate automatically */
> >  	if (!cfg->logblocks) {
> >