minor ->release fixups and cleanups

minor ->release fixups and cleanups

[Christoph Hellwig]

Hi all,

Al Viro pointed out a while ago that while ->release returns an errno
it is never checked and rather pointless.  He pointed to XFS as an
instance that can return one.

This cleans up the code including remove the separate xfs_release
helper, and also tacks on another fixlet to only release our write
related resources when the file actually was opened for writing.

[PATCH 1/2] xfs: remove xfs_release

[Christoph Hellwig]

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

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

Re: [PATCH 1/2] xfs: remove xfs_release

[Brian Foster]

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
> 

Re: [PATCH 1/2] xfs: remove xfs_release

[Christoph Hellwig]

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.

Re: [PATCH 1/2] xfs: remove xfs_release

[Brian Foster]

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

Re: [PATCH 1/2] xfs: remove xfs_release

[Darrick J. Wong]

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

Re: [PATCH 1/2] xfs: remove xfs_release

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 2/2] xfs: shortcut xfs_file_release for read-only file descriptors

[Christoph Hellwig]

xfs_file_release currently performs flushing of truncated blocks and
freeing of the post-EOF speculative preallocation for all file
descriptors as long as they are not on a read-only mount.  Switch to
check for FMODE_WRITE instead as we should only perform these actions
on writable file descriptors, and no such file descriptors can be
created on a read-only mount.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 72680edf2ceb..06f0eb25c7cc 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1066,7 +1066,7 @@ xfs_file_release(
 	struct xfs_inode	*ip = XFS_I(inode);
 	struct xfs_mount	*mp = ip->i_mount;
 
-	if (mp->m_flags & XFS_MOUNT_RDONLY)
+	if (!(file->f_mode & FMODE_WRITE))
 		return 0;
 	
 	if (XFS_FORCED_SHUTDOWN(mp))
-- 
2.20.1

Re: [PATCH 2/2] xfs: shortcut xfs_file_release for read-only file descriptors

[Brian Foster]

On Mon, Sep 16, 2019 at 02:20:41PM +0200, Christoph Hellwig wrote:
> xfs_file_release currently performs flushing of truncated blocks and
> freeing of the post-EOF speculative preallocation for all file
> descriptors as long as they are not on a read-only mount.  Switch to
> check for FMODE_WRITE instead as we should only perform these actions
> on writable file descriptors, and no such file descriptors can be
> created on a read-only mount.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 72680edf2ceb..06f0eb25c7cc 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1066,7 +1066,7 @@ xfs_file_release(
>  	struct xfs_inode	*ip = XFS_I(inode);
>  	struct xfs_mount	*mp = ip->i_mount;
>  
> -	if (mp->m_flags & XFS_MOUNT_RDONLY)
> +	if (!(file->f_mode & FMODE_WRITE))
>  		return 0;
>  	

Didn't Dave have a variant of this patch for dealing with a
fragmentation issue (IIRC)? Anyways, seems fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	if (XFS_FORCED_SHUTDOWN(mp))
> -- 
> 2.20.1
> 

Re: [PATCH 2/2] xfs: shortcut xfs_file_release for read-only file descriptors

[Christoph Hellwig]

On Mon, Sep 16, 2019 at 08:53:23AM -0400, Brian Foster wrote:
> Didn't Dave have a variant of this patch for dealing with a
> fragmentation issue (IIRC)? Anyways, seems fine:

I don't really remember one.  But maybe Dave can chime in.

Re: [PATCH 2/2] xfs: shortcut xfs_file_release for read-only file descriptors

[Darrick J. Wong]

On Wed, Sep 18, 2019 at 06:50:00PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 16, 2019 at 08:53:23AM -0400, Brian Foster wrote:
> > Didn't Dave have a variant of this patch for dealing with a
> > fragmentation issue (IIRC)? Anyways, seems fine:
> 
> I don't really remember one.  But maybe Dave can chime in.

He did[1], but IIRC the discussion petered out because we didn't have a
good way to measure the long term fragmentation effects of doing this.
I sent in a second patch[2] that only trimmed posteof blocks the first
time a file is closed, which reduced fragmentation dramatically on
workloads where there are a lot of synchronous open-append-close writes
(e.g. logging daemons).

None of that stuff ever got merged, so maybe it's time to revisit these.

--D

[1] https://lore.kernel.org/linux-xfs/20190207050813.24271-2-david@fromorbit.com/
[2] https://lore.kernel.org/linux-xfs/155259894034.30230.7188877605950498518.stgit@magnolia/