From: Brian Foster <bfoster@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 04/11] xfs: don't try to map blocks beyond i_size in writeback
Date: Fri, 1 Feb 2019 07:46:42 -0500 [thread overview]
Message-ID: <20190201124641.GA40798@bfoster> (raw)
In-Reply-To: <20190201072520.GB14711@lst.de>
On Fri, Feb 01, 2019 at 08:25:20AM +0100, Christoph Hellwig wrote:
> On Thu, Jan 31, 2019 at 01:11:09PM -0500, Brian Foster wrote:
> > The code looks fine, but I don't see any more value in this code than
> > the similar code down in xfs_iomap_write_allocate(). The comment implies
> > this skips writeback for the rest of the file, but AFACT the higher
> > level page->index code in xfs_do_writepage() already does that. All
> > these checks do is skip the remaining blocks in the current page. When
> > you consider that we're most likely sending an I/O in the latter case
> > either way, I'm curious why we'd bother to keep this around at all.
>
> It just seems a little pointless to take locks and do a lookup in the
> extent tree. But I can try dropping it and re-run tests without it.
I agree that is pointless, but what's the point of saving lock cycles
and an in-core extent lookup when we've already committed to sending an
I/O? AFAICT we may also still have to cycle through however many pages
are still referenced by the current pagevec. The racing truncate is
going to wait on the higher level page lock and page writeback state
before it ever gets to the point of contending on ilock.
More broadly, this is a codepath that is historically brittle with
regard to uncommon branches in the code, whether that be incorrect error
handling (page discard), the similar truncate detection code in
xfs_iomap_write_allocate() that clearly had broken -EAGAIN handling code
for who knows how long, etc. IMO, this all stems from lack of good test
coverage in the area of writeback failure.
Unless I'm missing something where this code is required for correctness
or somehow provides a measureable performance benefit, I think we're
better off keeping this path as simple as possible. It should be fairly
easy to provide test coverage for the page granularity truncate handling
code (generic/524 may already cover this). I'm not so sure about that
for the block granularity handling without having some kind of
additional instrumentation.
Brian
next prev parent reply other threads:[~2019-02-01 12:46 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-31 7:55 make delalloc conversion more robust and clear Christoph Hellwig
2019-01-31 7:55 ` [PATCH 01/11] FOLD: improve xfs_bmapi_delalloc Christoph Hellwig
2019-01-31 18:09 ` Brian Foster
2019-02-01 7:23 ` Christoph Hellwig
2019-02-01 7:28 ` Christoph Hellwig
2019-02-01 12:46 ` Brian Foster
2019-02-01 16:08 ` Christoph Hellwig
2019-01-31 7:55 ` [PATCH 02/11] xfs: remove the io_type field from the writeback context and ioend Christoph Hellwig
2019-01-31 18:10 ` Brian Foster
2019-01-31 7:55 ` [PATCH 03/11] xfs: remove the s_maxbytes checks in xfs_map_blocks Christoph Hellwig
2019-01-31 18:10 ` Brian Foster
2019-01-31 7:55 ` [PATCH 04/11] xfs: don't try to map blocks beyond i_size in writeback Christoph Hellwig
2019-01-31 18:11 ` Brian Foster
2019-02-01 7:25 ` Christoph Hellwig
2019-02-01 12:46 ` Brian Foster [this message]
2019-02-01 16:07 ` Christoph Hellwig
2019-01-31 7:55 ` [PATCH 05/11] xfs: simplify the xfs_bmap_btree_to_extents calling conventions Christoph Hellwig
2019-01-31 18:11 ` Brian Foster
2019-01-31 7:55 ` [PATCH 06/11] xfs: factor out two helpers from xfs_bmapi_write Christoph Hellwig
2019-01-31 18:28 ` Brian Foster
2019-01-31 7:55 ` [PATCH 07/11] xfs: split XFS_BMAPI_DELALLOC handling " Christoph Hellwig
2019-01-31 19:28 ` Brian Foster
2019-01-31 7:55 ` [PATCH 08/11] xfs: move transaction handling to xfs_bmapi_convert_delalloc Christoph Hellwig
2019-01-31 19:28 ` Brian Foster
2019-01-31 7:55 ` [PATCH 09/11] xfs: move stat accounting " Christoph Hellwig
2019-01-31 19:29 ` Brian Foster
2019-01-31 7:55 ` [PATCH 10/11] xfs: move xfs_iomap_write_allocate to xfs_aops.c Christoph Hellwig
2019-01-31 19:31 ` Brian Foster
2019-01-31 7:55 ` [PATCH 11/11] xfs: retry COW fork delalloc conversion when no extent was found Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190201124641.GA40798@bfoster \
--to=bfoster@redhat.com \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).