| Message ID | 20190916122041.24636-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] xfs: remove xfs_release | expand |
On Mon, Sep 16, 2019 at 02:20:40PM +0200, Christoph Hellwig wrote: > We can just move the code directly to xfs_file_release. Additionally > remove the pointless i_mode verification, and the error returns that > are entirely ignored by the calller of ->release. > The caller might not care if this call generates errors, but shouldn't we care if something fails? IOW, perhaps we should have an exit path with a WARN_ON_ONCE() or some such to indicate that an unhandled error has occurred..? > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_file.c | 66 ++++++++++++++++++++++++++++++++++++-- > fs/xfs/xfs_inode.c | 80 ---------------------------------------------- > fs/xfs/xfs_inode.h | 1 - > 3 files changed, 63 insertions(+), 84 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index d952d5962e93..72680edf2ceb 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -1060,10 +1060,70 @@ xfs_dir_open( > > STATIC int > xfs_file_release( > - struct inode *inode, > - struct file *filp) > + struct inode *inode, > + struct file *file) > { > - return xfs_release(XFS_I(inode)); > + struct xfs_inode *ip = XFS_I(inode); > + struct xfs_mount *mp = ip->i_mount; > + > + if (mp->m_flags & XFS_MOUNT_RDONLY) > + return 0; > + Whitespace damage on the above line. > + if (XFS_FORCED_SHUTDOWN(mp)) > + return 0; > + > + /* > + * If we previously truncated this file and removed old data in the > + * process, we want to initiate "early" writeout on the last close. > + * This is an attempt to combat the notorious NULL files problem which > + * is particularly noticeable from a truncate down, buffered (re-)write > + * (delalloc), followed by a crash. What we are effectively doing here > + * is significantly reducing the time window where we'd otherwise be > + * exposed to that problem. > + */ > + if (xfs_iflags_test_and_clear(ip, XFS_ITRUNCATED)) { > + xfs_iflags_clear(ip, XFS_IDIRTY_RELEASE); > + if (ip->i_delayed_blks > 0) > + filemap_flush(inode->i_mapping); > + return 0; > + } > + > + if (inode->i_nlink == 0 || !xfs_can_free_eofblocks(ip, false)) > + return 0; > + > + /* > + * Check if the inode is being opened, written and closed frequently and > + * we have delayed allocation blocks outstanding (e.g. streaming writes > + * from the NFS server), truncating the blocks past EOF will cause > + * fragmentation to occur. > + * > + * In this case don't do the truncation, but we have to be careful how > + * we detect this case. Blocks beyond EOF show up as i_delayed_blks even > + * when the inode is clean, so we need to truncate them away first > + * before checking for a dirty release. Hence on the first dirty close > + * we will still remove the speculative allocation, but after that we > + * will leave it in place. > + */ > + if (xfs_iflags_test(ip, XFS_IDIRTY_RELEASE)) > + return 0; > + > + /* > + * If we can't get the iolock just skip truncating the blocks past EOF > + * because we could deadlock with the mmap_sem otherwise. We'll get > + * another chance to drop them once the last reference to the inode is > + * dropped, so we'll never leak blocks permanently. > + */ > + if (xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) { > + xfs_free_eofblocks(ip); > + xfs_iunlock(ip, XFS_IOLOCK_EXCL); > + } > + > + /* > + * Delalloc blocks after truncation means it really is dirty. > + */ This isn't necessarily true now that we ignore errors. I.e., this also subtly changes the logic of the function. Brian > + if (ip->i_delayed_blks) > + xfs_iflags_set(ip, XFS_IDIRTY_RELEASE); > + return 0; > } > > STATIC int > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 18f4b262e61c..b21405540c37 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -1590,86 +1590,6 @@ xfs_itruncate_extents_flags( > return error; > } > > -int > -xfs_release( > - xfs_inode_t *ip) > -{ > - xfs_mount_t *mp = ip->i_mount; > - int error; > - > - 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; > - > - /* > - * If we previously truncated this file and removed old data > - * in the process, we want to initiate "early" writeout on > - * the last close. This is an attempt to combat the notorious > - * NULL files problem which is particularly noticeable from a > - * truncate down, buffered (re-)write (delalloc), followed by > - * a crash. What we are effectively doing here is > - * significantly reducing the time window where we'd otherwise > - * be exposed to that problem. > - */ > - truncated = xfs_iflags_test_and_clear(ip, XFS_ITRUNCATED); > - if (truncated) { > - xfs_iflags_clear(ip, XFS_IDIRTY_RELEASE); > - if (ip->i_delayed_blks > 0) { > - error = filemap_flush(VFS_I(ip)->i_mapping); > - if (error) > - return error; > - } > - } > - } > - > - if (VFS_I(ip)->i_nlink == 0) > - return 0; > - > - if (xfs_can_free_eofblocks(ip, false)) { > - > - /* > - * Check if the inode is being opened, written and closed > - * frequently and we have delayed allocation blocks outstanding > - * (e.g. streaming writes from the NFS server), truncating the > - * blocks past EOF will cause fragmentation to occur. > - * > - * In this case don't do the truncation, but we have to be > - * careful how we detect this case. Blocks beyond EOF show up as > - * i_delayed_blks even when the inode is clean, so we need to > - * truncate them away first before checking for a dirty release. > - * Hence on the first dirty close we will still remove the > - * speculative allocation, but after that we will leave it in > - * place. > - */ > - if (xfs_iflags_test(ip, XFS_IDIRTY_RELEASE)) > - return 0; > - /* > - * If we can't get the iolock just skip truncating the blocks > - * past EOF because we could deadlock with the mmap_sem > - * otherwise. We'll get another chance to drop them once the > - * last reference to the inode is dropped, so we'll never leak > - * blocks permanently. > - */ > - if (xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) { > - error = xfs_free_eofblocks(ip); > - xfs_iunlock(ip, XFS_IOLOCK_EXCL); > - if (error) > - return error; > - } > - > - /* delalloc blocks after truncation means it really is dirty */ > - if (ip->i_delayed_blks) > - xfs_iflags_set(ip, XFS_IDIRTY_RELEASE); > - } > - return 0; > -} > - > /* > * xfs_inactive_truncate > * > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index 558173f95a03..4299905135b2 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -410,7 +410,6 @@ enum layout_break_reason { > (((pip)->i_mount->m_flags & XFS_MOUNT_GRPID) || \ > (VFS_I(pip)->i_mode & S_ISGID)) > > -int xfs_release(struct xfs_inode *ip); > void xfs_inactive(struct xfs_inode *ip); > int xfs_lookup(struct xfs_inode *dp, struct xfs_name *name, > struct xfs_inode **ipp, struct xfs_name *ci_name); > -- > 2.20.1 >
On Mon, Sep 16, 2019 at 08:53:11AM -0400, Brian Foster wrote: > The caller might not care if this call generates errors, but shouldn't > we care if something fails? IOW, perhaps we should have an exit path > with a WARN_ON_ONCE() or some such to indicate that an unhandled error > has occurred..? Not sure there is much of a point. Basically all errors are either due to a forced shutdown or cause a forced shutdown anyway, so we'll already get warnings.
On Wed, Sep 18, 2019 at 06:49:38PM +0200, Christoph Hellwig wrote: > On Mon, Sep 16, 2019 at 08:53:11AM -0400, Brian Foster wrote: > > The caller might not care if this call generates errors, but shouldn't > > we care if something fails? IOW, perhaps we should have an exit path > > with a WARN_ON_ONCE() or some such to indicate that an unhandled error > > has occurred..? > > Not sure there is much of a point. Basically all errors are either > due to a forced shutdown or cause a forced shutdown anyway, so we'll > already get warnings. Well, what's the point of this change in the first place? I see various error paths that aren't directly related to shutdown. A writeback submission error for instance looks like it will warn, but not necessarily shut down (and the filemap_flush() call is already within a !XFS_FORCED_SHUTDOWN() check). So not all errors are associated with or cause shutdown. I suppose you could audit the various error paths that lead back into this function and document that further if you really wanted to go that route... Also, you snipped the rest of my feedback... how does the fact that the caller doesn't care about errors have anything to do with the fact that the existing logic within this function does? I'm not convinced the changes here are quite correct, but at the very least the commit log description is lacking/misleading. Brian
On Wed, Sep 18, 2019 at 02:12:04PM -0400, Brian Foster wrote: > On Wed, Sep 18, 2019 at 06:49:38PM +0200, Christoph Hellwig wrote: > > On Mon, Sep 16, 2019 at 08:53:11AM -0400, Brian Foster wrote: > > > The caller might not care if this call generates errors, but shouldn't > > > we care if something fails? IOW, perhaps we should have an exit path > > > with a WARN_ON_ONCE() or some such to indicate that an unhandled error > > > has occurred..? > > > > Not sure there is much of a point. Basically all errors are either > > due to a forced shutdown or cause a forced shutdown anyway, so we'll > > already get warnings. > > Well, what's the point of this change in the first place? I see various > error paths that aren't directly related to shutdown. A writeback > submission error for instance looks like it will warn, but not > necessarily shut down (and the filemap_flush() call is already within a > !XFS_FORCED_SHUTDOWN() check). So not all errors are associated with or > cause shutdown. I suppose you could audit the various error paths that > lead back into this function and document that further if you really > wanted to go that route... I agree with Brian, there ought to be some kind of warning that some error happened with inode XXX even if we do end up shutting down immediately afterwards. > Also, you snipped the rest of my feedback... how does the fact that the > caller doesn't care about errors have anything to do with the fact that > the existing logic within this function does? I'm not convinced the > changes here are quite correct, but at the very least the commit log > description is lacking/misleading. I was wondering that too -- if filemap_flush, we'd stop immediately, but now we keep going. That certainly seems like a behavior change that ought to be a separate patch from combining the two release functions? --D > Brian
On Wed, Sep 18, 2019 at 11:21:35AM -0700, Darrick J. Wong wrote: > On Wed, Sep 18, 2019 at 02:12:04PM -0400, Brian Foster wrote: > > On Wed, Sep 18, 2019 at 06:49:38PM +0200, Christoph Hellwig wrote: > > > On Mon, Sep 16, 2019 at 08:53:11AM -0400, Brian Foster wrote: > > > > The caller might not care if this call generates errors, but shouldn't > > > > we care if something fails? IOW, perhaps we should have an exit path > > > > with a WARN_ON_ONCE() or some such to indicate that an unhandled error > > > > has occurred..? > > > > > > Not sure there is much of a point. Basically all errors are either > > > due to a forced shutdown or cause a forced shutdown anyway, so we'll > > > already get warnings. > > > > Well, what's the point of this change in the first place? I see various > > error paths that aren't directly related to shutdown. A writeback > > submission error for instance looks like it will warn, but not > > necessarily shut down (and the filemap_flush() call is already within a > > !XFS_FORCED_SHUTDOWN() check). So not all errors are associated with or > > cause shutdown. I suppose you could audit the various error paths that > > lead back into this function and document that further if you really > > wanted to go that route... > > I agree with Brian, there ought to be some kind of warning that some > error happened with inode XXX even if we do end up shutting down > immediately afterwards. FWIW, we have precedence for that - see xfs_inactive_ifree(), which logs errors noisily because we can't return errors to the VFS inode teardown path (i.e. evict -> destroy_inode -> xfs_fs_destroy_inode -> xfs_inactive path). These were originally added because a static error return checker flagged xfs_inactive() as a place where errors were silently ignored and users had no indication that somethign bad had happened to their file. -> release -> xfs_file_release -> xfs_release is no different in this respect.... Cheers, Dave.
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index d952d5962e93..72680edf2ceb 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -1060,10 +1060,70 @@ xfs_dir_open( STATIC int xfs_file_release( - struct inode *inode, - struct file *filp) + struct inode *inode, + struct file *file) { - return xfs_release(XFS_I(inode)); + struct xfs_inode *ip = XFS_I(inode); + struct xfs_mount *mp = ip->i_mount; + + if (mp->m_flags & XFS_MOUNT_RDONLY) + return 0; + + if (XFS_FORCED_SHUTDOWN(mp)) + return 0; + + /* + * If we previously truncated this file and removed old data in the + * process, we want to initiate "early" writeout on the last close. + * This is an attempt to combat the notorious NULL files problem which + * is particularly noticeable from a truncate down, buffered (re-)write + * (delalloc), followed by a crash. What we are effectively doing here + * is significantly reducing the time window where we'd otherwise be + * exposed to that problem. + */ + if (xfs_iflags_test_and_clear(ip, XFS_ITRUNCATED)) { + xfs_iflags_clear(ip, XFS_IDIRTY_RELEASE); + if (ip->i_delayed_blks > 0) + filemap_flush(inode->i_mapping); + return 0; + } + + if (inode->i_nlink == 0 || !xfs_can_free_eofblocks(ip, false)) + return 0; + + /* + * Check if the inode is being opened, written and closed frequently and + * we have delayed allocation blocks outstanding (e.g. streaming writes + * from the NFS server), truncating the blocks past EOF will cause + * fragmentation to occur. + * + * In this case don't do the truncation, but we have to be careful how + * we detect this case. Blocks beyond EOF show up as i_delayed_blks even + * when the inode is clean, so we need to truncate them away first + * before checking for a dirty release. Hence on the first dirty close + * we will still remove the speculative allocation, but after that we + * will leave it in place. + */ + if (xfs_iflags_test(ip, XFS_IDIRTY_RELEASE)) + return 0; + + /* + * If we can't get the iolock just skip truncating the blocks past EOF + * because we could deadlock with the mmap_sem otherwise. We'll get + * another chance to drop them once the last reference to the inode is + * dropped, so we'll never leak blocks permanently. + */ + if (xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) { + xfs_free_eofblocks(ip); + xfs_iunlock(ip, XFS_IOLOCK_EXCL); + } + + /* + * Delalloc blocks after truncation means it really is dirty. + */ + if (ip->i_delayed_blks) + xfs_iflags_set(ip, XFS_IDIRTY_RELEASE); + return 0; } STATIC int diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 18f4b262e61c..b21405540c37 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -1590,86 +1590,6 @@ xfs_itruncate_extents_flags( return error; } -int -xfs_release( - xfs_inode_t *ip) -{ - xfs_mount_t *mp = ip->i_mount; - int error; - - 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; - - /* - * If we previously truncated this file and removed old data - * in the process, we want to initiate "early" writeout on - * the last close. This is an attempt to combat the notorious - * NULL files problem which is particularly noticeable from a - * truncate down, buffered (re-)write (delalloc), followed by - * a crash. What we are effectively doing here is - * significantly reducing the time window where we'd otherwise - * be exposed to that problem. - */ - truncated = xfs_iflags_test_and_clear(ip, XFS_ITRUNCATED); - if (truncated) { - xfs_iflags_clear(ip, XFS_IDIRTY_RELEASE); - if (ip->i_delayed_blks > 0) { - error = filemap_flush(VFS_I(ip)->i_mapping); - if (error) - return error; - } - } - } - - if (VFS_I(ip)->i_nlink == 0) - return 0; - - if (xfs_can_free_eofblocks(ip, false)) { - - /* - * Check if the inode is being opened, written and closed - * frequently and we have delayed allocation blocks outstanding - * (e.g. streaming writes from the NFS server), truncating the - * blocks past EOF will cause fragmentation to occur. - * - * In this case don't do the truncation, but we have to be - * careful how we detect this case. Blocks beyond EOF show up as - * i_delayed_blks even when the inode is clean, so we need to - * truncate them away first before checking for a dirty release. - * Hence on the first dirty close we will still remove the - * speculative allocation, but after that we will leave it in - * place. - */ - if (xfs_iflags_test(ip, XFS_IDIRTY_RELEASE)) - return 0; - /* - * If we can't get the iolock just skip truncating the blocks - * past EOF because we could deadlock with the mmap_sem - * otherwise. We'll get another chance to drop them once the - * last reference to the inode is dropped, so we'll never leak - * blocks permanently. - */ - if (xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) { - error = xfs_free_eofblocks(ip); - xfs_iunlock(ip, XFS_IOLOCK_EXCL); - if (error) - return error; - } - - /* delalloc blocks after truncation means it really is dirty */ - if (ip->i_delayed_blks) - xfs_iflags_set(ip, XFS_IDIRTY_RELEASE); - } - return 0; -} - /* * xfs_inactive_truncate * diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index 558173f95a03..4299905135b2 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -410,7 +410,6 @@ enum layout_break_reason { (((pip)->i_mount->m_flags & XFS_MOUNT_GRPID) || \ (VFS_I(pip)->i_mode & S_ISGID)) -int xfs_release(struct xfs_inode *ip); void xfs_inactive(struct xfs_inode *ip); int xfs_lookup(struct xfs_inode *dp, struct xfs_name *name, struct xfs_inode **ipp, struct xfs_name *ci_name);
We can just move the code directly to xfs_file_release. Additionally remove the pointless i_mode verification, and the error returns that are entirely ignored by the calller of ->release. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_file.c | 66 ++++++++++++++++++++++++++++++++++++-- fs/xfs/xfs_inode.c | 80 ---------------------------------------------- fs/xfs/xfs_inode.h | 1 - 3 files changed, 63 insertions(+), 84 deletions(-)