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

[Yang Joseph <joseph.yang@xtaotech.com>]

On 02/27/2018 10:47 PM, Eric Sandeen wrote:
> On 2/27/18 4:44 AM, Yang Joseph wrote:
>> 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:
>
> # repair/xfs_repair /dev/sdb1
> xfs_repair: cannot open /dev/sdb1: Device or resource busy
As /dev/sdb1 will never be modified, I think this behavior is acceptable.
>
> 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.
>
> 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)
This is horrible. Maybe we can add O_EXCL flag when open target device.

   //copy/xfs_copy.c:main()
   /* now open targets */

   open_flags = O_RDWR | O_EXCL; // add O_EXCL

#mount
...
/dev/sdf1 on /root/xx1 type ext4 (rw,relatime,data=ordered)
// /dev/sdf2 contains a xfs image
# ./xfs_copy.new /dev/sdf2 /dev/sdf1
xfs_copy.new:  couldn't open target "/dev/sdf1"
Aborting XFS copy - reason:  Device or resource busy

>
> (Anyone want to investigate whether every device open in xfsprogs should
> be O_EXCL?)
// from manpage: OPEN(2)
on Linux 2.6
               and later, O_EXCL can be used without O_CREAT if pathname 
refers to a block device.  If the block device is  in  use
               by the system (e.g., mounted), open() fails with the 
error EBUSY.

>
> 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 <joseph.yang@xtaotech.com>
>
> 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.
ok, thank you.
>
> Thanks,
> -Eric
>
>
>> thx,
>>
>> Yang Honggang
>>
>> -------------------------new patch----------------------
>> diff --git a/libxfs/linux.c b/libxfs/linux.c
>> index 0bace3e..6ad24ce 100644
>> --- a/libxfs/linux.c
>> +++ b/libxfs/linux.c
>> @@ -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.
>
>>   /*
>>    * 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-----------------
>>
>> On 02/25/2018 06:04 AM, Dave Chinner wrote:
>>> On Sat, Feb 24, 2018 at 11:56:44AM -0600, Eric Sandeen wrote:
>>>> On 2/24/18 5:23 AM, Yang Joseph wrote:
>>>>> 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...
>>>
>>>> That sounds like a bug worth fixing, but I am much
>>>> less excited about adding options which could do serious damage
>>>> to a filesystem.
>>> 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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>