* [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int
2014-09-07 16:41 [PATCH 0/5] xfs_repair fixes, part 1 Eric Sandeen
@ 2014-09-07 16:41 ` Eric Sandeen
2014-09-08 13:45 ` Brian Foster
2014-09-09 22:28 ` Christoph Hellwig
2014-09-07 16:41 ` [PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr Eric Sandeen
` (4 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Eric Sandeen @ 2014-09-07 16:41 UTC (permalink / raw)
To: xfs
process_dinode_int() reports bad flags if
dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if
any flags are set outside the known set. But
then instead of clearing them, it does
flags &= ~XFS_DIFLAG_ANY which keeps *only*
the bad flags. This leads to persistent,
unrepairable errors of the form:
"Bad flags set in inode XXX"
Fix this.
While we are at it, fix a couple lines which
look like they used to be continuation lines,
but are no longer.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
repair/dinode.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/repair/dinode.c b/repair/dinode.c
index 8891e84..38a6562 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2456,7 +2456,7 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
_("Bad flags set in inode %" PRIu64 "\n"),
lino);
}
- flags &= ~XFS_DIFLAG_ANY;
+ flags &= XFS_DIFLAG_ANY;
}
if (flags & (XFS_DIFLAG_REALTIME | XFS_DIFLAG_RTINHERIT)) {
@@ -2513,11 +2513,11 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
}
if (!verify_mode && flags != be16_to_cpu(dino->di_flags)) {
if (!no_modify) {
- do_warn(_(", fixing bad flags.\n"));
+ do_warn(_("fixing bad flags.\n"));
dino->di_flags = cpu_to_be16(flags);
*dirty = 1;
} else
- do_warn(_(", would fix bad flags.\n"));
+ do_warn(_("would fix bad flags.\n"));
}
}
--
1.7.1
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int
2014-09-07 16:41 ` [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int Eric Sandeen
@ 2014-09-08 13:45 ` Brian Foster
2014-09-09 22:28 ` Christoph Hellwig
1 sibling, 0 replies; 23+ messages in thread
From: Brian Foster @ 2014-09-08 13:45 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Sun, Sep 07, 2014 at 11:41:01AM -0500, Eric Sandeen wrote:
> process_dinode_int() reports bad flags if
> dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if
> any flags are set outside the known set. But
> then instead of clearing them, it does
> flags &= ~XFS_DIFLAG_ANY which keeps *only*
> the bad flags. This leads to persistent,
> unrepairable errors of the form:
>
> "Bad flags set in inode XXX"
>
> Fix this.
>
> While we are at it, fix a couple lines which
> look like they used to be continuation lines,
> but are no longer.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> repair/dinode.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 8891e84..38a6562 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2456,7 +2456,7 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
> _("Bad flags set in inode %" PRIu64 "\n"),
> lino);
> }
> - flags &= ~XFS_DIFLAG_ANY;
> + flags &= XFS_DIFLAG_ANY;
> }
>
> if (flags & (XFS_DIFLAG_REALTIME | XFS_DIFLAG_RTINHERIT)) {
> @@ -2513,11 +2513,11 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
> }
> if (!verify_mode && flags != be16_to_cpu(dino->di_flags)) {
> if (!no_modify) {
> - do_warn(_(", fixing bad flags.\n"));
> + do_warn(_("fixing bad flags.\n"));
> dino->di_flags = cpu_to_be16(flags);
> *dirty = 1;
> } else
> - do_warn(_(", would fix bad flags.\n"));
> + do_warn(_("would fix bad flags.\n"));
> }
> }
>
> --
> 1.7.1
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int
2014-09-07 16:41 ` [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int Eric Sandeen
2014-09-08 13:45 ` Brian Foster
@ 2014-09-09 22:28 ` Christoph Hellwig
2014-09-09 22:33 ` Eric Sandeen
1 sibling, 1 reply; 23+ messages in thread
From: Christoph Hellwig @ 2014-09-09 22:28 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Sun, Sep 07, 2014 at 11:41:01AM -0500, Eric Sandeen wrote:
> process_dinode_int() reports bad flags if
> dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if
> any flags are set outside the known set. But
> then instead of clearing them, it does
> flags &= ~XFS_DIFLAG_ANY which keeps *only*
> the bad flags. This leads to persistent,
> unrepairable errors of the form:
You know you can use up to 75 characters per line for your commit messages,
don't you? :)
Otherwise looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int
2014-09-09 22:28 ` Christoph Hellwig
@ 2014-09-09 22:33 ` Eric Sandeen
2014-09-09 23:48 ` Dave Chinner
0 siblings, 1 reply; 23+ messages in thread
From: Eric Sandeen @ 2014-09-09 22:33 UTC (permalink / raw)
To: Christoph Hellwig, Eric Sandeen; +Cc: xfs
On 9/9/14 5:28 PM, Christoph Hellwig wrote:
> On Sun, Sep 07, 2014 at 11:41:01AM -0500, Eric Sandeen wrote:
>> process_dinode_int() reports bad flags if
>> dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if
>> any flags are set outside the known set. But
>> then instead of clearing them, it does
>> flags &= ~XFS_DIFLAG_ANY which keeps *only*
>> the bad flags. This leads to persistent,
>> unrepairable errors of the form:
>
> You know you can use up to 75 characters per line for your commit messages,
> don't you? :)
hah, it's not automated at all, I guess my visual perception
of the window is shrinking. Dave, feel free to fix on commit if
inclined :)
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int
2014-09-09 22:33 ` Eric Sandeen
@ 2014-09-09 23:48 ` Dave Chinner
0 siblings, 0 replies; 23+ messages in thread
From: Dave Chinner @ 2014-09-09 23:48 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Christoph Hellwig, Eric Sandeen, xfs
On Tue, Sep 09, 2014 at 05:33:30PM -0500, Eric Sandeen wrote:
> On 9/9/14 5:28 PM, Christoph Hellwig wrote:
> >On Sun, Sep 07, 2014 at 11:41:01AM -0500, Eric Sandeen wrote:
> >>process_dinode_int() reports bad flags if
> >>dino->di_flags & ~XFS_DIFLAG_ANY - i.e. if
> >>any flags are set outside the known set. But
> >>then instead of clearing them, it does
> >>flags &= ~XFS_DIFLAG_ANY which keeps *only*
> >>the bad flags. This leads to persistent,
> >>unrepairable errors of the form:
> >
> >You know you can use up to 75 characters per line for your commit messages,
> >don't you? :)
>
> hah, it's not automated at all, I guess my visual perception
> of the window is shrinking. Dave, feel free to fix on commit if
> inclined :)
I mostly do already - I tend to reflow commit messages to 68
characters (same width I use for email) when I see something like
this.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr
2014-09-07 16:41 [PATCH 0/5] xfs_repair fixes, part 1 Eric Sandeen
2014-09-07 16:41 ` [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int Eric Sandeen
@ 2014-09-07 16:41 ` Eric Sandeen
2014-09-08 13:45 ` Brian Foster
2014-09-09 22:29 ` Christoph Hellwig
2014-09-07 16:41 ` [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt Eric Sandeen
` (3 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Eric Sandeen @ 2014-09-07 16:41 UTC (permalink / raw)
To: xfs
process_shortform_attr uses the "junkit" error to
track whether an error was found, but by assigning
it directly to the result of valuecheck, previous
errors are ignored, leading to unrepairable errors
of the form i.e.
"entry has INCOMPLETE flag on in shortform attribute"
or
"entry contains illegal character in shortform attribute name"
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
repair/attr_repair.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index a27a3ec..d60b664 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -914,7 +914,8 @@ process_shortform_attr(
/* Only check values for root security attributes */
if (currententry->flags & XFS_ATTR_ROOT)
- junkit = valuecheck(mp, (char *)¤tentry->nameval[0],
+ junkit |= valuecheck(mp,
+ (char *)¤tentry->nameval[0],
NULL, currententry->namelen,
currententry->valuelen);
--
1.7.1
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr
2014-09-07 16:41 ` [PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr Eric Sandeen
@ 2014-09-08 13:45 ` Brian Foster
2014-09-09 22:29 ` Christoph Hellwig
1 sibling, 0 replies; 23+ messages in thread
From: Brian Foster @ 2014-09-08 13:45 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Sun, Sep 07, 2014 at 11:41:02AM -0500, Eric Sandeen wrote:
> process_shortform_attr uses the "junkit" error to
> track whether an error was found, but by assigning
> it directly to the result of valuecheck, previous
> errors are ignored, leading to unrepairable errors
> of the form i.e.
>
> "entry has INCOMPLETE flag on in shortform attribute"
> or
> "entry contains illegal character in shortform attribute name"
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> repair/attr_repair.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> index a27a3ec..d60b664 100644
> --- a/repair/attr_repair.c
> +++ b/repair/attr_repair.c
> @@ -914,7 +914,8 @@ process_shortform_attr(
>
> /* Only check values for root security attributes */
> if (currententry->flags & XFS_ATTR_ROOT)
> - junkit = valuecheck(mp, (char *)¤tentry->nameval[0],
> + junkit |= valuecheck(mp,
> + (char *)¤tentry->nameval[0],
> NULL, currententry->namelen,
> currententry->valuelen);
>
> --
> 1.7.1
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr
2014-09-07 16:41 ` [PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr Eric Sandeen
2014-09-08 13:45 ` Brian Foster
@ 2014-09-09 22:29 ` Christoph Hellwig
1 sibling, 0 replies; 23+ messages in thread
From: Christoph Hellwig @ 2014-09-09 22:29 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Sun, Sep 07, 2014 at 11:41:02AM -0500, Eric Sandeen wrote:
> process_shortform_attr uses the "junkit" error to
> track whether an error was found, but by assigning
> it directly to the result of valuecheck, previous
> errors are ignored, leading to unrepairable errors
> of the form i.e.
>
> "entry has INCOMPLETE flag on in shortform attribute"
> or
> "entry contains illegal character in shortform attribute name"
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt
2014-09-07 16:41 [PATCH 0/5] xfs_repair fixes, part 1 Eric Sandeen
2014-09-07 16:41 ` [PATCH 1/5] xfs_repair: clear bad flgs in process_dinode_int Eric Sandeen
2014-09-07 16:41 ` [PATCH 2/5] xfs_repair: preserve error state in process_shortform_attr Eric Sandeen
@ 2014-09-07 16:41 ` Eric Sandeen
2014-09-08 13:45 ` Brian Foster
2014-09-07 16:41 ` [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype Eric Sandeen
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Eric Sandeen @ 2014-09-07 16:41 UTC (permalink / raw)
To: xfs
In phase 6's longform_dir2_entry_check, if we never
find a '.' entry we never add a reference to that entry;
if we subsequently rebuild it, '.' gets added, but
no ref to it is ever made. This leads to Phase 7 doing
i.e.:
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 2 to 1
and the next run will do:
Phase 7 - verify and correct link counts...
resetting inode 5184 nlinks from 1 to 2
So if '.' was never found, but the directory got
rebuilt, manually add the ref for it.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
repair/phase6.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/repair/phase6.c b/repair/phase6.c
index f13069f..cc36a9c 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2288,6 +2288,12 @@ out_fix:
if (bplist[i])
libxfs_putbuf(bplist[i]);
longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
+ /*
+ * If we didn't find a dot, we never added a ref for it;
+ * it's there now after the rebuild, so mark it as reached.
+ */
+ if (*need_dot)
+ add_inode_ref(irec, ino_offset);
*num_illegal = 0;
*need_dot = 0;
} else {
--
1.7.1
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt
2014-09-07 16:41 ` [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt Eric Sandeen
@ 2014-09-08 13:45 ` Brian Foster
2014-09-08 14:25 ` Brian Foster
2014-09-08 14:33 ` Eric Sandeen
0 siblings, 2 replies; 23+ messages in thread
From: Brian Foster @ 2014-09-08 13:45 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Sun, Sep 07, 2014 at 11:41:03AM -0500, Eric Sandeen wrote:
> In phase 6's longform_dir2_entry_check, if we never
> find a '.' entry we never add a reference to that entry;
> if we subsequently rebuild it, '.' gets added, but
> no ref to it is ever made. This leads to Phase 7 doing
> i.e.:
>
> Phase 7 - verify and correct link counts...
> resetting inode 5184 nlinks from 2 to 1
>
> and the next run will do:
>
> Phase 7 - verify and correct link counts...
> resetting inode 5184 nlinks from 1 to 2
>
> So if '.' was never found, but the directory got
> rebuilt, manually add the ref for it.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> repair/phase6.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/repair/phase6.c b/repair/phase6.c
> index f13069f..cc36a9c 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -2288,6 +2288,12 @@ out_fix:
> if (bplist[i])
> libxfs_putbuf(bplist[i]);
> longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
> + /*
> + * If we didn't find a dot, we never added a ref for it;
> + * it's there now after the rebuild, so mark it as reached.
> + */
> + if (*need_dot)
> + add_inode_ref(irec, ino_offset);
So if I follow this correctly, we iterate through the dir, add each name
to the hashtable and handle the inode reference count in the first
longform_dir2_entry_check() loop. If something is wrong, we call
longform_dir2_rebuild() to rebuild the dir from the hashtable of
names/inodes. We may or may not have added a reference for dot at that
point, and need_dot is set appropriately.
This seems Ok, but where is the dot entry actually added? Hmm, I see
that we handle dot in the longform_dir2_rebuild() loop by just skipping
over it...
Brian
> *num_illegal = 0;
> *need_dot = 0;
> } else {
> --
> 1.7.1
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt
2014-09-08 13:45 ` Brian Foster
@ 2014-09-08 14:25 ` Brian Foster
2014-09-08 14:44 ` Eric Sandeen
2014-09-08 14:33 ` Eric Sandeen
1 sibling, 1 reply; 23+ messages in thread
From: Brian Foster @ 2014-09-08 14:25 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Mon, Sep 08, 2014 at 09:45:25AM -0400, Brian Foster wrote:
> On Sun, Sep 07, 2014 at 11:41:03AM -0500, Eric Sandeen wrote:
> > In phase 6's longform_dir2_entry_check, if we never
> > find a '.' entry we never add a reference to that entry;
> > if we subsequently rebuild it, '.' gets added, but
> > no ref to it is ever made. This leads to Phase 7 doing
> > i.e.:
> >
> > Phase 7 - verify and correct link counts...
> > resetting inode 5184 nlinks from 2 to 1
> >
> > and the next run will do:
> >
> > Phase 7 - verify and correct link counts...
> > resetting inode 5184 nlinks from 1 to 2
> >
> > So if '.' was never found, but the directory got
> > rebuilt, manually add the ref for it.
> >
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> > repair/phase6.c | 6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/repair/phase6.c b/repair/phase6.c
> > index f13069f..cc36a9c 100644
> > --- a/repair/phase6.c
> > +++ b/repair/phase6.c
> > @@ -2288,6 +2288,12 @@ out_fix:
> > if (bplist[i])
> > libxfs_putbuf(bplist[i]);
> > longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
> > + /*
> > + * If we didn't find a dot, we never added a ref for it;
> > + * it's there now after the rebuild, so mark it as reached.
> > + */
> > + if (*need_dot)
> > + add_inode_ref(irec, ino_offset);
>
> So if I follow this correctly, we iterate through the dir, add each name
> to the hashtable and handle the inode reference count in the first
> longform_dir2_entry_check() loop. If something is wrong, we call
> longform_dir2_rebuild() to rebuild the dir from the hashtable of
> names/inodes. We may or may not have added a reference for dot at that
> point, and need_dot is set appropriately.
>
> This seems Ok, but where is the dot entry actually added? Hmm, I see
> that we handle dot in the longform_dir2_rebuild() loop by just skipping
> over it...
>
It looks like this happens in process_dir_inode() after this whole
check/rebuild sequence, directory format permitting. There's also an
add_inode_ref() there. Perhaps the bug here is that we clear need_dot
when we shouldn't..?
Brian
> Brian
>
>
> > *num_illegal = 0;
> > *need_dot = 0;
> > } else {
> > --
> > 1.7.1
> >
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt
2014-09-08 14:25 ` Brian Foster
@ 2014-09-08 14:44 ` Eric Sandeen
0 siblings, 0 replies; 23+ messages in thread
From: Eric Sandeen @ 2014-09-08 14:44 UTC (permalink / raw)
To: Brian Foster, Eric Sandeen; +Cc: xfs
On 9/8/14 9:25 AM, Brian Foster wrote:
> On Mon, Sep 08, 2014 at 09:45:25AM -0400, Brian Foster wrote:
>> On Sun, Sep 07, 2014 at 11:41:03AM -0500, Eric Sandeen wrote:
>>> In phase 6's longform_dir2_entry_check, if we never
>>> find a '.' entry we never add a reference to that entry;
>>> if we subsequently rebuild it, '.' gets added, but
>>> no ref to it is ever made. This leads to Phase 7 doing
>>> i.e.:
>>>
>>> Phase 7 - verify and correct link counts...
>>> resetting inode 5184 nlinks from 2 to 1
>>>
>>> and the next run will do:
>>>
>>> Phase 7 - verify and correct link counts...
>>> resetting inode 5184 nlinks from 1 to 2
>>>
>>> So if '.' was never found, but the directory got
>>> rebuilt, manually add the ref for it.
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>> repair/phase6.c | 6 ++++++
>>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/repair/phase6.c b/repair/phase6.c
>>> index f13069f..cc36a9c 100644
>>> --- a/repair/phase6.c
>>> +++ b/repair/phase6.c
>>> @@ -2288,6 +2288,12 @@ out_fix:
>>> if (bplist[i])
>>> libxfs_putbuf(bplist[i]);
>>> longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
>>> + /*
>>> + * If we didn't find a dot, we never added a ref for it;
>>> + * it's there now after the rebuild, so mark it as reached.
>>> + */
>>> + if (*need_dot)
>>> + add_inode_ref(irec, ino_offset);
>>
>> So if I follow this correctly, we iterate through the dir, add each name
>> to the hashtable and handle the inode reference count in the first
>> longform_dir2_entry_check() loop. If something is wrong, we call
>> longform_dir2_rebuild() to rebuild the dir from the hashtable of
>> names/inodes. We may or may not have added a reference for dot at that
>> point, and need_dot is set appropriately.
>>
>> This seems Ok, but where is the dot entry actually added? Hmm, I see
>> that we handle dot in the longform_dir2_rebuild() loop by just skipping
>> over it...
>>
>
> It looks like this happens in process_dir_inode() after this whole
> check/rebuild sequence, directory format permitting. There's also an
> add_inode_ref() there. Perhaps the bug here is that we clear need_dot
> when we shouldn't..?
If we do that, the first run says:
bad hash table for directory inode 5184 (no data entry): rebuilding
rebuilding directory inode 5184
creating missing "." entry in dir ino 5184
and then the 2nd run says:
multiple . entries in directory inode 5184: clearing entry
so, no. ;)
The issue is that add_inode_ref() is keeping track (in repair)
of reached paths to the inode, in counted_nlinks.
If we didn't find '.' originally, we didn't add that ref.
When we do:
longform_dir2_rebuild
xfs_dir_init() // creates shortform
<loop over names>
xfs_dir_createname
xfs_dir2_sf_to_block when it's big enough
add '.' entry
and then we've added the '.' but haven't added the reference repair needs
internally.
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt
2014-09-08 13:45 ` Brian Foster
2014-09-08 14:25 ` Brian Foster
@ 2014-09-08 14:33 ` Eric Sandeen
1 sibling, 0 replies; 23+ messages in thread
From: Eric Sandeen @ 2014-09-08 14:33 UTC (permalink / raw)
To: Brian Foster, Eric Sandeen; +Cc: xfs
On 9/8/14 8:45 AM, Brian Foster wrote:
> On Sun, Sep 07, 2014 at 11:41:03AM -0500, Eric Sandeen wrote:
>> In phase 6's longform_dir2_entry_check, if we never
>> find a '.' entry we never add a reference to that entry;
>> if we subsequently rebuild it, '.' gets added, but
>> no ref to it is ever made. This leads to Phase 7 doing
>> i.e.:
>>
>> Phase 7 - verify and correct link counts...
>> resetting inode 5184 nlinks from 2 to 1
>>
>> and the next run will do:
>>
>> Phase 7 - verify and correct link counts...
>> resetting inode 5184 nlinks from 1 to 2
>>
>> So if '.' was never found, but the directory got
>> rebuilt, manually add the ref for it.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>> repair/phase6.c | 6 ++++++
>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/repair/phase6.c b/repair/phase6.c
>> index f13069f..cc36a9c 100644
>> --- a/repair/phase6.c
>> +++ b/repair/phase6.c
>> @@ -2288,6 +2288,12 @@ out_fix:
>> if (bplist[i])
>> libxfs_putbuf(bplist[i]);
>> longform_dir2_rebuild(mp, ino, ip, irec, ino_offset, hashtab);
>> + /*
>> + * If we didn't find a dot, we never added a ref for it;
>> + * it's there now after the rebuild, so mark it as reached.
>> + */
>> + if (*need_dot)
>> + add_inode_ref(irec, ino_offset);
>
> So if I follow this correctly, we iterate through the dir, add each name
> to the hashtable and handle the inode reference count in the first
> longform_dir2_entry_check() loop. If something is wrong, we call
> longform_dir2_rebuild() to rebuild the dir from the hashtable of
> names/inodes. We may or may not have added a reference for dot at that
> point, and need_dot is set appropriately.
>
> This seems Ok, but where is the dot entry actually added? Hmm, I see
> that we handle dot in the longform_dir2_rebuild() loop by just skipping
> over it...
longform_dir2_rebuild calls this before the loop:
/*
* Initialize a directory with its "." and ".." entries.
*/
int
xfs_dir_init()
But it doesn't actually create .; this creates a shortform dir,
and shortform dirs have no '.' - I guess the comment could use
an update.
In the loop we call xfs_dir_createname() for everything in the hash,
eventually things don't fit in shortform and we do xfs_dir2_sf_to_block,
which creates the dot:
/*
* Create entry for .
*/
dep = xfs_dir3_data_dot_entry_p(mp, hdr);
dep->inumber = cpu_to_be64(dp->i_ino);
dep->namelen = 1;
dep->name[0] = '.';
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype
2014-09-07 16:41 [PATCH 0/5] xfs_repair fixes, part 1 Eric Sandeen
` (2 preceding siblings ...)
2014-09-07 16:41 ` [PATCH 3/5] xfs_repair: fix dir refcount when '.' missing and dir is rebuilt Eric Sandeen
@ 2014-09-07 16:41 ` Eric Sandeen
2014-09-08 0:10 ` Dave Chinner
2014-09-07 16:41 ` [PATCH 5/5] xfs_repair: set proper ftype when moving to lost+found Eric Sandeen
2014-09-07 17:02 ` [PATCH 6/5] xfs_repair: don't re-add root dotdot if root dir was rebuilt Eric Sandeen
5 siblings, 1 reply; 23+ messages in thread
From: Eric Sandeen @ 2014-09-07 16:41 UTC (permalink / raw)
To: xfs
xfs_dir3_dirent_get_ftype() gets the file type
off disk, but ASSERTs if it's invalid:
ASSERT(type < XFS_DIR3_FT_MAX);
This might be cut & paste from
xfs_dir3_dirent_put_ftype which should be checking
that it's not been passed bad values, but we
shouldn't ASSERT on bad values read from disk.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
include/xfs_da_format.h | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
index 89a1a21..11f1420 100644
--- a/include/xfs_da_format.h
+++ b/include/xfs_da_format.h
@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
if (xfs_sb_version_hasftype(&mp->m_sb)) {
__uint8_t type = dep->name[dep->namelen];
- ASSERT(type < XFS_DIR3_FT_MAX);
if (type < XFS_DIR3_FT_MAX)
return type;
--
1.7.1
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype
2014-09-07 16:41 ` [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype Eric Sandeen
@ 2014-09-08 0:10 ` Dave Chinner
2014-09-08 1:02 ` Eric Sandeen
0 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2014-09-08 0:10 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
On Sun, Sep 07, 2014 at 11:41:04AM -0500, Eric Sandeen wrote:
> xfs_dir3_dirent_get_ftype() gets the file type
> off disk, but ASSERTs if it's invalid:
>
> ASSERT(type < XFS_DIR3_FT_MAX);
>
> This might be cut & paste from
> xfs_dir3_dirent_put_ftype which should be checking
> that it's not been passed bad values, but we
> shouldn't ASSERT on bad values read from disk.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> include/xfs_da_format.h | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
> index 89a1a21..11f1420 100644
> --- a/include/xfs_da_format.h
> +++ b/include/xfs_da_format.h
> @@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
> if (xfs_sb_version_hasftype(&mp->m_sb)) {
> __uint8_t type = dep->name[dep->namelen];
>
> - ASSERT(type < XFS_DIR3_FT_MAX);
> if (type < XFS_DIR3_FT_MAX)
> return type;
Needs to be fixed kernel-side first.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype
2014-09-08 0:10 ` Dave Chinner
@ 2014-09-08 1:02 ` Eric Sandeen
2014-09-08 3:16 ` Dave Chinner
0 siblings, 1 reply; 23+ messages in thread
From: Eric Sandeen @ 2014-09-08 1:02 UTC (permalink / raw)
To: Dave Chinner, Eric Sandeen; +Cc: xfs
On 9/7/14 7:10 PM, Dave Chinner wrote:
> On Sun, Sep 07, 2014 at 11:41:04AM -0500, Eric Sandeen wrote:
...
>> diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
>> index 89a1a21..11f1420 100644
>> --- a/include/xfs_da_format.h
>> +++ b/include/xfs_da_format.h
>> @@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
>> if (xfs_sb_version_hasftype(&mp->m_sb)) {
>> __uint8_t type = dep->name[dep->namelen];
>>
>> - ASSERT(type < XFS_DIR3_FT_MAX);
>> if (type < XFS_DIR3_FT_MAX)
>> return type;
>
> Needs to be fixed kernel-side first.
xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)
bleah, why do they have different names... Ok, will send.
-Eric
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype
2014-09-08 1:02 ` Eric Sandeen
@ 2014-09-08 3:16 ` Dave Chinner
2014-09-08 3:18 ` Eric Sandeen
0 siblings, 1 reply; 23+ messages in thread
From: Dave Chinner @ 2014-09-08 3:16 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Eric Sandeen, xfs
On Sun, Sep 07, 2014 at 08:02:20PM -0500, Eric Sandeen wrote:
> On 9/7/14 7:10 PM, Dave Chinner wrote:
> >On Sun, Sep 07, 2014 at 11:41:04AM -0500, Eric Sandeen wrote:
>
> ...
>
> >>diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
> >>index 89a1a21..11f1420 100644
> >>--- a/include/xfs_da_format.h
> >>+++ b/include/xfs_da_format.h
> >>@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
> >> if (xfs_sb_version_hasftype(&mp->m_sb)) {
> >> __uint8_t type = dep->name[dep->namelen];
> >>
> >>- ASSERT(type < XFS_DIR3_FT_MAX);
> >> if (type < XFS_DIR3_FT_MAX)
> >> return type;
> >
> >Needs to be fixed kernel-side first.
>
> xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)
>
> bleah, why do they have different names... Ok, will send.
Because kernel has changed, and we need to do yet another large
kernel/user libxfs sync.
I haven't found time to do that yet. Unless you want to volunteer
for it....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype
2014-09-08 3:16 ` Dave Chinner
@ 2014-09-08 3:18 ` Eric Sandeen
2014-09-08 6:23 ` Dave Chinner
0 siblings, 1 reply; 23+ messages in thread
From: Eric Sandeen @ 2014-09-08 3:18 UTC (permalink / raw)
To: Dave Chinner; +Cc: Eric Sandeen, xfs
On 9/7/14 10:16 PM, Dave Chinner wrote:
> On Sun, Sep 07, 2014 at 08:02:20PM -0500, Eric Sandeen wrote:
>> On 9/7/14 7:10 PM, Dave Chinner wrote:
...
>>> Needs to be fixed kernel-side first.
>>
>> xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)
>>
>> bleah, why do they have different names... Ok, will send.
>
> Because kernel has changed, and we need to do yet another large
> kernel/user libxfs sync.
>
> I haven't found time to do that yet. Unless you want to volunteer
> for it....
I could give it a shot. Do you usually do it commit-by-commit,
diff-and-edit, or a big copy?
-Eric
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype
2014-09-08 3:18 ` Eric Sandeen
@ 2014-09-08 6:23 ` Dave Chinner
0 siblings, 0 replies; 23+ messages in thread
From: Dave Chinner @ 2014-09-08 6:23 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Eric Sandeen, xfs
On Sun, Sep 07, 2014 at 10:18:09PM -0500, Eric Sandeen wrote:
> On 9/7/14 10:16 PM, Dave Chinner wrote:
> >On Sun, Sep 07, 2014 at 08:02:20PM -0500, Eric Sandeen wrote:
> >>On 9/7/14 7:10 PM, Dave Chinner wrote:
>
> ...
>
> >>>Needs to be fixed kernel-side first.
> >>
> >>xfs_dir3_dirent_get_ftype doesn't exist in kernelspace :)
> >>
> >>bleah, why do they have different names... Ok, will send.
> >
> >Because kernel has changed, and we need to do yet another large
> >kernel/user libxfs sync.
> >
> >I haven't found time to do that yet. Unless you want to volunteer
> >for it....
>
> I could give it a shot. Do you usually do it commit-by-commit,
> diff-and-edit, or a big copy?
This one is going to need multiple phases, and in a moment you're
going to understand what all functions called into libxfs should
have a "libxfs define wrapper":
1. sync up to kernel commit before error negation
2. make sure all external function calls have libxfs
define wrappers
3. commit error negation patch and change sign of all libxfs
define wrappers
4. apply kernel libxfs rename patch and restructure
libxfs userspace and xfsprogs build to match
5. continue syncing kernel changes commit by commit.
Given the scope of changes, and the fact that some of the changes
have already been applied (i.e. bug fixes, finobt changes, etc)
step #1 is not going to be a straight forward commit-by-commit.
So I'm happy to do that as a bulk commit. Steps #2 and #3 are
relatively straight forward, too, just a bit painful as we need
to be sure we don't leak negative errors.
Step #4 is the big one; we want to end up with the kernel
fs/xfs/libxfs to be identical to the xfsprogs:/libxfs/ code and so
in userspace we've got to move headers around and rejig includes and
things like that. That's one I should probably do.
And from there, step 5 should be a simple extract/sed/commit process
of pulling patch by patch from the kernel changes that touch
fs/xfs/libxfs....
So, really, the big step right now is #1...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/5] xfs_repair: set proper ftype when moving to lost+found
2014-09-07 16:41 [PATCH 0/5] xfs_repair fixes, part 1 Eric Sandeen
` (3 preceding siblings ...)
2014-09-07 16:41 ` [PATCH 4/5] xfs_repair: don't ASSERT on corrupt ftype Eric Sandeen
@ 2014-09-07 16:41 ` Eric Sandeen
2014-09-07 21:26 ` Eric Sandeen
2014-09-07 17:02 ` [PATCH 6/5] xfs_repair: don't re-add root dotdot if root dir was rebuilt Eric Sandeen
5 siblings, 1 reply; 23+ messages in thread
From: Eric Sandeen @ 2014-09-07 16:41 UTC (permalink / raw)
To: xfs
When we move files to lost+found, we're setting the
filetype to UNKNOWN. This leaves an inconsistency which
is discovered on a subsequent repair:
would fix ftype mismatch (0/1) in directory/child inode 5838/5839
Setting the proper ftype at the time of the move
resolves this:
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
repair/phase6.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/repair/phase6.c b/repair/phase6.c
index cc36a9c..02714c2 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1091,14 +1091,13 @@ mv_orphanage(
ino_tree_node_t *irec;
int ino_offset = 0;
struct xfs_name xname;
+ __uint16_t di_mode;
ASSERT(xfs_sb_version_hasdirv2(&mp->m_sb));
xname.name = fname;
xname.len = snprintf((char *)fname, sizeof(fname), "%llu",
(unsigned long long)ino);
- /* XXX use xfs_mode_to_ftype[] when userspace gains it */
- xname.type = XFS_DIR3_FT_UNKNOWN;
err = libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip, 0);
if (err)
@@ -1117,6 +1116,10 @@ mv_orphanage(
if ((err = libxfs_iget(mp, NULL, ino, 0, &ino_p, 0)))
do_error(_("%d - couldn't iget disconnected inode\n"), err);
+ di_mode = ino_p->i_d.di_mode;
+ di_mode = (di_mode & S_IFMT) >> S_SHIFT;
+ xname.type = xfs_mode_to_ftype[di_mode];
+
if (isa_dir) {
irec = find_inode_rec(mp, XFS_INO_TO_AGNO(mp, orphanage_ino),
XFS_INO_TO_AGINO(mp, orphanage_ino));
--
1.7.1
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 6/5] xfs_repair: don't re-add root dotdot if root dir was rebuilt
2014-09-07 16:41 [PATCH 0/5] xfs_repair fixes, part 1 Eric Sandeen
` (4 preceding siblings ...)
2014-09-07 16:41 ` [PATCH 5/5] xfs_repair: set proper ftype when moving to lost+found Eric Sandeen
@ 2014-09-07 17:02 ` Eric Sandeen
5 siblings, 0 replies; 23+ messages in thread
From: Eric Sandeen @ 2014-09-07 17:02 UTC (permalink / raw)
To: xfs
If we've rebuilt the root directory, ".." was taken
care of, so clear need_root_dotdot.
Otherwise it will be added twice, and a subsequent repair
will say:
entry ".." (ino 5824) in dir 5824 is a duplicate name, would junk entry
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
(sorry for 6/5, this just popped out and is similar to the
patch 3/5 I just sent, so probably worth doing at the same time.
diff --git a/repair/phase6.c b/repair/phase6.c
index cc36a9c..2e67c60 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2294,6 +2297,9 @@ out_fix:
*/
if (*need_dot)
add_inode_ref(irec, ino_offset);
+ /* If we rebuilt the root dir, dot dot is in good shape */
+ if (ino == mp->m_sb.sb_rootino)
+ need_root_dotdot = 0;
*num_illegal = 0;
*need_dot = 0;
} else {
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 23+ messages in thread