* [PATCH] xfs_repair: handle missing extent states
@ 2017-09-18 16:39 Darrick J. Wong
2017-09-18 17:16 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2017-09-18 16:39 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs, amir73il
Missed a couple of the new extent states in the bmbt processing, so add
them to avoid aborting xfs_repair.
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
repair/dinode.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/repair/dinode.c b/repair/dinode.c
index f817b5a..b35a523 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -796,6 +796,7 @@ _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"),
case XR_E_FS_MAP:
case XR_E_INO:
case XR_E_INUSE_FS:
+ case XR_E_REFC:
do_warn(
_("%s fork in inode %" PRIu64 " claims metadata block %" PRIu64 "\n"),
forkname, ino, b);
@@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
forkname, ftype, ino, b);
goto done;
+ case XR_E_COW:
+ do_warn(
+_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
+ forkname, ftype, ino, b);
+ goto done;
+
default:
do_error(
_("illegal state %d in block map %" PRIu64 "\n"),
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_repair: handle missing extent states
2017-09-18 16:39 [PATCH] xfs_repair: handle missing extent states Darrick J. Wong
@ 2017-09-18 17:16 ` Eric Sandeen
2017-09-18 17:18 ` Eric Sandeen
2017-09-18 17:30 ` Darrick J. Wong
0 siblings, 2 replies; 6+ messages in thread
From: Eric Sandeen @ 2017-09-18 17:16 UTC (permalink / raw)
To: Darrick J. Wong, Eric Sandeen; +Cc: xfs, amir73il
On 9/18/17 11:39 AM, Darrick J. Wong wrote:
> Missed a couple of the new extent states in the bmbt processing, so add
> them to avoid aborting xfs_repair.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> repair/dinode.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/repair/dinode.c b/repair/dinode.c
> index f817b5a..b35a523 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -796,6 +796,7 @@ _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"),
> case XR_E_FS_MAP:
> case XR_E_INO:
> case XR_E_INUSE_FS:
> + case XR_E_REFC:
> do_warn(
> _("%s fork in inode %" PRIu64 " claims metadata block %" PRIu64 "\n"),
> forkname, ino, b);
I already whined about the naming and use of these XR_E #defines on irc,
but I think that since XR_E_REFC is set in process_rmap_rec when it's found
in the rmap btree, this case should be above the /* fallthrough */ so it says
"rmap claims metadata use!\n" like everything else set there, no?
> @@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
> forkname, ftype, ino, b);
> goto done;
>
> + case XR_E_COW:
> + do_warn(
> +_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
> + forkname, ftype, ino, b);
> + goto done;
> +
why do cow blocks get a special case and custom warning vs the above cases
that just say "metadata?"
Obviously it's just nitpicking over the do_warn message string, just
double checking on the consistency front.
-Eric
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_repair: handle missing extent states
2017-09-18 17:16 ` Eric Sandeen
@ 2017-09-18 17:18 ` Eric Sandeen
2017-09-18 17:30 ` Darrick J. Wong
2017-09-18 17:30 ` Darrick J. Wong
1 sibling, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2017-09-18 17:18 UTC (permalink / raw)
To: Darrick J. Wong, Eric Sandeen; +Cc: xfs, amir73il
On 9/18/17 12:16 PM, Eric Sandeen wrote:
>> @@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
>> forkname, ftype, ino, b);
>> goto done;
>>
>> + case XR_E_COW:
>> + do_warn(
>> +_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
>> + forkname, ftype, ino, b);
>> + goto done;
>> +
> why do cow blocks get a special case and custom warning vs the above cases
> that just say "metadata?"
>
> Obviously it's just nitpicking over the do_warn message string, just
> double checking on the consistency front.
And this was also found via the rmap, but that fact isn't printed like
it is for every other type. For that reason I'd probably rather just add
XR_RE_COW above the /* fallthrough */ too, for consistency. Thoughts?
Thanks,
-Eric
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_repair: handle missing extent states
2017-09-18 17:18 ` Eric Sandeen
@ 2017-09-18 17:30 ` Darrick J. Wong
2017-09-18 17:35 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2017-09-18 17:30 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Eric Sandeen, xfs, amir73il
On Mon, Sep 18, 2017 at 12:18:50PM -0500, Eric Sandeen wrote:
>
>
> On 9/18/17 12:16 PM, Eric Sandeen wrote:
> >> @@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
> >> forkname, ftype, ino, b);
> >> goto done;
> >>
> >> + case XR_E_COW:
> >> + do_warn(
> >> +_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
> >> + forkname, ftype, ino, b);
> >> + goto done;
> >> +
> > why do cow blocks get a special case and custom warning vs the above cases
> > that just say "metadata?"
> >
> > Obviously it's just nitpicking over the do_warn message string, just
> > double checking on the consistency front.
>
> And this was also found via the rmap, but that fact isn't printed like
> it is for every other type. For that reason I'd probably rather just add
> XR_RE_COW above the /* fallthrough */ too, for consistency. Thoughts?
Strictly speaking, CoW blocks aren't metadata; they're, uh, alternate
data. I'd rather have xfs_repair deliver a more specific message about
the other alleged owner of a block so I'd know where to start looking --
"claims CoW blocks" implies that I ought to start looking at the rmapbt
for cow-owned blocks and the refcountbt, vs. just "shares with metadata,
urk".
That said, we probably ought to have the rmapbt scan set XR_E_COW1 and
have the refcountbt scan set XR_E_COW (similar to what we do for things
like INUSE/INUSE1). Will try to work on that for 4.14, but for now
let's at least not abort xfs_repair here.
--D
>
> Thanks,
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_repair: handle missing extent states
2017-09-18 17:30 ` Darrick J. Wong
@ 2017-09-18 17:35 ` Eric Sandeen
0 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2017-09-18 17:35 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Eric Sandeen, xfs, amir73il
On 9/18/17 12:30 PM, Darrick J. Wong wrote:
> On Mon, Sep 18, 2017 at 12:18:50PM -0500, Eric Sandeen wrote:
>>
>>
>> On 9/18/17 12:16 PM, Eric Sandeen wrote:
>>>> @@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
>>>> forkname, ftype, ino, b);
>>>> goto done;
>>>>
>>>> + case XR_E_COW:
>>>> + do_warn(
>>>> +_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
>>>> + forkname, ftype, ino, b);
>>>> + goto done;
>>>> +
>>> why do cow blocks get a special case and custom warning vs the above cases
>>> that just say "metadata?"
>>>
>>> Obviously it's just nitpicking over the do_warn message string, just
>>> double checking on the consistency front.
>>
>> And this was also found via the rmap, but that fact isn't printed like
>> it is for every other type. For that reason I'd probably rather just add
>> XR_RE_COW above the /* fallthrough */ too, for consistency. Thoughts?
>
> Strictly speaking, CoW blocks aren't metadata; they're, uh, alternate
> data.
Oh, fair point. ok.
> I'd rather have xfs_repair deliver a more specific message about
> the other alleged owner of a block so I'd know where to start looking --
> "claims CoW blocks" implies that I ought to start looking at the rmapbt
> for cow-owned blocks and the refcountbt, vs. just "shares with metadata,
> urk".
*nod*
> That said, we probably ought to have the rmapbt scan set XR_E_COW1 and
> have the refcountbt scan set XR_E_COW (similar to what we do for things
> like INUSE/INUSE1). Will try to work on that for 4.14, but for now
> let's at least not abort xfs_repair here.
yep not asking for the world here, just trying to understand and maybe plant
the seeds for future cleanups. ;)
-Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_repair: handle missing extent states
2017-09-18 17:16 ` Eric Sandeen
2017-09-18 17:18 ` Eric Sandeen
@ 2017-09-18 17:30 ` Darrick J. Wong
1 sibling, 0 replies; 6+ messages in thread
From: Darrick J. Wong @ 2017-09-18 17:30 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Eric Sandeen, xfs, amir73il
On Mon, Sep 18, 2017 at 12:16:48PM -0500, Eric Sandeen wrote:
> On 9/18/17 11:39 AM, Darrick J. Wong wrote:
> > Missed a couple of the new extent states in the bmbt processing, so add
> > them to avoid aborting xfs_repair.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > repair/dinode.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/repair/dinode.c b/repair/dinode.c
> > index f817b5a..b35a523 100644
> > --- a/repair/dinode.c
> > +++ b/repair/dinode.c
> > @@ -796,6 +796,7 @@ _("%s fork in ino %" PRIu64 " claims free block %" PRIu64 "\n"),
> > case XR_E_FS_MAP:
> > case XR_E_INO:
> > case XR_E_INUSE_FS:
> > + case XR_E_REFC:
> > do_warn(
> > _("%s fork in inode %" PRIu64 " claims metadata block %" PRIu64 "\n"),
> > forkname, ino, b);
>
> I already whined about the naming and use of these XR_E #defines on irc,
> but I think that since XR_E_REFC is set in process_rmap_rec when it's found
> in the rmap btree, this case should be above the /* fallthrough */ so it says
> "rmap claims metadata use!\n" like everything else set there, no?
Sure.
--D
>
> > @@ -812,6 +813,12 @@ _("%s fork in %s inode %" PRIu64 " claims used block %" PRIu64 "\n"),
> > forkname, ftype, ino, b);
> > goto done;
> >
> > + case XR_E_COW:
> > + do_warn(
> > +_("%s fork in %s inode %" PRIu64 " claims CoW block %" PRIu64 "\n"),
> > + forkname, ftype, ino, b);
> > + goto done;
> > +
>
> why do cow blocks get a special case and custom warning vs the above cases
> that just say "metadata?"
>
> Obviously it's just nitpicking over the do_warn message string, just
> double checking on the consistency front.
>
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-18 17:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-18 16:39 [PATCH] xfs_repair: handle missing extent states Darrick J. Wong
2017-09-18 17:16 ` Eric Sandeen
2017-09-18 17:18 ` Eric Sandeen
2017-09-18 17:30 ` Darrick J. Wong
2017-09-18 17:35 ` Eric Sandeen
2017-09-18 17:30 ` 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).