[PATCH] xfs: di_flushiter considered harmful

[Markus Trippelsdorf]

On 2013.07.22 at 20:18 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner at redhat.com>
> 
> When we made all inode updates transactional, we no longer needed
> the log recovery detection for inodes being newer on disk than the
> transaction being replayed - it was redundant as replay of the log
> would always result in the latest version of the inode woul dbe on
> disk. It was redundant, but left in place because it wasn't
> considered to be a problem.
> 
> However, with the new "don't read inodes on create" optimisation,
> flushiter has come back to bite us. Essentially, the optimisation
> made always initialises flushiter to zero in the create transaction,
> and so if we then crash and run recovery and the inode already on
> disk has a non-zero flushiter it will skip recovery of that inode.
> As a result, log recovery does the wrong thing and we end up with a
> corrupt filesystem.
> 
> Because we have to support old kernel to new kernl upgrades, we
> can't just get rid of the flushiter support in log recovery as we
> might be upgrading from a kernel that doesn't have fully transaction
> inode updates.  Unfortunately, for v4 superblocks there is no way to
> guarantee that log recovery knows about this fact.
> 
> We cannot add a new inode format flag to say it's a "special inode
> create" because it won't be understood by older kernels and so
> recovery could do the wrong thing on downgrade. We cannot specially
> detect the combination of zero mode/non-zero flushiter on disk to
> non-zero mode, zero flushiter in the log item during recovery
> because wrapping of the flushiter can result in false detection.
> 
> Hence that makes this "don't use flushiter" optimisation limited to
> a disk format that guarantees that we don't need it. And that means
> the only fix here is to limit the "no read IO on create"
> optimisation to version 5 superblocks....

I think your patch misses the following part:

@@ -1054,17 +1056,15 @@ xfs_iread(
 
        /* shortcut IO on inode allocation if possible */
        if ((iget_flags & XFS_IGET_CREATE) &&
-           !(mp->m_flags & XFS_MOUNT_IKEEP)) {
+           !(mp->m_flags & XFS_MOUNT_IKEEP) &&
+           xfs_sb_version_hascrc(&mp->m_sb)) {
                /* initialise the on-disk inode core */
                memset(&ip->i_d, 0, sizeof(ip->i_d));
                ip->i_d.di_magic = XFS_DINODE_MAGIC;
                ip->i_d.di_gen = prandom_u32();
-               if (xfs_sb_version_hascrc(&mp->m_sb)) {
-                       ip->i_d.di_version = 3;
-                       ip->i_d.di_ino = ip->i_ino;
-                       uuid_copy(&ip->i_d.di_uuid, &mp->m_sb.sb_uuid);
-               } else
-                       ip->i_d.di_version = 2;
+               ip->i_d.di_version = 3;
+               ip->i_d.di_ino = ip->i_ino;
+               uuid_copy(&ip->i_d.di_uuid, &mp->m_sb.sb_uuid);
                return 0;
        }
 


-- 
Markus