* [PATCH 0/2] xfs_db: more type_f cleanups
@ 2020-08-20 23:44 Eric Sandeen
2020-08-21 0:05 ` [PATCH 1/2] xfs_db: short circuit type_f if type is unchanged Eric Sandeen
2020-08-21 0:09 ` [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior Eric Sandeen
0 siblings, 2 replies; 9+ messages in thread
From: Eric Sandeen @ 2020-08-20 23:44 UTC (permalink / raw)
To: linux-xfs
These go on top of Zorro's
"xfs_db: use correct inode to set inode type"
-Eric
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] xfs_db: short circuit type_f if type is unchanged
2020-08-20 23:44 [PATCH 0/2] xfs_db: more type_f cleanups Eric Sandeen
@ 2020-08-21 0:05 ` Eric Sandeen
2020-08-21 14:46 ` Darrick J. Wong
2020-08-21 0:09 ` [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior Eric Sandeen
1 sibling, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2020-08-21 0:05 UTC (permalink / raw)
To: linux-xfs
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"));
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior
2020-08-20 23:44 [PATCH 0/2] xfs_db: more type_f cleanups Eric Sandeen
2020-08-21 0:05 ` [PATCH 1/2] xfs_db: short circuit type_f if type is unchanged Eric Sandeen
@ 2020-08-21 0:09 ` Eric Sandeen
2020-08-21 14:52 ` Darrick J. Wong
2020-08-24 16:51 ` [PATCH 2/2 V2] xfs_db: consolidate set_iocur_type() behavior Eric Sandeen
1 sibling, 2 replies; 9+ messages in thread
From: Eric Sandeen @ 2020-08-21 0:09 UTC (permalink / raw)
To: linux-xfs
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xfs_db: short circuit type_f if type is unchanged
2020-08-21 0:05 ` [PATCH 1/2] xfs_db: short circuit type_f if type is unchanged Eric Sandeen
@ 2020-08-21 14:46 ` Darrick J. Wong
2020-08-21 15:46 ` Eric Sandeen
0 siblings, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2020-08-21 14:46 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
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"));
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior
2020-08-21 0:09 ` [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior Eric Sandeen
@ 2020-08-21 14:52 ` Darrick J. Wong
2020-08-21 15:48 ` Eric Sandeen
2020-08-24 16:51 ` [PATCH 2/2 V2] xfs_db: consolidate set_iocur_type() behavior Eric Sandeen
1 sibling, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2020-08-21 14:52 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xfs_db: short circuit type_f if type is unchanged
2020-08-21 14:46 ` Darrick J. Wong
@ 2020-08-21 15:46 ` Eric Sandeen
0 siblings, 0 replies; 9+ messages in thread
From: Eric Sandeen @ 2020-08-21 15:46 UTC (permalink / raw)
To: Darrick J. Wong, Eric Sandeen; +Cc: linux-xfs
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"));
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior
2020-08-21 14:52 ` Darrick J. Wong
@ 2020-08-21 15:48 ` Eric Sandeen
0 siblings, 0 replies; 9+ messages in thread
From: Eric Sandeen @ 2020-08-21 15:48 UTC (permalink / raw)
To: Darrick J. Wong, Eric Sandeen; +Cc: linux-xfs
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
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2 V2] xfs_db: consolidate set_iocur_type() behavior
2020-08-21 0:09 ` [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior Eric Sandeen
2020-08-21 14:52 ` Darrick J. Wong
@ 2020-08-24 16:51 ` Eric Sandeen
2020-08-24 17:03 ` Darrick J. Wong
1 sibling, 1 reply; 9+ messages in thread
From: Eric Sandeen @ 2020-08-24 16:51 UTC (permalink / raw)
To: linux-xfs; +Cc: Darrick J. Wong
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2 V2] xfs_db: consolidate set_iocur_type() behavior
2020-08-24 16:51 ` [PATCH 2/2 V2] xfs_db: consolidate set_iocur_type() behavior Eric Sandeen
@ 2020-08-24 17:03 ` Darrick J. Wong
0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2020-08-24 17:03 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-24 17:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-20 23:44 [PATCH 0/2] xfs_db: more type_f cleanups Eric Sandeen
2020-08-21 0:05 ` [PATCH 1/2] xfs_db: short circuit type_f if type is unchanged Eric Sandeen
2020-08-21 14:46 ` Darrick J. Wong
2020-08-21 15:46 ` Eric Sandeen
2020-08-21 0:09 ` [PATCH 2/2] xfs_db: consolidate set_iocur_type behavior Eric Sandeen
2020-08-21 14:52 ` Darrick J. Wong
2020-08-21 15:48 ` Eric Sandeen
2020-08-24 16:51 ` [PATCH 2/2 V2] xfs_db: consolidate set_iocur_type() behavior Eric Sandeen
2020-08-24 17:03 ` Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).