[2/2] xfs: shortcut xfs_file_release for read-only file descriptors
diff mbox series

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

Commit Message

hch@lst.de Sept. 16, 2019, 12:20 p.m. UTC
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(-)

Comments

Brian Foster Sept. 16, 2019, 12:53 p.m. UTC | #1
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
>
hch@lst.de Sept. 18, 2019, 4:50 p.m. UTC | #2
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.
Darrick J. Wong Sept. 18, 2019, 5:06 p.m. UTC | #3
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/

Patch
diff mbox series

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