xfs_scrub: fall back to scrub-by-handle if opening handles fails

Back when I originally wrote xfs_scrub, I decided that phase 3 (the file
scrubber) should try to open all regular files by handle to pin the file
during the scrub.  At the time, I decided that an ESTALE return value
should cause the entire inode group (aka an inobt record) to be
rescanned for thoroughness reasons.

Over the past four years, I've realized that checking the return value
here isn't necessary.  For most runtime errors, we already fall back to
scrubbing with the file handle, at a fairly small performance cost.

For ESTALE, if the file has been freed and reallocated, its metadata has
been rewritten completely since bulkstat, so it's not necessary to check
it for latent disk errors.  If the file was freed, we can simply move on
to the next file.  If the filesystem is corrupt enough that
open-by-handle fails (this also results in ESTALE), we actually /want/
to fall back to scrubbing that file by handle, not rescanning the entire
inode group.

Therefore, remove the ESTALE check and leave a comment detailing these
findings.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

diff --git a/scrub/phase3.c b/scrub/phase3.c
index 7da112999e7949ee30c931da1f7ded31eb158274..d22758c1dc0f9c922cac97c592fae273433b1e3b 100644 (file)
--- a/scrub/phase3.c
+++ b/scrub/phase3.c
@@ -65,18 +65,26 @@ scrub_inode(
         * cost of opening the handle (looking up the inode in the inode btree,
         * grabbing the inode, checking the generation) with every scrub call.
         *
+        * Ignore any runtime or corruption related errors here because we can
+        * fall back to scrubbing by handle.  ESTALE can be ignored for the
+        * following reasons:
+        *
+        *  - If the file has been deleted since bulkstat, there's nothing to
+        *    check.  Scrub-by-handle returns ENOENT for such inodes.
+        *  - If the file has been deleted and reallocated since bulkstat,
+        *    its ondisk metadata have been rewritten and is assumed to be ok.
+        *    Scrub-by-handle also returns ENOENT if the generation doesn't
+        *    match.
+        *  - The file itself is corrupt and cannot be loaded.  In this case,
+        *    we fall back to scrub-by-handle.
+        *
         * Note: We cannot use this same trick for directories because the VFS
         * will try to reconnect directory file handles to the root directory
         * by walking '..' entries upwards, and loops in the dirent index
         * btree will cause livelocks.
-        *
-        * ESTALE means we scan the whole cluster again.
         */
-       if (S_ISREG(bstat->bs_mode)) {
+       if (S_ISREG(bstat->bs_mode))
                fd = scrub_open_handle(handle);
-               if (fd < 0 && errno == ESTALE)
-                       return ESTALE;
-       }
 
        /* Scrub the inode. */
        error = scrub_file(ctx, fd, bstat, XFS_SCRUB_TYPE_INODE, &alist);