[PATCH 0/2] xfs_db: more type_f cleanups

[PATCH 0/2] xfs_db: more type_f cleanups

[Eric Sandeen]

These go on top of Zorro's

"xfs_db: use correct inode to set inode type"

-Eric

[PATCH 1/2] xfs_db: short circuit type_f if type is unchanged

[Eric Sandeen]

There's no reason to go through the type change code if the
type has not been changed.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/db/type.c b/db/type.c
index 3cb1e868..572ac6d6 100644
--- a/db/type.c
+++ b/db/type.c
@@ -216,6 +216,8 @@ type_f(
 		tt = findtyp(argv[1]);
 		if (tt == NULL) {
 			dbprintf(_("no such type %s\n"), argv[1]);
+		} else if (iocur_top->typ == tt) {
+			return 0;
 		} else {
 			if (iocur_top->typ == NULL)
 				dbprintf(_("no current object\n"));

Re: [PATCH 1/2] xfs_db: short circuit type_f if type is unchanged

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 07:05:37PM -0500, Eric Sandeen wrote:
> There's no reason to go through the type change code if the
> type has not been changed.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/db/type.c b/db/type.c
> index 3cb1e868..572ac6d6 100644
> --- a/db/type.c
> +++ b/db/type.c
> @@ -216,6 +216,8 @@ type_f(
>  		tt = findtyp(argv[1]);
>  		if (tt == NULL) {
>  			dbprintf(_("no such type %s\n"), argv[1]);
> +		} else if (iocur_top->typ == tt) {
> +			return 0;

Doesn't this mean that verifier errors won't be printed if the user asks
to set the type to the current type?  e.g.

xfs_db> agf 0
xfs_db> addr bnoroot
xfs_db> fuzz -d level random
Allowing fuzz of corrupted data with good CRC
level = 59679
xfs_db> type bnobt
Metadata corruption detected at 0x5586779a7b18, xfs_bnobt block 0x8/0x1000
xfs_db> type bnobt
Metadata corruption detected at 0x5586779a7b18, xfs_bnobt block 0x8/0x1000

<shrug> OTOH, db doesn't consistently have that behavior either --
inodes only behave like that for crc errors, so maybe this is fine.

Eh whatever, it's the debugging tool, you should be paying attention
anyways.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  		} else {
>  			if (iocur_top->typ == NULL)
>  				dbprintf(_("no current object\n"));
> 

Re: [PATCH 1/2] xfs_db: short circuit type_f if type is unchanged

[Eric Sandeen]

On 8/21/20 9:46 AM, Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 07:05:37PM -0500, Eric Sandeen wrote:
>> There's no reason to go through the type change code if the
>> type has not been changed.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/db/type.c b/db/type.c
>> index 3cb1e868..572ac6d6 100644
>> --- a/db/type.c
>> +++ b/db/type.c
>> @@ -216,6 +216,8 @@ type_f(
>>  		tt = findtyp(argv[1]);
>>  		if (tt == NULL) {
>>  			dbprintf(_("no such type %s\n"), argv[1]);
>> +		} else if (iocur_top->typ == tt) {
>> +			return 0;
> 
> Doesn't this mean that verifier errors won't be printed if the user asks
> to set the type to the current type?  e.g.
> 
> xfs_db> agf 0
> xfs_db> addr bnoroot
> xfs_db> fuzz -d level random
> Allowing fuzz of corrupted data with good CRC
> level = 59679
> xfs_db> type bnobt
> Metadata corruption detected at 0x5586779a7b18, xfs_bnobt block 0x8/0x1000
> xfs_db> type bnobt
> Metadata corruption detected at 0x5586779a7b18, xfs_bnobt block 0x8/0x1000

Oh, ok, I hadn't thought about how we could change the buffer after the
fact, I was thinking "nothing should change"

Honestly this isn't critical, it was a trivially backportable fix for the weird
inode thing that Zorro fixed earlier, but with that in place it's not really
necessary.

I could go either way here too.  If we want to rerun verifiers by resetting
the type, that's fine.

-Eric

> <shrug> OTOH, db doesn't consistently have that behavior either --
> inodes only behave like that for crc errors, so maybe this is fine.
> 
> Eh whatever, it's the debugging tool, you should be paying attention
> anyways.
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> --D
> 
>>  		} else {
>>  			if (iocur_top->typ == NULL)
>>  				dbprintf(_("no current object\n"));
>>
> 

[PATCH 2/2] xfs_db: consolidate set_iocur_type behavior

[Eric Sandeen]

Right now there are 3 cases to type_f: inode type, type with fields,
and a default.  The first two were added to address issues with handling
V5 metadata.

The first two already use some version of set_cur, which handles all
of the validation etc. There's no reason to leave the open-coded bits
at the end, just send every non-inode type through set_cur and be done
with it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

 io.c |   28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/db/io.c b/db/io.c
index 884da599..235191f5 100644
--- a/db/io.c
+++ b/db/io.c
@@ -603,33 +603,15 @@ set_iocur_type(
 				iocur_top->boff / mp->m_sb.sb_inodesize);
 		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), agino);
 		set_cur_inode(ino);
-		return;
-	}
-
-	/* adjust buffer size for types with fields & hence fsize() */
-	if (type->fields) {
-		int bb_count;	/* type's size in basic blocks */
+	} else  {
+		int bb_count = 1;	/* type's size in basic blocks */
 
-		bb_count = BTOBB(byteize(fsize(type->fields,
+		/* adjust buffer size for types with fields & hence fsize() */
+		if (type->fields)
+			bb_count = BTOBB(byteize(fsize(type->fields,
 					       iocur_top->data, 0, 0)));
 		set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
 	}
-	iocur_top->typ = type;
-
-	/* verify the buffer if the type has one. */
-	if (!bp)
-		return;
-	if (!type->bops) {
-		bp->b_ops = NULL;
-		bp->b_flags |= LIBXFS_B_UNCHECKED;
-		return;
-	}
-	if (!(bp->b_flags & LIBXFS_B_UPTODATE))
-		return;
-	bp->b_error = 0;
-	bp->b_ops = type->bops;
-	bp->b_ops->verify_read(bp);
-	bp->b_flags &= ~LIBXFS_B_UNCHECKED;
 }
 
 static void

Re: [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior

[Darrick J. Wong]

On Thu, Aug 20, 2020 at 07:09:45PM -0500, Eric Sandeen wrote:
> Right now there are 3 cases to type_f: inode type, type with fields,
> and a default.  The first two were added to address issues with handling
> V5 metadata.
> 
> The first two already use some version of set_cur, which handles all
> of the validation etc. There's no reason to leave the open-coded bits
> at the end, just send every non-inode type through set_cur and be done
> with it.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
>  io.c |   28 +++++-----------------------
>  1 file changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/db/io.c b/db/io.c
> index 884da599..235191f5 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -603,33 +603,15 @@ set_iocur_type(
>  				iocur_top->boff / mp->m_sb.sb_inodesize);
>  		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), agino);
>  		set_cur_inode(ino);
> -		return;
> -	}
> -
> -	/* adjust buffer size for types with fields & hence fsize() */
> -	if (type->fields) {
> -		int bb_count;	/* type's size in basic blocks */
> +	} else  {

Two spaces    ^^ between the else and the paren.

I also wonder, why not leave the "return;" at the end of the
"if (type->typnm == TYP_INODE)" part and then you can reduce the
indenting level of the rest of the function?

<shrug> maintainer's prerogative on that one though.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

/me also wonders why dquots don't get the same "save the buffer offset"
treatment as inodes, but that's a separate question. :)

--D

> +		int bb_count = 1;	/* type's size in basic blocks */
>  
> -		bb_count = BTOBB(byteize(fsize(type->fields,
> +		/* adjust buffer size for types with fields & hence fsize() */
> +		if (type->fields)
> +			bb_count = BTOBB(byteize(fsize(type->fields,
>  					       iocur_top->data, 0, 0)));
>  		set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
>  	}
> -	iocur_top->typ = type;
> -
> -	/* verify the buffer if the type has one. */
> -	if (!bp)
> -		return;
> -	if (!type->bops) {
> -		bp->b_ops = NULL;
> -		bp->b_flags |= LIBXFS_B_UNCHECKED;
> -		return;
> -	}
> -	if (!(bp->b_flags & LIBXFS_B_UPTODATE))
> -		return;
> -	bp->b_error = 0;
> -	bp->b_ops = type->bops;
> -	bp->b_ops->verify_read(bp);
> -	bp->b_flags &= ~LIBXFS_B_UNCHECKED;
>  }
>  
>  static void
> 

Re: [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior

[Eric Sandeen]

On 8/21/20 9:52 AM, Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 07:09:45PM -0500, Eric Sandeen wrote:
>> Right now there are 3 cases to type_f: inode type, type with fields,
>> and a default.  The first two were added to address issues with handling
>> V5 metadata.
>>
>> The first two already use some version of set_cur, which handles all
>> of the validation etc. There's no reason to leave the open-coded bits
>> at the end, just send every non-inode type through set_cur and be done
>> with it.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>>  io.c |   28 +++++-----------------------
>>  1 file changed, 5 insertions(+), 23 deletions(-)
>>
>> diff --git a/db/io.c b/db/io.c
>> index 884da599..235191f5 100644
>> --- a/db/io.c
>> +++ b/db/io.c
>> @@ -603,33 +603,15 @@ set_iocur_type(
>>  				iocur_top->boff / mp->m_sb.sb_inodesize);
>>  		ino = XFS_AGINO_TO_INO(mp, xfs_daddr_to_agno(mp, b), agino);
>>  		set_cur_inode(ino);
>> -		return;
>> -	}
>> -
>> -	/* adjust buffer size for types with fields & hence fsize() */
>> -	if (type->fields) {
>> -		int bb_count;	/* type's size in basic blocks */
>> +	} else  {
> 
> Two spaces    ^^ between the else and the paren.
> 
> I also wonder, why not leave the "return;" at the end of the
> "if (type->typnm == TYP_INODE)" part and then you can reduce the
> indenting level of the rest of the function?

Oh, that might be nice.  I'll see how that looks.

> <shrug> maintainer's prerogative on that one though.
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> /me also wonders why dquots don't get the same "save the buffer offset"
> treatment as inodes, but that's a separate question. :)

hmmmm good question tho.

Thanks for the reviews,
-Eric

> --D
> 
>> +		int bb_count = 1;	/* type's size in basic blocks */
>>  
>> -		bb_count = BTOBB(byteize(fsize(type->fields,
>> +		/* adjust buffer size for types with fields & hence fsize() */
>> +		if (type->fields)
>> +			bb_count = BTOBB(byteize(fsize(type->fields,
>>  					       iocur_top->data, 0, 0)));
>>  		set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
>>  	}
>> -	iocur_top->typ = type;
>> -
>> -	/* verify the buffer if the type has one. */
>> -	if (!bp)
>> -		return;
>> -	if (!type->bops) {
>> -		bp->b_ops = NULL;
>> -		bp->b_flags |= LIBXFS_B_UNCHECKED;
>> -		return;
>> -	}
>> -	if (!(bp->b_flags & LIBXFS_B_UPTODATE))
>> -		return;
>> -	bp->b_error = 0;
>> -	bp->b_ops = type->bops;
>> -	bp->b_ops->verify_read(bp);
>> -	bp->b_flags &= ~LIBXFS_B_UNCHECKED;
>>  }
>>  
>>  static void
>>
> 

[PATCH 2/2 V2] xfs_db: consolidate set_iocur_type() behavior

[Eric Sandeen]

Right now there are 3 cases to type_f(): inode type, type with fields,
and a default. The first two were added to address issues with handling
V5 metadata.

The first two already use some version of set_cur(), which handles all
of the validation etc. There's no reason to leave the open-coded bits
at the end, just send every non-inode type through set_cur() and be
done with it.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Drop unused *bp var declaration
    un-indent/un-else the non-inode code

diff --git a/db/io.c b/db/io.c
index 884da599..b8cb767e 100644
--- a/db/io.c
+++ b/db/io.c
@@ -586,7 +586,7 @@ void
 set_iocur_type(
 	const typ_t	*type)
 {
-	struct xfs_buf	*bp = iocur_top->bp;
+	int 		bb_count = 1;	/* type's size in basic blocks */
 
 	/*
 	 * Inodes are special; verifier checks all inodes in the chunk, the
@@ -607,29 +607,10 @@ set_iocur_type(
 	}
 
 	/* adjust buffer size for types with fields & hence fsize() */
-	if (type->fields) {
-		int bb_count;	/* type's size in basic blocks */
-
+	if (type->fields)
 		bb_count = BTOBB(byteize(fsize(type->fields,
-					       iocur_top->data, 0, 0)));
-		set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
-	}
-	iocur_top->typ = type;
-
-	/* verify the buffer if the type has one. */
-	if (!bp)
-		return;
-	if (!type->bops) {
-		bp->b_ops = NULL;
-		bp->b_flags |= LIBXFS_B_UNCHECKED;
-		return;
-	}
-	if (!(bp->b_flags & LIBXFS_B_UPTODATE))
-		return;
-	bp->b_error = 0;
-	bp->b_ops = type->bops;
-	bp->b_ops->verify_read(bp);
-	bp->b_flags &= ~LIBXFS_B_UNCHECKED;
+				       iocur_top->data, 0, 0)));
+	set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
 }
 
 static void

Re: [PATCH 2/2 V2] xfs_db: consolidate set_iocur_type() behavior

[Darrick J. Wong]

On Mon, Aug 24, 2020 at 11:51:56AM -0500, Eric Sandeen wrote:
> Right now there are 3 cases to type_f(): inode type, type with fields,
> and a default. The first two were added to address issues with handling
> V5 metadata.
> 
> The first two already use some version of set_cur(), which handles all
> of the validation etc. There's no reason to leave the open-coded bits
> at the end, just send every non-inode type through set_cur() and be
> done with it.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: Drop unused *bp var declaration
>     un-indent/un-else the non-inode code
> 
> diff --git a/db/io.c b/db/io.c
> index 884da599..b8cb767e 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -586,7 +586,7 @@ void
>  set_iocur_type(
>  	const typ_t	*type)
>  {
> -	struct xfs_buf	*bp = iocur_top->bp;
> +	int 		bb_count = 1;	/* type's size in basic blocks */

Space after 'int' and before tabs.

With that fixed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  
>  	/*
>  	 * Inodes are special; verifier checks all inodes in the chunk, the
> @@ -607,29 +607,10 @@ set_iocur_type(
>  	}
>  
>  	/* adjust buffer size for types with fields & hence fsize() */
> -	if (type->fields) {
> -		int bb_count;	/* type's size in basic blocks */
> -
> +	if (type->fields)
>  		bb_count = BTOBB(byteize(fsize(type->fields,
> -					       iocur_top->data, 0, 0)));
> -		set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
> -	}
> -	iocur_top->typ = type;
> -
> -	/* verify the buffer if the type has one. */
> -	if (!bp)
> -		return;
> -	if (!type->bops) {
> -		bp->b_ops = NULL;
> -		bp->b_flags |= LIBXFS_B_UNCHECKED;
> -		return;
> -	}
> -	if (!(bp->b_flags & LIBXFS_B_UPTODATE))
> -		return;
> -	bp->b_error = 0;
> -	bp->b_ops = type->bops;
> -	bp->b_ops->verify_read(bp);
> -	bp->b_flags &= ~LIBXFS_B_UNCHECKED;
> +				       iocur_top->data, 0, 0)));
> +	set_cur(type, iocur_top->bb, bb_count, DB_RING_IGN, NULL);
>  }
>  
>  static void
> 
>