[PATCH 0/2 resend] xfs: readonly handling changes

[PATCH 0/2 resend] xfs: readonly handling changes

[Eric Sandeen]

A couple of changes to how xfs behaves w.r.t. readonly
mounts.

1) write unmount record even for RO mounts (which may have done recovery)
2) remove readonly checks from xfs_release and xfs_inactive paths

Both of these stem from the fact that a readonly mount (as opposed to a
ro,norecovery mount) is /not/ guaranteed to do no writes to the block
device; in fact we write straightaway when we replay the log on an ro
mount.  Other "OMG don't write!" leftovers linger, and cause issues as
described in the following 2 patches.

Dave suggested grand plans for coalescing more of this into common
paths ala remount handling, i.e. a RO mount gets mounted RW and then
just goes through the same transition to RO at the end, but I haven't
put all that together yet, and these patches address a couple bugs in
a more targeted fashion until that can happen.

Dave also objected earlier to my 2nd patch which allows orphan recovery
to proceed on an ro mount.  There may be filesystems out there with
fairly long orphan inode lists which have never been recovered due to
this bug, and I guess the concern was that $SOMEHOW, processing them now
would cause $PROBLEMS.  I'm at a loss for how to address these concerns;
I tried to come up with a way to craft such a filesystem for a regression
test, but failed.  Once the flaw has been fixed in the kernel, I don't
know how to recreate the situation at will for testing.

Christoph was concerned about the xfstest which demonstrates this flaw
still failing, so resending these patches to see if we can make some
progress.

Thanks,
-Eric

[PATCH 1/2] xfs: write unmount record for ro mounts

[Eric Sandeen]

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 <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---


diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 0053bcf..0bd1341 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -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 cd 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)));

[PATCH 2/2] xfs: toggle readonly state around xfs_log_mount_finish

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

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---


diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 0bd1341..ca56d85 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -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;
 }
 

Re: [PATCH 1/2] xfs: write unmount record for ro mounts

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs: toggle readonly state around xfs_log_mount_finish

[Christoph Hellwig]

On Fri, Jul 21, 2017 at 10:25:35AM -0500, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> 
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 0bd1341..ca56d85 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -743,16 +743,23 @@
>  	struct xfs_mount	*mp)
>  {
>  	int	error = 0;
> +	int	readonly = (mp->m_flags & XFS_MOUNT_RDONLY);

bool?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/2 V2] xfs: toggle readonly state around xfs_log_mount_finish

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

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---

V2: bool "readonly" per Christoph's request

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 0bd1341..ca56d85 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -743,16 +743,23 @@
 	struct xfs_mount	*mp)
 {
 	int	error = 0;
+	bool	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;
 }
 

Re: [PATCH 2/2 V2] xfs: toggle readonly state around xfs_log_mount_finish

[Darrick J. Wong]

On Fri, Aug 11, 2017 at 12:45:05PM -0700, Eric Sandeen wrote:
> 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 <sandeen@redhat.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> 
> V2: bool "readonly" per Christoph's request
> 

Looks good, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 0bd1341..ca56d85 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -743,16 +743,23 @@
>  	struct xfs_mount	*mp)
>  {
>  	int	error = 0;
> +	bool	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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html