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