[PATCH 0/2] xfs: readonly handling changes

[PATCH 0/2] xfs: readonly handling changes

[Eric Sandeen]

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

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

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 do that straightaway when we replay the log.  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.

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

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index b1469f0..62176b8 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 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: remove readonly checks from xfs_release & xfs_inactive

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

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 <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index edfa6a5..bf74165 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -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

Re: [PATCH 2/2] xfs: remove readonly checks from xfs_release & xfs_inactive

[Eric Sandeen]

On 3/9/17 2:24 PM, Eric Sandeen wrote:
> 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;

sorry, above is wrong.

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

> 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 <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index edfa6a5..bf74165 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] xfs: remove readonly checks from xfs_release & xfs_inactive

[Brian Foster]

On Thu, Mar 09, 2017 at 02:39:59PM -0600, Eric Sandeen wrote:
> On 3/9/17 2:24 PM, Eric Sandeen wrote:
> > 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;
> 
> sorry, above is wrong.
> 

Care to elaborate? :) Do you mean we should not be making modifications
here after (ro) 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 - 
> 
> 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...
> 
> > 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 <sandeen@redhat.com>
> > ---
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index edfa6a5..bf74165 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -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;
> > -

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.

Brian

> >  	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 majordomo@vger.kernel.org
> > 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] xfs: remove readonly checks from xfs_release & xfs_inactive

[Eric Sandeen]

On 3/13/17 8:23 AM, Brian Foster wrote:
> 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

Re: [PATCH 2/2] xfs: remove readonly checks from xfs_release & xfs_inactive

[Brian Foster]

On Mon, Mar 13, 2017 at 05:16:07PM -0500, Eric Sandeen wrote:
> On 3/13/17 8:23 AM, Brian Foster wrote:
> > 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.
> 

Ok, I suppose we'd have to filter out from "mounting" context. That is
only for the case where we run log recovery though, right?

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

Hmm, that does sound like a cleaner approach.

Brian

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

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

V2: And now for something completely different...

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 62176b8..374edc1 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 2/2 V2] xfs: toggle readonly state around xfs_log_mount_finish

[Brian Foster]

On Tue, Mar 14, 2017 at 06:23:57PM -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>

> V2: And now for something completely different...
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 62176b8..374edc1 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;
>  }
>  
> 
> --
> 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

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

[Brian Foster]

On Thu, Mar 09, 2017 at 02:15:01PM -0600, Eric Sandeen wrote:
> 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 b1469f0..62176b8 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 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darrick J. Wong]

On Wed, Mar 15, 2017 at 07:36:29AM -0400, Brian Foster wrote:
> On Tue, Mar 14, 2017 at 06:23:57PM -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>
> > ---
> > 

Both patches look ok, so I'll put them on the test queue for -rc4.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > V2: And now for something completely different...
> > 
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index 62176b8..374edc1 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;
> >  }
> >  
> > 
> > --
> > 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
> --
> 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

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

[Dave Chinner]

On Thu, Mar 16, 2017 at 12:15:00PM -0700, Darrick J. Wong wrote:
> On Wed, Mar 15, 2017 at 07:36:29AM -0400, Brian Foster wrote:
> > On Tue, Mar 14, 2017 at 06:23:57PM -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>
> > > ---
> > > 
> 
> Both patches look ok, so I'll put them on the test queue for -rc4.
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Eric Sandeen]

On 3/16/17 4:42 PM, Dave Chinner wrote:
> On Thu, Mar 16, 2017 at 12:15:00PM -0700, Darrick J. Wong wrote:
>> On Wed, Mar 15, 2017 at 07:36:29AM -0400, Brian Foster wrote:
>>> On Tue, Mar 14, 2017 at 06:23:57PM -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>
>>>> ---
>>>>
>>
>> Both patches look ok, so I'll put them on the test queue for -rc4.
>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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....

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.

-Eric

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

[Dave Chinner]

On Thu, Mar 16, 2017 at 04:52:43PM -0700, Eric Sandeen wrote:
> On 3/16/17 4:42 PM, Dave Chinner wrote:
> > On Thu, Mar 16, 2017 at 12:15:00PM -0700, Darrick J. Wong wrote:
> >> On Wed, Mar 15, 2017 at 07:36:29AM -0400, Brian Foster wrote:
> >>> On Tue, Mar 14, 2017 at 06:23:57PM -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>
> >>>> ---
> >>>>
> >>
> >> Both patches look ok, so I'll put them on the test queue for -rc4.
> >> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > 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....
> 
> 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....

> 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.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Sat, Mar 18, 2017 at 06:38:35PM +1100, Dave Chinner wrote:
> On Thu, Mar 16, 2017 at 04:52:43PM -0700, Eric Sandeen wrote:
> > On 3/16/17 4:42 PM, Dave Chinner wrote:
> > > On Thu, Mar 16, 2017 at 12:15:00PM -0700, Darrick J. Wong wrote:
> > >> On Wed, Mar 15, 2017 at 07:36:29AM -0400, Brian Foster wrote:
> > >>> On Tue, Mar 14, 2017 at 06:23:57PM -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>
> > >>>> ---
> > >>>>
> > >>
> > >> Both patches look ok, so I'll put them on the test queue for -rc4.
> > >> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > 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....
> > 
> > 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....
> 
> > 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....

FWIW I let this run w/ all my testing configs during LSF/Vault last week
and I didn't see any new failures.  I'll hold off on sending these patches.

But, waiting for 4.12 does provide the opportunity to add more stressful
tests than what generic/417 does now.  How about a test that creates a
big directory structure + some heavily fragmented files, then opens all
of those files, deletes the directory tree, shuts down the fs, then
attempts a ro mode recovery?  That way we have a lot of files and a lot
of bmap records to get rid of during mount.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com