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

[Brian Foster <bfoster@redhat.com>]

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