| Message ID | 36942625-073a-56ba-4d31-cd9511f3bfb8@sandeen.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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