Prevent atime from being updated when TMPFS(5) is mounted read-only

[sobomax]

Summary

As @kib has pointed out in the D19682, TMPFS(5) updates atime regardless of whether it's mounted read-only or read-write. This small patch fixes the issue in question.

Test Plan

#!/bin/sh -xe

tempfoo="`basename $0`"
TDIR="`mktemp -d /tmp/${tempfoo}.XXXXXX`"

sudo mount -o size=100k -t tmpfs /dev/null ${TDIR}
TFILE="${TDIR}/foo"
echo bar | sudo tee "${TFILE}"
eval `stat -s ${TFILE}`
sudo mount -u -o ro ${TDIR}
old_atime=${st_atime}
sleep 2
cat "${TFILE}" > /dev/null
eval `stat -s ${TFILE}`
sudo umount ${TDIR}
rmdir ${TDIR}
if [ ${old_atime} -ne ${st_atime} ]
then
  echo "FAIL, ${st_atime} != ${old_atime}"
  exit 1
fi
echo "Looks OK"
exit 0

[kib]

Your 'fix' is not enough even for mtime. Look at the tmpfs_sync(MNT_LAZY).

As I explained to you (and you ignored me), your enablement of the read-only upgrade for tmpfs is completely wrong. Regardless of your buggy changes to tmpfs_mount(), which are both racy and do not fully commit the pending metadata updates, filesystem needs a lot of work to make it properly honor the MNT_RDONLY flag. The code requires complete audit to make it safe.

Try the following:

1. open tmpfs file rw and mmap it with PROT_READ|PROT_WRITE, MAP_SHARED.
2. without exiting the mapping process, remount the tmpfs mount ro
3. write to the mapping, watch the file content.

Just in case you think it is not too hard, close the file descriptor after #1.

[sobomax]

Check read-only flag in an unified manner.

[sobomax]

In D19707#422201, @kib wrote:

Your 'fix' is not enough even for mtime. Look at the tmpfs_sync(MNT_LAZY).

Hmm, could you be more specific, perhaps? I looked through tmpfs_sync(MNT_LAZY) and don't see any places where update might be happening.

As I explained to you (and you ignored me),

That's your impression of it. I did not ignore you, your comment came too late when I was already in the process of final testing of my changes after positive review from @delphij. Actually I did not get a chance to read it until the next morning. In fact I followed up and making an effort to improve it further based on that (this review request is the living proof of that), even though per se the code that has been committed is sufficient to solve our immediate need.

your enablement of the read-only upgrade for tmpfs is completely wrong.

That's very subjective PoV. From our prospective, the change could not possibly be wrong since it gives us the functionality that we need and solves our problem at hand. Yes, it might not be perfect or even complete, but something rarely is. If nothing else, this is "opt-in" feature, so nobody is forced to use it. Again, from our PoV lack of ability to change from RW to RO and vice-versa is much bigger problem than some corner cases in which such "RO after RW" filesystem might (or might not) have issues based on usage scenario. It's like having network interface that you cannot change IP/mask of or take up/down as needed. And guess what? There are plenty of interfaces that do crazy things when you change IP address or netmask, yet nobody thinks of disabling this functionality until particular network driver does it perfectly.

Regardless of your buggy changes to tmpfs_mount(), which are both racy and do not fully commit the pending metadata updates, filesystem needs a lot of work to make it properly honor the MNT_RDONLY flag. The code requires complete audit to make it safe.

Well, I believe in an incremental approach to software engineering. Not only that, I believe that in fact it's the only approach that actually works. I also helps (if one could) refrain from labeling someone else's work as "buggy", "completely wrong" etc and stick to technical issues at hand. There is not a huge amount of code in TMPFS(5), so such audit is definitely not outside of our reach.

Try the following:

1. open tmpfs file rw and mmap it with PROT_READ|PROT_WRITE, MAP_SHARED.
2. without exiting the mapping process, remount the tmpfs mount ro
3. write to the mapping, watch the file content.

Just in case you think it is not too hard, close the file descriptor after #1.

I suppose you don't have a test case written to expose this bug, do you?

[kib]

In D19707#422314, @sobomax wrote:

In D19707#422201, @kib wrote:

Your 'fix' is not enough even for mtime. Look at the tmpfs_sync(MNT_LAZY).

Hmm, could you be more specific, perhaps? I looked through tmpfs_sync(MNT_LAZY) and don't see any places where update might be happening.

tmpfs_sync(MNT_LAZY) checks for OBJ_TMPFS_DIRTY flag for v_object for each currently existing vnode (not inode). The flag means that there are dirty pages in the object queue, which means that mtime should be updated. Either mtime is updated and then inode content is changed, or it is too late since you set MNT_RDONLY and then mtime is wrong.

This is, of course, relatively minor comparing with the fact that pages can be dirtied after MNT_RDONLY is set.

As I explained to you (and you ignored me),

That's your impression of it. I did not ignore you, your comment came too late when I was already in the process of final testing of my changes after positive review from @delphij. Actually I did not get a chance to read it until the next morning. In fact I followed up and making an effort to improve it further based on that (this review request is the living proof of that), even though per se the code that has been committed is sufficient to solve our immediate need.

You committed in 2 or 3 hours after the review was posted, and 0.5-1 hour after I commented.

your enablement of the read-only upgrade for tmpfs is completely wrong.

That's very subjective PoV. From our prospective, the change could not possibly be wrong since it gives us the functionality that we need and solves our problem at hand. Yes, it might not be perfect or even complete, but something rarely is. If nothing else, this is "opt-in" feature, so nobody is forced to use it. Again, from our PoV lack of ability to change from RW to RO and vice-versa is much bigger problem than some corner cases in which such "RO after RW" filesystem might (or might not) have issues based on usage scenario. It's like having network interface that you cannot change IP/mask of or take up/down as needed. And guess what? There are plenty of interfaces that do crazy things when you change IP address or netmask, yet nobody thinks of disabling this functionality until particular network driver does it perfectly.

This is plain demagogy. Calling your change incomplete is an offense for changes which are incomplete. Kind of bugs that corrupt user data is very hard to expel from collective user memory, lot of efforts of many people where put to make FreeBSD reliable, you knowingly break it with straight face and claims that it is fine.

I explained to you that tmpfs code is not prepared to operate in ro mode, the immutable data is not in too many ways. I know this because I did significant re-acrchitecturing of tmpfs code and read it. The only qualification for the feature you added is that it does set MNT_RDONLY flag.

Regardless of your buggy changes to tmpfs_mount(), which are both racy and do not fully commit the pending metadata updates, filesystem needs a lot of work to make it properly honor the MNT_RDONLY flag. The code requires complete audit to make it safe.

Well, I believe in an incremental approach to software engineering. Not only that, I believe that in fact it's the only approach that actually works. I also helps (if one could) refrain from labeling someone else's work as "buggy", "completely wrong" etc and stick to technical issues at hand. There is not a huge amount of code in TMPFS(5), so such audit is definitely not outside of our reach.

This audit must be done before the commit that makes the knob. Otherwise it is called drive-by, not incremental. Why should I not call buggy, incomplete (at best) and lacking proper review commits such ?

Try the following:

1. open tmpfs file rw and mmap it with PROT_READ|PROT_WRITE, MAP_SHARED.
2. without exiting the mapping process, remount the tmpfs mount ro
3. write to the mapping, watch the file content.

Just in case you think it is not too hard, close the file descriptor after #1.

I suppose you don't have a test case written to expose this bug, do you?

No, I have zero motivation to prepare the test case.