linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Damien Le Moal <Damien.LeMoal@wdc.com>,
	Andreas Gruenbacher <agruenba@redhat.com>,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: lift the xfs writepage code into iomap v2
Date: Thu, 27 Jun 2019 18:32:56 -0700	[thread overview]
Message-ID: <20190628013256.GE5179@magnolia> (raw)
In-Reply-To: <20190627104836.25446-1-hch@lst.de>

On Thu, Jun 27, 2019 at 12:48:23PM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series cleans up the xfs writepage code

Ok.  Patches #2 and #3 are trivial so I put them in my internal branch.

By now I'm sure everyone's noticed that I suspect that patch #7 fixes
the generic/475 crash that Eryu reported, so I've added it to my
internal branch for testing.

Patch #8 is a simple cleanup so I put that one in too.  If I notice any
problems with either of these two patches then I can always back them
out before the next push to for-next.  I'd wanted to make those cleanups
for a while and they're more or less what I would've done.

> and then lifts it to
> fs/iomap.c so that it could be use by other file system.  I've been
> wanting to this for a while so that I could eventually convert gfs2
> over to it, but I never got to it.  Now Damien has a new zonefs
> file system for semi-raw access to zoned block devices that would
> like to use the iomap code instead of reinventing it, so I finally
> had to do the work.

Sooo many conflicted feelings on this question. :)

I agree with Christoph that sharing /high quality/ code in the kernel
has served the kernel well over the years and I want that to continue.
Sharing the lower part of our writeback code so filesystems can opt out
of writing their own code to map dirty pages to storage extents and
attach them to struct bios is (I think) a good strategy.

However, I don't think sharing crap code in the kernel is serving us
well. I dislike this recent development where we decide to wire up XFS
to some new API, beat on it aggressively, and then spend months sorting
out how to make it work the way people think it does.  I do not wish to
see any of the iomap code bit rot to the point that it becomes a
nightmare to someone else.

I think Dave has voiced some valid concerns about our ability to support
this code over the long term once we start sharing it with other fses.
XFS has a longish history of sailing away from generic code so that we
can remove awkward abstractions which aren't working well for us.  If
we're going to continue to go our own way with things like file locking
and mapping I wonder how long we'd keep using the iomap ioends before
moving away again.  How well will that iomap code avoid bitrot once XFS
does that?

We are already past -rc6, so I think the second part of the series
(patches #10-13) is too late for 5.3.  I need more time to think about
how this would work out in the scenario where (a) we take on the extra
work of ensuring that our writeback improvements don't screw things up
for everyone else, or (b) we end up forking away after a while.

To be clear, I don't have a problem with the idea of iomap containing a
common ioend creation library, but I would really like to see what it
looks like to share the code with actual users.  I haven't seen any yet,
though at this early stage I am not surprised.

I think what I want to do is to proceed on a provisional basis -- create
a branch off of the next -rc1 (perhaps omitting the part that removes
xfs ioend processing) and let's see where zonedfs et. al. go from there.

How does that sound?  Who are the other potential users?

> This new version should have addressed all comments from the review,
> except that I haven't split iomap.c, which is a little too invasive
> with other pending changes to the file.  I do however offer to submit
> a split right at the end of the merge window when it is least invasive.

Already working on it, will send it tomorrow or tonight or something.

--D

> Changes since v1:
>  - rebased to the latest xfs for-next tree
>  - keep the preallocated transactions for size updates
>  - rename list_pop to list_pop_entry and related cleanups
>  - better document the nofs context handling
>  - document that the iomap tracepoints are not a stable API

  parent reply	other threads:[~2019-06-28  1:33 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-27 10:48 lift the xfs writepage code into iomap v2 Christoph Hellwig
2019-06-27 10:48 ` [PATCH 01/13] list.h: add list_pop and list_pop_entry helpers Christoph Hellwig
2019-06-27 20:48   ` Darrick J. Wong
2019-06-27 10:48 ` [PATCH 02/13] xfs: remove the unused xfs_count_page_state declaration Christoph Hellwig
2019-06-27 17:50   ` Darrick J. Wong
2019-06-27 10:48 ` [PATCH 03/13] xfs: fix a comment typo in xfs_submit_ioend Christoph Hellwig
2019-06-27 10:48 ` [PATCH 04/13] xfs: initialize iomap->flags in xfs_bmbt_to_iomap Christoph Hellwig
2019-06-27 20:44   ` Darrick J. Wong
2019-06-27 10:48 ` [PATCH 05/13] xfs: use a struct iomap in xfs_writepage_ctx Christoph Hellwig
2019-06-27 10:48 ` [PATCH 06/13] xfs: remove XFS_TRANS_NOFS Christoph Hellwig
2019-06-27 22:30   ` Darrick J. Wong
2019-06-28  5:37     ` Christoph Hellwig
2019-06-28 17:41       ` Darrick J. Wong
2019-06-27 10:48 ` [PATCH 07/13] xfs: allow merging ioends over append boundaries Christoph Hellwig
2019-06-27 18:23   ` Darrick J. Wong
2019-06-27 21:43     ` Luis Chamberlain
2019-06-28  2:52       ` Zorro Lang
2019-06-28  3:33         ` Darrick J. Wong
2019-06-28  5:51     ` Christoph Hellwig
2019-06-28 17:05       ` Darrick J. Wong
2019-06-27 10:48 ` [PATCH 08/13] xfs: simplify xfs_ioend_can_merge Christoph Hellwig
2019-06-27 10:48 ` [PATCH 09/13] xfs: refactor the ioend merging code Christoph Hellwig
2019-06-27 10:48 ` [PATCH 10/13] xfs: turn io_append_trans into an io_private void pointer Christoph Hellwig
2019-06-27 10:48 ` [PATCH 11/13] xfs: remove the fork fields in the writepage_ctx and ioend Christoph Hellwig
2019-06-27 10:48 ` [PATCH 12/13] iomap: move the xfs writeback code to iomap.c Christoph Hellwig
2019-06-27 10:48 ` [PATCH 13/13] iomap: add tracing for the address space operations Christoph Hellwig
2019-06-28  1:32 ` Darrick J. Wong [this message]
2019-06-28  5:42   ` lift the xfs writepage code into iomap v2 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=20190628013256.GE5179@magnolia \
    --to=darrick.wong@oracle.com \
    --cc=Damien.LeMoal@wdc.com \
    --cc=agruenba@redhat.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --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).