On Mon, May 11, 2020 at 09:44:21AM -0700, Darrick J. Wong wrote:
> On Sat, May 09, 2020 at 10:08:50AM -0700, Christoph Hellwig wrote:
> > On Sat, May 09, 2020 at 09:29:58AM -0700, Darrick J. Wong wrote:
> > > @@ -140,11 +140,24 @@ _("can't read %s block %u for inode %" PRIu64 "\n"),
> > > if (whichfork == XFS_DATA_FORK &&
> > > (nodehdr.magic == XFS_DIR2_LEAFN_MAGIC ||
> > > nodehdr.magic == XFS_DIR3_LEAFN_MAGIC)) {
> > > + int bad = 0;
> > > +
> > > if (i != -1) {
> > > do_warn(
> > > _("found non-root LEAFN node in inode %" PRIu64 " bno = %u\n"),
> > > da_cursor->ino, bno);
> > > + bad++;
> > > }
> > > +
> > > + /* corrupt leafn node; rebuild the dir. */
> > > + if (!bad &&
> > > + (bp->b_error == -EFSBADCRC ||
> > > + bp->b_error == -EFSCORRUPTED)) {
> > > + do_warn(
> > > +_("corrupt %s LEAFN block %u for inode %" PRIu64 "\n"),
> > > + FORKNAME(whichfork), bno, da_cursor->ino);
> > > + }
> > > +
> >
> > So this doesn't really change any return value, but just the error
> > message. But looking at this code I wonder why we don't check
> > b_error first thing after reading the buffer, as checking the magic
> > for a corrupt buffer seems a little pointless.
>
> <shrug> In the first hunk I was merely following what we did for DA_NODE
> blocks (check magic, then check for corruption errors) but I guess I
> could just pull that up in the file. I'll have a look and see what
> happens if I do that.
...and on re-examination, the other da_read_buf callers in repair/dir2.c
ought to look for EFSCORRUPTED (they already look for EFSBADCRC) and
complain about that. Seeing as the directory salvage is done separately
in phase6.c, I guess there's no harm in jumping out early in phases 3/4.
--D
> --D
