[1/2] xfs: remove xfs_release
diff mbox series

Message ID 20190916122041.24636-2-hch@lst.de
State New
Headers show
Series
  • [1/2] xfs: remove xfs_release
Related show

Commit Message

Christoph Hellwig Sept. 16, 2019, 12:20 p.m. UTC
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(-)

Comments

Brian Foster Sept. 16, 2019, 12:53 p.m. UTC | #1
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
>
Christoph Hellwig Sept. 18, 2019, 4:49 p.m. UTC | #2
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.
Brian Foster Sept. 18, 2019, 6:12 p.m. UTC | #3
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
Darrick J. Wong Sept. 18, 2019, 6:21 p.m. UTC | #4
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
Dave Chinner Sept. 18, 2019, 10:25 p.m. UTC | #5
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.

Patch
diff mbox series

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