| Message ID | 8babc8eb-1713-91c9-1efa-496909340a6f@huawei.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | tune2fs: check whether dev is mounted or in use before setting | expand |
On Sat, Mar 18, 2023 at 11:36:03AM +0800, Zhiqiang Liu wrote: > From: Zhiqiang Liu <liuzhiqiang26@huawei.com> > > tune2fs is used to adjust various tunable filesystem pars, which > may conflict with kernel operations. So we should check whether > device is mounted or in use at the begin similar to e2fsck and mke2fs. > > Of course, we can ignore this check if -f is set. Tune2fs is designed to work on mounted file systems. There are individual checks for those changes which can not be safely done on mounted file systems, but most changes are safe to do on mounted file systems. Cheers, - Ted
On 2023/3/19 0:24, Theodore Ts'o wrote: > On Sat, Mar 18, 2023 at 11:36:03AM +0800, Zhiqiang Liu wrote: >> From: Zhiqiang Liu <liuzhiqiang26@huawei.com> >> >> tune2fs is used to adjust various tunable filesystem pars, which >> may conflict with kernel operations. So we should check whether >> device is mounted or in use at the begin similar to e2fsck and mke2fs. >> >> Of course, we can ignore this check if -f is set. > > Tune2fs is designed to work on mounted file systems. There are > individual checks for those changes which can not be safely done on > mounted file systems, but most changes are safe to do on mounted file > systems. > Thanks for your reply. Does quota setting is safely done on mounted or busy filesystems? We have met a problem as follows, # mkfs.ext4 /dev/sdd # mount /dev/sdd /test # /test mountpoint is used in other namespace # umount /dev/sdd # tune2fs -O project,quota /dev/sdd # mount -o prjquota /dev/sdd /test # mount | grep sdd /dev/sdd on /test type ext4 (rw,relatime,seclabel,prjquota) # quotaon -Ppv /test quotaon: Mountpoint (or device) /test not found or has no quota enabled Here, tune2fs only check whether /test is mountted when setting project,quota, it does not check whether /test is busy (/test is mounted in other namespace). Users will be very confused about why prjquota does no take effect. Should we check whether mountpoint is busy when setting quota? Zhiqiang Liu. > Cheers, > > - Ted > > . >
On Sun, Mar 19, 2023 at 01:44:57PM +0800, Zhiqiang Liu wrote: > > Does quota setting is safely done on mounted or busy filesystems? No, and tune2fs already disallows this. if (Q_flag) { if (mount_flags & EXT2_MF_MOUNTED) { fputs(_("The quota feature may only be changed when " "the filesystem is unmounted.\n"), stderr); rc = 1; goto closefs; } > We have met a problem as follows, > # mkfs.ext4 /dev/sdd > # mount /dev/sdd /test > # /test mountpoint is used in other namespace > # umount /dev/sdd > # tune2fs -O project,quota /dev/sdd First of all, you unmounted /dev/sdd above. If it had remained mounted... root@kvm-xfstests:~# mkfs.ext4 -q /dev/vdc root@kvm-xfstests:~# mount /dev/vdc /vdc [ 103.424437] EXT4-fs (vdc): mounted filesystem 37d825ba-e289-4507-a17b-71c4b84cc773 with ordered data mode. Quota mode: none. root@kvm-xfstests:~# tune2fs -O project,quota /dev/vdc tune2fs 1.47.0 (5-Feb-2023) The quota feature may only be changed when the filesystem is unmounted. root@kvm-xfstests:~# > # mount -o prjquota /dev/sdd /test > # mount | grep sdd > /dev/sdd on /test type ext4 (rw,relatime,seclabel,prjquota) > # quotaon -Ppv /test > quotaon: Mountpoint (or device) /test not found or has no quota enabled Your problem is that a problem of understanding. The quotaon and quotaoff commands are not needed and are not supported once you use the ext4 "quota as a first class feature". Before the existence of the ext4 quota feature, ext4 had quota support, but it was done using visible files (aquota.user and aquota.group in the top-level directory of the file system). This older quota system had a lot of problems. The top-level files were visible, and could be corrupted by users. Since you had to explicitly enable quota support, the quota files could easily get out of sync with reality, which required use of a separate quotacheck command; since it ran on the mounted file system, it was (a) slow, and (b) while quotacheck was running, if anything else created or deleted files, the quota files could be out of sync with reality even before the quotacheck was completed. The new ext4 quota feature means that the moment the file system is mounted, the quota information is updated. You don't have the option of turning off quota tracking (other than unmounting the file system, and removing the quota feature, of course). You also don't need to run quotacheck; if the quota information is out of date, e2fsck will notice and correct the problem. For example: root@kvm-xfstests:~# tune2fs -O project,quota /dev/vdc tune2fs 1.47.0 (5-Feb-2023) root@kvm-xfstests:~# mount /dev/vdc /vdc [ 18.920024] EXT4-fs (vdc): recovery complete [ 18.920342] EXT4-fs (vdc): mounted filesystem 37d825ba-e289-4507-a17b-71c4b84cc773 with ordered data mode. Quota mode: journalled. root@kvm-xfstests:~# mkdir /vdc/quota root@kvm-xfstests:~# chattr -p 123 /vdc/quota root@kvm-xfstests:~# cp /etc/issue /vdc/quota root@kvm-xfstests:~# repquota -P /vdc *** Report for project quotas on device /dev/vdc Block grace time: 7days; Inode grace time: 7days Block limits File limits Project used soft hard grace used soft hard grace ---------------------------------------------------------------------- #0 -- 24 0 0 3 0 0 #123 -- 4 0 0 1 0 0 See? No need to use quotaon! > Here, tune2fs only check whether /test is mountted when setting project,quota, > it does not check whether /test is busy (/test is mounted in other namespace). > Users will be very confused about why prjquota does no take effect. The question is whether or not /test is mounted, but whether or not the device is mounted. In your example, you actually formatted the file system, so the device was clearly not mounted: > # mkfs.ext4 /dev/sdd > # mount /dev/sdd /test Are you saying that the problem is after this point, you created addditional mount namespaces, which where cloned off of the existing mount namespace, and left /dev/sdd mounted there? #1, don't do that. #2, your patch wouldn't have helped, since you were also only checking EXT2_MF_MOUNTED, and it works by checking /proc/mounts. If you are using mount namespaces, yes, it's possible for ext2fs_check_if_mounted() to give incorrect results. So I'm pretty sure you must not have tested your patch before you fired it off to me. :-) Now, what we *can* do if we want to bullet-proof against people using mount namespaces and doing stupid things would be to change the test (mount_flags & EXT2_MF_MOUNTED) to (mount_flags & (EXT2_MF_BUSY | EXT2_MF_MOUNTED))) The EXT2_MF_BUSY flag will indicates that an attempt to open the device with O_EXCL will fail. We do this in some places in tune2fs.c, but we missed this for a couple of cases, including in the tests for I_flag and Q_flag. Cheers, - Ted
On 2023/3/19 21:02, Theodore Ts'o wrote: > On Sun, Mar 19, 2023 at 01:44:57PM +0800, Zhiqiang Liu wrote: >> >> Does quota setting is safely done on mounted or busy filesystems? > > No, and tune2fs already disallows this. > > if (Q_flag) { > if (mount_flags & EXT2_MF_MOUNTED) { > fputs(_("The quota feature may only be changed when " > "the filesystem is unmounted.\n"), stderr); > rc = 1; > goto closefs; > } > > >> We have met a problem as follows, >> # mkfs.ext4 /dev/sdd >> # mount /dev/sdd /test >> # /test mountpoint is used in other namespace >> # umount /dev/sdd >> # tune2fs -O project,quota /dev/sdd > > First of all, you unmounted /dev/sdd above. If it had remained > mounted... > > root@kvm-xfstests:~# mkfs.ext4 -q /dev/vdc > root@kvm-xfstests:~# mount /dev/vdc /vdc > [ 103.424437] EXT4-fs (vdc): mounted filesystem 37d825ba-e289-4507-a17b-71c4b84cc773 with ordered data mode. Quota mode: none. > root@kvm-xfstests:~# tune2fs -O project,quota /dev/vdc > tune2fs 1.47.0 (5-Feb-2023) > The quota feature may only be changed when the filesystem is unmounted. > root@kvm-xfstests:~# > >> # mount -o prjquota /dev/sdd /test >> # mount | grep sdd >> /dev/sdd on /test type ext4 (rw,relatime,seclabel,prjquota) >> # quotaon -Ppv /test >> quotaon: Mountpoint (or device) /test not found or has no quota enabled > > Your problem is that a problem of understanding. The quotaon and > quotaoff commands are not needed and are not supported once you use > the ext4 "quota as a first class feature". > > Before the existence of the ext4 quota feature, ext4 had quota > support, but it was done using visible files (aquota.user and > aquota.group in the top-level directory of the file system). This > older quota system had a lot of problems. The top-level files were > visible, and could be corrupted by users. Since you had to explicitly > enable quota support, the quota files could easily get out of sync > with reality, which required use of a separate quotacheck command; > since it ran on the mounted file system, it was (a) slow, and (b) > while quotacheck was running, if anything else created or deleted > files, the quota files could be out of sync with reality even before > the quotacheck was completed. > > The new ext4 quota feature means that the moment the file system is > mounted, the quota information is updated. You don't have the option > of turning off quota tracking (other than unmounting the file system, > and removing the quota feature, of course). You also don't need to > run quotacheck; if the quota information is out of date, e2fsck will > notice and correct the problem. > > For example: > > root@kvm-xfstests:~# tune2fs -O project,quota /dev/vdc > tune2fs 1.47.0 (5-Feb-2023) > root@kvm-xfstests:~# mount /dev/vdc /vdc > [ 18.920024] EXT4-fs (vdc): recovery complete > [ 18.920342] EXT4-fs (vdc): mounted filesystem 37d825ba-e289-4507-a17b-71c4b84cc773 with ordered data mode. Quota mode: journalled. > root@kvm-xfstests:~# mkdir /vdc/quota > root@kvm-xfstests:~# chattr -p 123 /vdc/quota > root@kvm-xfstests:~# cp /etc/issue /vdc/quota > root@kvm-xfstests:~# repquota -P /vdc > *** Report for project quotas on device /dev/vdc > Block grace time: 7days; Inode grace time: 7days > Block limits File limits > Project used soft hard grace used soft hard grace > ---------------------------------------------------------------------- > #0 -- 24 0 0 3 0 0 > #123 -- 4 0 0 1 0 0 > > See? No need to use quotaon! > Thank you very much for your patient explanation. >> Here, tune2fs only check whether /test is mountted when setting project,quota, >> it does not check whether /test is busy (/test is mounted in other namespace). >> Users will be very confused about why prjquota does no take effect. > > The question is whether or not /test is mounted, but whether or not > the device is mounted. In your example, you actually formatted the > file system, so the device was clearly not mounted: > >> # mkfs.ext4 /dev/sdd >> # mount /dev/sdd /test > > Are you saying that the problem is after this point, you created > addditional mount namespaces, which where cloned off of the existing > mount namespace, and left /dev/sdd mounted there? Yes, that's exactly what I meant. I'm so sorry for my confusing expression. > > #1, don't do that. #2, your patch wouldn't have helped, since you > were also only checking EXT2_MF_MOUNTED, and it works by checking > /proc/mounts. If you are using mount namespaces, yes, it's possible > for ext2fs_check_if_mounted() to give incorrect results. So I'm > pretty sure you must not have tested your patch before you fired it > off to me. :-) Thanks for pointing out it. In fact, I have tested my patch before sending it to you. In my patch, I check both EXT2_MF_MOUNTED and EXT2_MF_BUSY flags similar to check_mount(). In my test, the /dev/sdd is unmounted in current namespace, but it was left mounted in other mount namespace. So only checking EXT2_MF_MOUNTED does not make sense. Actually, current tune2fs has already check EXT2_MF_MOUNTED flag when Q_flag is set. So, I refer to check_mount() to add EXT2_MF_BUSY check. The details as follows, --- a/misc/tune2fs.c +++ b/misc/tune2fs.c @@ -3327,6 +3327,22 @@ retry_open: goto closefs; } + if (open_flag & EXT2_FLAG_RW) { + if (mount_flags & EXT2_MF_MOUNTED) { + fprintf(stderr, _("Warning! %s is mounted.\n"), device_name); + if (!f_flag) { + rc = 1; + goto closefs; + } + } else if (mount_flags & EXT2_MF_BUSY) { + fprintf(stderr, _("Warning! %s is in use by the system.\n"), + device_name); + if (!f_flag) { + rc = 1; + goto closefs; + } + } > > Now, what we *can* do if we want to bullet-proof against people using > mount namespaces and doing stupid things would be to change the test > > (mount_flags & EXT2_MF_MOUNTED) > > to > > (mount_flags & (EXT2_MF_BUSY | EXT2_MF_MOUNTED))) > > The EXT2_MF_BUSY flag will indicates that an attempt to open the > device with O_EXCL will fail. We do this in some places in tune2fs.c, > but we missed this for a couple of cases, including in the tests for > I_flag and Q_flag. > > Cheers, > > - Ted Thanks for your suggestion. At the begin, I wanted to add EXT2_MF_BUSY check only in the Q_flag test. Because I didn't understand that tune2fs is designed for mounted file systems, I added the EXT2_MF_BUSY and EXT2_MF_MOUNTED before all tests. I will follow your suggestions to add EXT2_MF_BUSY check in both I_flag and Q_flag tests, and send a new patch. Thank you again for your patient explanation. Zhiqiang Liu. > . >
diff --git a/misc/tune2fs.c b/misc/tune2fs.c index 458f7cf6..b667e1f4 100644 --- a/misc/tune2fs.c +++ b/misc/tune2fs.c @@ -3327,6 +3327,22 @@ retry_open: goto closefs; } + if (open_flag & EXT2_FLAG_RW) { + if (mount_flags & EXT2_MF_MOUNTED) { + fprintf(stderr, _("Warning! %s is mounted.\n"), device_name); + if (!f_flag) { + rc = 1; + goto closefs; + } + } else if (mount_flags & EXT2_MF_BUSY) { + fprintf(stderr, _("Warning! %s is in use by the system.\n"), + device_name); + if (!f_flag) { + rc = 1; + goto closefs; + } + } + } #ifdef NO_RECOVERY /* Warn if file system needs recovery and it is opened for writing. */ if ((open_flag & EXT2_FLAG_RW) && !(mount_flags & EXT2_MF_MOUNTED) &&