xfs: readonly handling changes

There are dueling comments in the xfs code about intent for log writes when unmounting a readonly filesystem.

In xfs_mountfs, we see the intent:

/*
* Now the log is fully replayed, we can transition to full read-only
* mode for read-only mounts. This will sync all the metadata and clean
* the log so that the recovery we just performed does not have to be
* replayed again on the next mount.
*/

and it calls xfs_quiesce_attr(), but by the time we get to xfs_log_unmount_write(), it returns early for a RDONLY mount:

* Don't write out unmount record on read-only mounts.

Because of this, sequential ro mounts of a filesystem with a dirty log will replay the log each time, which seems odd.

Fix this by writing an unmount record even for RO mounts, as long as norecovery wasn't specified (don't write a clean log record if a dirty log may still be there!) and the log device is writable.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/xfs_log.c +5 -3

index b1469f0..62176b8 100644
@@ -801,11 +801,14 @@
 	int		 error;
 
 	/*
-	 * Don't write out unmount record on read-only mounts.
+	 * Don't write out unmount record on norecovery mounts or ro devices.
 	 * Or, if we are doing a forced umount (typically because of IO errors).
 	 */
-	if (mp->m_flags & XFS_MOUNT_RDONLY)
+	if (mp->m_flags & XFS_MOUNT_NORECOVERY ||
+	    xfs_readonly_buftarg(log->l_mp->m_logdev_targp)) {
+		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
 		return 0;
+	}
 
 	error = _xfs_log_force(mp, XFS_LOG_SYNC, NULL);
 	ASSERT(error || !(XLOG_FORCED_SHUTDOWN(log)));
 
--

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

xfs_release & xfs_inactive both had early returns for readonly mounts.

Ultimately, this means that when we do log recovery on a read-only mount, we do not process unlinked inodes, because of this misguided effort to not do /any/ IO, ever, on a readonly mount. IO at mount time is fine, and expected - after all we just got done doing log recovery! Even ro mounts, without the norecovery flag, can do enough IO to put the filesystem in a consistent state.

We should not get here after mount is complete;

at that point
the vfs will not allow anything from userspace to make modifications which would get us here with any IO to do -

we can't unlink files, or create blocks past eof, etc. So it's safe to just remove these checks.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/xfs_inode.c +0 -9

index edfa6a5..bf74165 100644
@@ -1658,10 +1658,6 @@
 	if (!S_ISREG(VFS_I(ip)->i_mode) || (VFS_I(ip)->i_mode == 0))
 		return 0;
 
-	/* If this is a read-only mount, don't do this (would generate I/O) */
-	if (mp->m_flags & XFS_MOUNT_RDONLY)
-		return 0;
-
 	if (!XFS_FORCED_SHUTDOWN(mp)) {
 		int truncated;
 

@@ -1896,10 +1892,6 @@
 	mp = ip->i_mount;
 	ASSERT(!xfs_iflags_test(ip, XFS_IRECOVERY));
 
-	/* If this is a read-only mount, don't do this (would generate I/O) */
-	if (mp->m_flags & XFS_MOUNT_RDONLY)
-		return;
-
 	if (VFS_I(ip)->i_nlink != 0) {
 		/*
 		 * force is true because we are evicting an inode from the
--

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

Quoted 11 lines · Eric Sandeen

xfs_release & xfs_inactive both had early returns for readonly mounts.

Ultimately, this means that when we do log recovery on a read-only mount, we do not process unlinked inodes, because of this misguided effort to not do /any/ IO, ever, on a readonly mount. IO at mount time is fine, and expected - after all we just got done doing log recovery! Even ro mounts, without the norecovery flag, can do enough IO to put the filesystem in a consistent state.

Quoted 1 line

We should not get here after mount is complete;

sorry, above is wrong.

Quoted 3 lines

at that point
the vfs will not allow anything from userspace to make modifications which would get us here with any IO to do -

but I think this part is right. :) I guess we might lose a little effiency doing pointless checks in i.e. xfs_release if it's a readonly mount and we know there is no work to do.

I won't resend until it's had a couple eyeballs...

Quoted 14 lines

we can't unlink files, or create blocks past eof, etc. So it's safe to just remove these checks.

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/xfs_inode.c +0 -0

index edfa6a5..bf74165 100644
@@ -1658,10 +1658,6 @@
 	if (!S_ISREG(VFS_I(ip)->i_mode) || (VFS_I(ip)->i_mode == 0))
 		return 0;
 

fs/xfs/xfs_inode.c line 1664

Quoted 4 lines

fs/xfs/xfs_inode.c +0 -4

-	/* If this is a read-only mount, don't do this (would generate I/O) */
-	if (mp->m_flags & XFS_MOUNT_RDONLY)
-		return 0;
-

Quoted 15 lines

fs/xfs/xfs_inode.c +0 -5

 	if (!XFS_FORCED_SHUTDOWN(mp)) {
 		int truncated;
 

@@ -1896,10 +1892,6 @@
 	mp = ip->i_mount;
 	ASSERT(!xfs_iflags_test(ip, XFS_IRECOVERY));
 
-	/* If this is a read-only mount, don't do this (would generate I/O) */
-	if (mp->m_flags & XFS_MOUNT_RDONLY)
-		return;
-
 	if (VFS_I(ip)->i_nlink != 0) {
 		/*
 		 * force is true because we are evicting an inode from the
--

Quoted 4 lines

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

--
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

I think some ASSERT(!ro) calls would be prudent in the newly reachable codepaths that would make modifications (in both xfs_release() and xfs_inactive()), just to catch any future bugs that would otherwise go undetected. Otherwise, both patches seem reasonable to me.

Quoted 4 lines · Brian Foster

I think some ASSERT(!ro) calls would be prudent in the newly reachable codepaths that would make modifications (in both xfs_release() and xfs_inactive()), just to catch any future bugs that would otherwise go undetected. Otherwise, both patches seem reasonable to me.

Ok, well - we can't assert (!ro) because we /do/ get here in the early stages of an ro mount.

I want to rework all this like Dave had suggested, but it's not getting done this release cycle, and I thought a couple targeted changes like this which fix the bug without making the code beautiful might still make it :)

Maybe the best shortcut for now is to stash, remove, and replace the RO mount flag like we do for log recovery itself, and clean it all up in the next round.

-Eric
--
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

When we do log recovery on a readonly mount, unlinked inode processing does not happen due to the readonly checks in xfs_inactive(), which are trying to prevent any I/O on a readonly mount.

This is misguided - we do I/O on readonly mounts all the time, for consistency; for example, log recovery. So do the same RDONLY flag twiddling around xfs_log_mount_finish() as we do around xfs_log_mount(), for the same reason.

This all cries out for a big rework but for now this is a simple fix to an obvious problem.

Signed-off-by Eric Sandeen <[email protected]>

---

V2: And now for something completely different...

fs/xfs/xfs_log.c +7 -1

index 62176b8..374edc1 100644
@@ -743,16 +743,23 @@
 	struct xfs_mount	*mp)
 {
 	int	error = 0;
+	int	readonly = (mp->m_flags & XFS_MOUNT_RDONLY);
 
 	if (mp->m_flags & XFS_MOUNT_NORECOVERY) {
 		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
 		return 0;
+	} else if (readonly) {
+		/* Allow unlinked processing to proceed */
+		mp->m_flags &= ~XFS_MOUNT_RDONLY;
 	}
 
 	error = xlog_recover_finish(mp->m_log);
 	if (!error)
 		xfs_log_work_queue(mp);
 
+	if (readonly)
+		mp->m_flags |= XFS_MOUNT_RDONLY;
+
 	return error;
 }
 
 
--

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

Quoted 10 lines · Eric Sandeen

When we do log recovery on a readonly mount, unlinked inode processing does not happen due to the readonly checks in xfs_inactive(), which are trying to prevent any I/O on a readonly mount.

This is misguided - we do I/O on readonly mounts all the time, for consistency; for example, log recovery. So do the same RDONLY flag twiddling around xfs_log_mount_finish() as we do around xfs_log_mount(), for the same reason.

Quoted 6 lines

This all cries out for a big rework but for now this is a simple fix to an obvious problem.

Signed-off-by Eric Sandeen <[email protected]>

---

Reviewed-by Brian Foster <[email protected]>

Quoted 32 lines

V2: And now for something completely different...

fs/xfs/xfs_log.c +7 -1

index 62176b8..374edc1 100644
@@ -743,16 +743,23 @@
 	struct xfs_mount	*mp)
 {
 	int	error = 0;
+	int	readonly = (mp->m_flags & XFS_MOUNT_RDONLY);
 
 	if (mp->m_flags & XFS_MOUNT_NORECOVERY) {
 		ASSERT(mp->m_flags & XFS_MOUNT_RDONLY);
 		return 0;
+	} else if (readonly) {
+		/* Allow unlinked processing to proceed */
+		mp->m_flags &= ~XFS_MOUNT_RDONLY;
 	}
 
 	error = xlog_recover_finish(mp->m_log);
 	if (!error)
 		xfs_log_work_queue(mp);
 
+	if (readonly)
+		mp->m_flags |= XFS_MOUNT_RDONLY;
+
 	return error;
 }
 
 
--

Quoted 3 lines

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

--
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

Quoted 14 lines · Brian Foster

Quoted 13 lines · Eric Sandeen

When we do log recovery on a readonly mount, unlinked inode processing does not happen due to the readonly checks in xfs_inactive(), which are trying to prevent any I/O on a readonly mount.

This is misguided - we do I/O on readonly mounts all the time, for consistency; for example, log recovery. So do the same RDONLY flag twiddling around xfs_log_mount_finish() as we do around xfs_log_mount(), for the same reason.

This all cries out for a big rework but for now this is a simple fix to an obvious problem.

Quoted 3 lines

Quoted 3 lines

Signed-off-by Eric Sandeen <[email protected]>

---

Both patches look ok, so I'll put them on the test queue for -rc4.

Reviewed-by Darrick J. Wong <[email protected]>

Quoted 15 lines · Darrick J. Wong

Quoted 14 lines · Brian Foster

Quoted 13 lines · Eric Sandeen

When we do log recovery on a readonly mount, unlinked inode processing does not happen due to the readonly checks in xfs_inactive(), which are trying to prevent any I/O on a readonly mount.

This is misguided - we do I/O on readonly mounts all the time, for consistency; for example, log recovery. So do the same RDONLY flag twiddling around xfs_log_mount_finish() as we do around xfs_log_mount(), for the same reason.

This all cries out for a big rework but for now this is a simple fix to an obvious problem.

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

Signed-off-by Eric Sandeen <[email protected]>

---

Both patches look ok, so I'll put them on the test queue for -rc4.

Reviewed-by Darrick J. Wong <[email protected]>

FWIW, I don't think this is a -rc candidate. Making log recovery process unlinked inode transactions on read-only mounts is a pretty major change in behaviour. Who knows exactly what dragons are lurking at lower layers that have never been run in this context until now.

Also, it's not urgent - we've lived with this behaviour for years - so waiting a month for the next merge window is not going to hurt anyone and it gives us a chance to test it - XFS developers are the people who should be burnt by the lurking dragons, not users who updated to a late -rcX kernel....

Quoted 29 lines · Dave Chinner

Quoted 15 lines · Darrick J. Wong

Quoted 14 lines · Brian Foster

Quoted 13 lines · Eric Sandeen

When we do log recovery on a readonly mount, unlinked inode processing does not happen due to the readonly checks in xfs_inactive(), which are trying to prevent any I/O on a readonly mount.

This is misguided - we do I/O on readonly mounts all the time, for consistency; for example, log recovery. So do the same RDONLY flag twiddling around xfs_log_mount_finish() as we do around xfs_log_mount(), for the same reason.

This all cries out for a big rework but for now this is a simple fix to an obvious problem.

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

Signed-off-by Eric Sandeen <[email protected]>

---

Both patches look ok, so I'll put them on the test queue for -rc4.

Reviewed-by Darrick J. Wong <[email protected]>

FWIW, I don't think this is a -rc candidate. Making log recovery process unlinked inode transactions on read-only mounts is a pretty major change in behaviour. Who knows exactly what dragons are lurking at lower layers that have never been run in this context until now.

Quoted 5 lines

Also, it's not urgent - we've lived with this behaviour for years - so waiting a month for the next merge window is not going to hurt anyone and it gives us a chance to test it - XFS developers are the people who should be burnt by the lurking dragons, not users who updated to a late -rcX kernel....

To shield Darrick a bit ;) I was agitating/asking for sooner, but admittedly that was a little bit selfish on my part.

Still, we have had field reports of people with /gigabytes/ missing from the root filesystem, and it was not fixable without an xfs_repair. Which on a root filesystem is ... special.

So, my fault for getting it sent late, for sure - but I do think it's an important fix. I know we can't really address the "unknown unknown" dragons easily, but actually completing recovery on RO mounts seems straightforward to me... we allow half of recovery to go, and disallow the other half. Seems plainly broken.

Quoted 36 lines · Eric Sandeen

Quoted 29 lines · Dave Chinner

Quoted 15 lines · Darrick J. Wong

Quoted 14 lines · Brian Foster

Quoted 13 lines · Eric Sandeen

When we do log recovery on a readonly mount, unlinked inode processing does not happen due to the readonly checks in xfs_inactive(), which are trying to prevent any I/O on a readonly mount.

This is misguided - we do I/O on readonly mounts all the time, for consistency; for example, log recovery. So do the same RDONLY flag twiddling around xfs_log_mount_finish() as we do around xfs_log_mount(), for the same reason.

This all cries out for a big rework but for now this is a simple fix to an obvious problem.

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

Signed-off-by Eric Sandeen <[email protected]>

---

Both patches look ok, so I'll put them on the test queue for -rc4.

Reviewed-by Darrick J. Wong <[email protected]>

FWIW, I don't think this is a -rc candidate. Making log recovery process unlinked inode transactions on read-only mounts is a pretty major change in behaviour. Who knows exactly what dragons are lurking at lower layers that have never been run in this context until now.

Quoted 5 lines

Also, it's not urgent - we've lived with this behaviour for years - so waiting a month for the next merge window is not going to hurt anyone and it gives us a chance to test it - XFS developers are the people who should be burnt by the lurking dragons, not users who updated to a late -rcX kernel....

Quoted 6 lines

To shield Darrick a bit ;) I was agitating/asking for sooner, but admittedly that was a little bit selfish on my part.

Still, we have had field reports of people with /gigabytes/ missing from the root filesystem, and it was not fixable without an xfs_repair. Which on a root filesystem is ... special.

That's information that should be in the commit message....

Quoted 5 lines

So, my fault for getting it sent late, for sure - but I do think it's an important fix. I know we can't really address the "unknown unknown" dragons easily, but actually completing recovery on RO mounts seems straightforward to me... we allow half of recovery to go, and disallow the other half. Seems plainly broken.

I still don't think that makes it an urgent, immediate -rcX fix. It definitely makes it a fix that should go to stable kernels, but that does not mean we should short-cut our integrationa nd testing processes. If anything, it makes it far more important to ensure the change is safe and well tested, because it's going to be distributed to /everyone/ in the near future through the stable update process, distros included.

As I've already said: rushing fixes upstream without adequate test time is almost always the wrong thing to do. Call me conservative, but I have plenty of scars to justify being careful about pushing fixes too quickly.

I'm more worried about the impact on the unknown number of read-only filesystems out there across the entire userbase that have the potential to process inodes that have been sitting orphaned for years than I am about the few recent users who have had to run xfs-repair on their root filesystem to fix this up due to the nature of ro->rw transition in root filesystem mounting. Let's make really sure everything is OK before we expose it to all our users running stable/distro kernels....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]