xfs
[Top] [All Lists]

Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 06/13] xfs: xfs_sync_data is redundant.
From: Ben Myers <bpm@xxxxxxx>
Date: Wed, 3 Oct 2012 19:05:52 -0500
Cc: Brian Foster <bfoster@xxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20121002205124.GS23520@dastard>
References: <1348807485-20165-1-git-send-email-david@xxxxxxxxxxxxx> <1348807485-20165-7-git-send-email-david@xxxxxxxxxxxxx> <5069F9B0.50804@xxxxxxxxxx> <20121002001021.GJ23520@dastard> <506A38D1.6090204@xxxxxxxxxx> <506AE5AD.60509@xxxxxxxxxx> <20121002205124.GS23520@dastard>
User-agent: Mutt/1.5.20 (2009-06-14)
Hey Dave,

On Wed, Oct 03, 2012 at 06:51:24AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> writeback_inodes_sb_if_idle() is not sufficient to trigger delalloc
> conversion fast enough to prevent spurious ENOSPC whent here are
> hundreds of writers, thousands of small files and GBs of free RAM.
> Change this to use sync_sb_inodes() to block callers while we wait
> for writeback like the previous xfs_flush_inodes implementation did.
> 
> We have to hold the s_umount lock here, but because this call can
> nest inside i_mutex (the parent directory in the create case, held
> by the VFS), we have to use down_read_trylock() to avoid potential
> deadlocks. In practice, this trylock will succeed on almost every
> attempt as unmount/remount type operations are exceedingly rare.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_inode.h |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index da69c18..b3dabe9 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -294,7 +294,12 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
>  static inline void
>  xfs_flush_inodes(struct xfs_inode *ip)
>  {
> -     writeback_inodes_sb_if_idle(VFS_I(ip)->i_sb, WB_REASON_FS_FREE_SPACE);
> +     struct super_block *sb = VFS_I(ip)->i_sb;
> +
> +     if (down_read_trylock(&sb->s_umount)) {
> +             sync_inodes_sb(sb);
> +             up_read(&sb->s_umount);
> +     }
>  }
>  
>  /*
> 

Is this the one you want to go with?  I'd prefer to apply this to patch 6
itself rather than check in the regression followed by the fix.  If you'd like
resubmit this patch that's great, otherwise I'll be happy to repost the result.

Thanks again Brian.

Regards,
Ben

<Prev in Thread] Current Thread [Next in Thread>