[Top] [All Lists]

Re: [PATCH 0/2] Improve writeout pattern from xfs_flush_pages()

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 0/2] Improve writeout pattern from xfs_flush_pages()
From: Jan Kara <jack@xxxxxxx>
Date: Thu, 4 Aug 2011 12:36:16 +0200
Cc: Jan Kara <jack@xxxxxxx>, xfs@xxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx
In-reply-to: <20110803214206.GA20477@xxxxxxxxxxxxx>
References: <1312404545-15400-1-git-send-email-jack@xxxxxxx> <20110803214206.GA20477@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Wed 03-08-11 17:42:06, Christoph Hellwig wrote:
> On Wed, Aug 03, 2011 at 10:49:03PM +0200, Jan Kara wrote:
> > 
> >   Hi,
> > 
> >   at one of customer's machines, I've spotted an issue that sync(1) called
> > after writing a single huge file has been achieving rather low throughput. 
> > After
> > debugging this with blktrace, I've found that the culprit was in flusher 
> > thread
> > racing with page writeout happening from XFS sync code. The patches below 
> > helped
> > that case. Although they are not a complete solution, I belive they are 
> > useful
> > anyway so please consider merging them...
> We currently have three calls to xfs_flush_pages with XBF_ASYNC set:
>  - xfs_setattr_size
>  - xfs_sync_inode_data
>  - xfs_release
> The first one actually is a synchronous writeout, just implemented in
> a rather odd way by doing the xfs_ioend_wait right after it, so your
> change is actively harmful for it.
  Oh, right. BTW cannot be truncate livelocked on a busy file because of
that xfs_ioend_wait()?

> The second is only called from xfs_flush_worker, which is the workqueue
> offload when we hit ENOSPC.  I can see how this might race with the
> writeback code, but the correct fix is to replace it with a call to
> writeback_inodes_sb(_if_idle) on that one is fixed to do a trylock on
> s_umount and thus won't deadlock.

> The third one is opportunistic writeout if a file got truncated down on
> final release.  filemap_flush probably is fine here, but there's no need
> for a range version.  If you replace it with filemap_flush please also
> kill the useless wrapper while you're at it.
  Do you mean xfs_flush_pages()? OK, I can do that.

  Thanks for having a look.

Jan Kara <jack@xxxxxxx>

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