xfs_repair: add '-F' option to ignore writable mount checking

Quoted 17 lines · Yang Joseph

Quoted 4 lines

xfs_repair should not touch non-xfs mountpoints in platform_check_mount(). If non-xfs mountpoints can be filtered out, the dead fuse mountpoint can never block our xfs_repair. The following patch can fix my problem and not add dangerous option to xfs_repair.

I don't think this is a safe solution. For example:

# mkfs.ext4 /dev/sdb1
# mount /dev/sdb1 /mnt/test

Old behavior:

# xfs_repair /dev/sdb1
xfs_repair: /dev/sdb1 contains a mounted filesystem

New behavior with your patch:

Quoted 8 lines

# repair/xfs_repair /dev/sdb1
xfs_repair: cannot open /dev/sdb1: Device or resource busy

With your patch, if we explicitly ask xfs_repair to repair a non-xfs filesystem which is mounted, it will get past all of the checks and try to open the device O_EXCL - which should fail if the device is mounted, but I'm guessing your fuse case would now simply hang here instead.

As fuse mountpoint is filtered out, xfs_repair will not hang here. (No stat(/fuse/mountpoint/))

Quoted 103 lines

Worse, other utilities aren't so graceful.

Old:

# xfs_copy /dev/sdb2 /dev/sdb1
xfs_copy:  a filesystem is mounted on target device "/dev/sdb1".
xfs_copy cannot copy to mounted filesystems.  Aborting

New:

copy/xfs_copy /dev/sdb2 /dev/sdb1
0% ... 10% ... 20% ... 30% ... 40% ... 50% ... 60% ... 70% ... 80% ... 90% ... 100%

All copies completed.

(now we've just written over a mounted, writable ext4 filesystem)

(Anyone want to investigate whether every device open in xfsprogs should be O_EXCL?)

Also, in general:

When sending a patch to the list, please prefix the subject with [PATCH] to make it obvious; patches also require a Signed-off-by:
line similar to:

Signed-off-by Yang Joseph <[email protected]>

The Signed-off-by: tag indicates that the signer was involved in the development of the patch.

It essentially states that you are the author of this work and you have the right to submit it for inclusion in this project.

Thanks,
-Eric

Quoted 10 lines

thx,

Yang Honggang

-------------------------new patch----------------------

libxfs/linux.c +0 -0

index 0bace3e..6ad24ce 100644

libxfs/linux.c static int max_block_alignment; · line 47

Quoted 4 lines

libxfs/linux.c +1 -0

@@ -44,6 +44,7 @@ static int max_block_alignment;
  #endif
 
  #define PROC_MOUNTED   "/proc/mounts"
+#define MNTTYPE_XFS        "xfs"

indentation should be consistent with the string above; the whole patch is whitespace-mangled and won't apply.

Quoted 47 lines

libxfs/linux.c +3 -1

  /*
   * Check if the filesystem is mounted.  Be verbose if asked, and

@@ -78,6 +79,9 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
                 return 1;
         }
         while ((mnt = getmntent(f)) != NULL) {
+               /* filter out non xfs mountpoint */
+               if (strncmp(mnt->mnt_type, MNTTYPE_XFS, strlen(mnt->mnt_type)))
+                       continue;
                 if (stat(mnt->mnt_dir, &mst) < 0)
                         continue;
                 if (mst.st_dev != s->st_rdev)
-------------------------new patch end-----------------

Quoted 21 lines · Dave Chinner

Quoted 6 lines · Eric Sandeen

Quoted 5 lines · Yang Joseph

hello,

Before the repair process, xfs_repair will check if user specified device already has a writable mountpoint. And it will stat all the mountpoints of the system. If there is a dead mountpoint, this checking will be blocked and xfs_repair will enter 'D' state.

So why is the mount point dead?

That kinda means that the filesystem is still mounted, but something has hung somewhere and the filesystem may still have active references to the underlying device and be doing stuff that is modifying the filesystem....

And if the device is still busy, then you aren't going to be able to mount the repaired device, anyway, because the block device is still busy...

Quoted 3 lines

That sounds like a bug worth fixing, but I am much less excited about adding options which could do serious damage to a filesystem.

Quoted 6 lines

TO me it sounds like something that should be fixed by a reboot, not by adding dangerous options to xfs_repair...

Cheers,

Dave.

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

hello,

My last reply is rejected, so I resend this email.

A new suggestion:

From: Yang Honggang <[email protected]>

stat(/path/to/device) instead of stat(mountpoint) to prevent platform_check_mount() from hanging on stat() systemcall when a dead fuse mountpoint is encountered. Because this kind of mountpoint has no local device, only 'ceph-fuse', stat('ceph-fuse') will return error, and the while loop will continue.

Signed-off-by Yang Honggang <[email protected]>

---
 libxfs/linux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

libxfs/linux.c +2 -2

index 0bace3e..d415c33 100644
@@ -78,9 +78,9 @@ platform_check_mount(char *name, char *block, struct 
 stat *s, int flags)
         return 1;
     }
     while ((mnt = getmntent(f)) != NULL) {
-        if (stat(mnt->mnt_dir, &mst) < 0)
+        if (stat(mnt->mnt_fsname, &mst) < 0)
             continue;
-        if (mst.st_dev != s->st_rdev)
+        if (mst.st_rdev != s->st_rdev)
             continue;
         /* Found our device, is RO OK? */

         if ((flags & CHECK_MOUNT_WRITABLE) && hasmntopt(mnt, MNTOPT_RO))