On Wed, Jun 20, 2018 at 09:22:42AM +1000, Dave Chinner wrote:
> On Tue, Jun 19, 2018 at 12:28:40PM -0400, Brian Foster wrote:
> > On Tue, Jun 19, 2018 at 08:48:10AM -0700, Darrick J. Wong wrote:
> > > On Tue, Jun 19, 2018 at 07:54:34AM -0400, Brian Foster wrote:
> > > > On Tue, Jun 19, 2018 at 08:42:59AM +1000, Dave Chinner wrote:
> > > > Rather, I'm asking about the pre-existing code that we remove. The hunk
> > > > just above from xfs_inactive_symlink():
> > > >
> > > > - /*
> > > > - * Zero length symlinks _can_ exist.
> > > > - */
> > > > - if (!pathlen) {
> > > > - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > > > - return 0;
> > > > - }
> > > >
> > > > ... obviously suggests that there could have been a situation where we'd
> > > > see a zero-length symlink on entry to xfs_inactive_symlink(). This
> > > > patch, however, focuses on fixing the transient zero-length symlink
> > > > caused by xfs_inactive_symlink_rmt() (which comes after the above
> > > > check).
>
> It can't happen through normal VFS paths, and I don't think it can
> happen through log recovery. That's why I replaced this with an ASSERT.
>
> In the normal behaviour case, zero length symlinks were created
> /after/ this point in time, and we've always been unable to read
> such inodes because xfs_dinode_verify() has always rejected zero
> length symlink inodes.
>
That was my initial understanding as well..
> However, log recovery doesn't trip over the transient inode state
> because it does not use xfs_dinode_verify() for inode recovery - it
> reads the buffer with xfs_inode_buf_ops, and that just checks the
> inode numbers and then recovers whatever is in the log over the top.
> It never checks for zero length symlinks on recovery, and it never
> goes through the dinode verifier (on read or write!) to catch this.
>
> It's not until we then recover the remaining intent chain that we go
> through xfs_iget/xfs_iread/xfs_inactive/xfs_ifree, and that
> xfs_iget() call runs xfs_dinode_verify(). If we've already recovered
> any part of the remote symlink removal intent chain (and we must
> have to be replaying EFIs!) this should see a zero length symlink
> inode. AIUI, we have no evidence that the verifier has ever fired at
> this point in time, even when recovering known transient zero length
> states.
>
Hmm, not sure we're talking about the same thing. I ran a quick
experiment to try and clear up my confusion here, at least. I injected a
shutdown at the point inactive creates the zero sized symlink and
created/removed a remote symlink. On a remount, I hit the following
during log recovery:
[ 685.931834] XFS (dm-3): Starting recovery (logdev: internal)
[ 685.993014] XFS (dm-3): Metadata corruption detected at xfs_dinode_verify+0x331/0x490 [xfs], inode 0x85 dinode
[ 685.996287] XFS (dm-3): Unmount and run xfs_repair
[ 685.996911] XFS (dm-3): First 128 bytes of corrupted metadata buffer:
...
[ 686.006647] Call Trace:
[ 686.006922] dump_stack+0x85/0xcb
[ 686.007338] xfs_iread+0xeb/0x220 [xfs]
[ 686.007820] xfs_iget+0x4bd/0x1100 [xfs]
[ 686.008344] xlog_recover_process_one_iunlink+0x4d/0x3c0 [xfs]
...
That seems to show that the verifier can impede unlinked list processing
if the change is at least present in the log.
If I recreate that same dirty log state and mount with this patch
applied (note that the fs is created without this patch to simulate an
old kernel that has not changed i_mode in the same transaction that sets
di_size = 0) along with a hack to avoid the check in
xfs_dinode_verify(), I now hit the new assert and corruption error
that's been placed in xfs_inactive_symlink().
So to Darrick's point, that seems to show that this is a vector to the
pre-existing len == 0 check in xfs_inactive_symlink(). Given that, it
seems to me that if we want to handle recovery from this state, we'd
still need to work around the verifier check and retain the initial
di_size == 0 check in xfs_inactive_symlink().
> i.e all the cases I've seen where repair complains about symlinks
> with "dfork format 2 and size 0" it is because the log is dirty and
> hasn't been replayed. Mounting the filesystem and running log
> recovery makes the problem go away, and this is what lead to me
> removing the zeor length handling - the verifier should already
> be firing if it is recovering an intent on a zero length symlink
> inode...
>
I haven't grokked all the details here, but the behavior you describe
does sound like this might be a different scenario from the above.
Brian
> That said, I'll try some more focussed testing with intentional
> corruption to see if it's just a case of my testing being (un)lucky
> and not triggering this issue...
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
