Page MenuHomeFreeBSD

Make it possible to update TMPFS mount point from read-only to read-write and vice versa.
ClosedPublic

Authored by sobomax on Mar 22 2019, 8:01 PM.

Details

Summary

Right now, attempt to update mounted TMPFS(5) fs from read-only to read-write or vica-versa is unsupported. Mmoreover, it might also cause very mysterious error message, if you have size parameter set in the /etc/fstab (as you probably should). This patch adds this missing functionality.

$ ./test_tmpfs.sh
+ echo '/dev/null /mnt tmpfs ro,size=100k 0 0'
+ sudo mount -F my.fstab /mnt
+ sudo mount -F my.fstab -u -o rw /mnt
mount: tmpfs: mount option <size> is unknown: Operation not supported

Test Plan

test_tmpfs.sh:

#!/bin/sh -xe

echo "/dev/null /mnt tmpfs ro,size=100k 0 0" > my.fstab
sudo mount -F my.fstab /mnt
sudo mount -F my.fstab -u -o rw /mnt
sudo umount -F my.fstab /mnt

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sobomax created this revision.Mar 22 2019, 8:01 PM
delphij accepted this revision.Mar 22 2019, 8:03 PM
This revision is now accepted and ready to land.Mar 22 2019, 8:03 PM
kib added a comment.Mar 22 2019, 9:03 PM

As I understand, currently ro tmpfs mounts are useless and the code is incomplete. I did very quick look and the first VOP I looked at, tmpfs_mkdir(), seems to allow modifications of the ro mounts (this is from code reading, not from testing). Was code review done to ensure that read-only tmpfs mounts indeed operate as intended ?

sys/fs/tmpfs/tmpfs_vfsops.c
180 ↗(On Diff #55362)

I cannot see a single reason to call VFS_SYNC() there.

186 ↗(On Diff #55362)

This call does not work reliably. What prevents other threads from opening files rw while vflush() is running ?

This revision was automatically updated to reflect the committed changes.
In D19682#421511, @kib wrote:

As I understand, currently ro tmpfs mounts are useless and the code is incomplete. I did very quick look and the first VOP I looked at, tmpfs_mkdir(), seems to allow modifications of the ro mounts (this is from code reading, not from testing). Was code review done to ensure that read-only tmpfs mounts indeed operate as intended ?

It seems to be working properly with this change.

[ssp-root@sipit_rack ~/freebsd11]$ sudo touch /mnt/foo
touch: /mnt/foo: Read-only file system
[ssp-root@sipit_rack ~/freebsd11]$ sudo mkdir /mnt/foo
mkdir: /mnt/foo: Read-only file system
[ssp-root@sipit_rack ~/freebsd11]$ sudo mount -u -o rw /mnt
[ssp-root@sipit_rack ~/freebsd11]$ sudo touch /mnt/foo
[ssp-root@sipit_rack ~/freebsd11]$ sudo mkdir /mnt/foodir
[ssp-root@sipit_rack ~/freebsd11]$ sudo rm -rf /mnt/foo /mnt/foodir
[ssp-root@sipit_rack ~/freebsd11]$ sudo mount -u -o ro /mnt
[ssp-root@sipit_rack ~/freebsd11]$ sudo touch /mnt/foo
touch: /mnt/foo: Read-only file system
[ssp-root@sipit_rack ~/freebsd11]$ sudo mkdir /mnt/foodir
mkdir: /mnt/foodir: Read-only file system
sobomax added inline comments.Mar 22 2019, 11:09 PM
sys/fs/tmpfs/tmpfs_vfsops.c
180 ↗(On Diff #55362)

The code is copied from MSDOSFS(5). AFAIK, most of the other FSses in the tree (checked nandfs, ext2fs) do the very same thing, including vflush below. If it's a NOP it should probably be deleted from those other places too.

186 ↗(On Diff #55362)

See above, if true, similar problem affects other file systems in the tree.

kib added inline comments.Mar 23 2019, 10:30 AM
sys/fs/tmpfs/tmpfs_vfsops.c
180 ↗(On Diff #55362)

Sigh. The call is needed for what you refer to as 'other filesystems', but does nothing for tmpfs.

186 ↗(On Diff #55362)

Yes, it is true and msdosfs has the same issue. Also the similar problem exists for msdosfs unmounts. Due to limited usage patterns of msdosfs I see why it was not reported so far.

For tmpfs the unmount issue was reported long time ago at early stage of the code existence in our tree, and fixed. Now you reintroduced the bug in slightly different setup.

kib added a comment.Mar 23 2019, 12:09 PM
In D19682#421511, @kib wrote:

As I understand, currently ro tmpfs mounts are useless and the code is incomplete. I did very quick look and the first VOP I looked at, tmpfs_mkdir(), seems to allow modifications of the ro mounts (this is from code reading, not from testing). Was code review done to ensure that read-only tmpfs mounts indeed operate as intended ?

It seems to be working properly with this change.

[ssp-root@sipit_rack ~/freebsd11]$ sudo touch /mnt/foo
touch: /mnt/foo: Read-only file system
[ssp-root@sipit_rack ~/freebsd11]$ sudo mkdir /mnt/foo
mkdir: /mnt/foo: Read-only file system
[ssp-root@sipit_rack ~/freebsd11]$ sudo mount -u -o rw /mnt
[ssp-root@sipit_rack ~/freebsd11]$ sudo touch /mnt/foo
[ssp-root@sipit_rack ~/freebsd11]$ sudo mkdir /mnt/foodir
[ssp-root@sipit_rack ~/freebsd11]$ sudo rm -rf /mnt/foo /mnt/foodir
[ssp-root@sipit_rack ~/freebsd11]$ sudo mount -u -o ro /mnt
[ssp-root@sipit_rack ~/freebsd11]$ sudo touch /mnt/foo
touch: /mnt/foo: Read-only file system
[ssp-root@sipit_rack ~/freebsd11]$ sudo mkdir /mnt/foodir
mkdir: /mnt/foodir: Read-only file system
# mount /tmp
# touch /tmp/1
# echo 12345 >/tmp/1
# mount -u -r /tmp
# stat /tmp/1
18446744071679573762 3 -rw-r--r-- 1 root wheel 18446744073709551615 6 "Mar 23 14:05:29 2019" "Mar 23 14:05:29 2019" "Mar 23 14:05:29 2019" "Mar 23 14:05:24 2019" 4096 8 0 /tmp/1
# cat /tmp/1
12345
# stat /tmp/1
18446744071679573762 3 -rw-r--r-- 1 root wheel 18446744073709551615 6 "Mar 23 14:05:45 2019" "Mar 23 14:05:29 2019" "Mar 23 14:05:29 2019" "Mar 23 14:05:24 2019" 4096 8 0 /tmp/1
# cat /tmp/1 
12345
# stat /tmp/1
18446744071679573762 3 -rw-r--r-- 1 root wheel 18446744073709551615 6 "Mar 23 14:05:56 2019" "Mar 23 14:05:29 2019" "Mar 23 14:05:29 2019" "Mar 23 14:05:24 2019" 4096 8 0 /tmp/1
# echo 1234 >|/tmp/2
-sh: cannot create /tmp/2: Read-only file system

Watch atime.

As I said, the ro feature of tmpfs was never exercised and cases when it works are mostly handle it due to sheer luck.