[2/2] xfs: remove readonly checks from xfs_release & xfs_inactive
diff mbox

Message ID 36942625-073a-56ba-4d31-cd9511f3bfb8@sandeen.net
State New
Headers show

Commit Message

Eric Sandeen March 9, 2017, 8:24 p.m. UTC
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>
---

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

Comments

Eric Sandeen March 9, 2017, 8:39 p.m. UTC | #1
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
> 
--
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
Brian Foster March 13, 2017, 1:23 p.m. UTC | #2
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
--
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
Eric Sandeen March 13, 2017, 10:16 p.m. UTC | #3
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
--
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
Brian Foster March 14, 2017, 11:35 a.m. UTC | #4
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
--
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
diff mbox

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