linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Andreas Gruenbacher <agruenba@redhat.com>
Cc: cluster-devel@redhat.com, Christoph Hellwig <hch@lst.de>,
	linux-fsdevel@vger.kernel.org, Dave Chinner <dchinner@redhat.com>
Subject: Re: [PATCH v4 06/11] iomap: Add write_{begin,end} iomap operations
Date: Tue, 15 May 2018 11:11:47 +1000	[thread overview]
Message-ID: <20180515011147.GF23861@dastard> (raw)
In-Reply-To: <20180514153624.29598-7-agruenba@redhat.com>

On Mon, May 14, 2018 at 05:36:19PM +0200, Andreas Gruenbacher wrote:
> Add write_begin and write_end operations to struct iomap_ops to provide
> a way of overriding the default behavior of iomap_write_begin and
> iomap_write_end.  This is needed for implementing data journaling: in
> the data journaling case, pages are written into the journal before
> being written back to their proper on-disk locations.

I'm not sure adding a per-page filesystem callout abstraction is
what we want in the iomap code.  The commit message does not explain
why gfs2 needs per-page callouts, nor why the per-page work cannot
be done/avoided by running code in the ->iomap_begin callout
(e.g. by unstuffing the inode so nothing needs to be done per-page).

My main concern is that the callouts to the filesystem in iomap are
- by design - per IO, not per page. The whole point of using iomap
was to get away from doing per-page filesystem operations on
multi-page IOs - it's highly inefficient when we only need to call
into the filesystem on a per-extent basis, and we simplified the
code a *lot* by avoiding such behaviours.

And to that point: this change has serious conflicts with the next
step for the iomap infrastructure: Christoph's recent
"remove bufferheads from iomap" series. Apart from the obvious code
conflicts, there's a fairly major architectural/functional conflict.

https://marc.info/?l=linux-fsdevel&m=152585212000411&w=2

That is, Christoph's patchset pushes us further away from
filesystems doing their own per-page processing of page state. It
converts the iomap IO path to store it's own per-page state tracking
information in page->private, greatly reducing memory overhead (16
bytes on 4k page instead of 104 bytes per bufferhead) and
streamlining the code.  This, however, essentially makes the
buffered IO path through the iomap infrastructure incompatible with
existing bufferhead based filesystems.

Depsite this, we can still provide limited support for buffered
writes on bufferhead based filesystems through the iomap
infrastructure, but I only see that as a mechanism for filesystems
moving away from dependencies on bufferheads. It would require a
different refactoring of the iomap code - I'd much prefer to keep
bufferhead dependent IO paths completely separate (e.g. via a new
actor function) to minimise impact and dependencies on the internal
bufferhead free iomap IO path....

Let's see what Christoph thinks first, though....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2018-05-15  1:11 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-14 15:36 [PATCH v4 00/11] gfs2 iomap write support Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 01/11] gfs2: Update find_metapath comment Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 02/11] gfs2: hole_size improvement Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 03/11] gfs2: gfs2_stuffed_write_end cleanup Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 04/11] gfs2: Remove ordered write mode handling from gfs2_trans_add_data Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 05/11] gfs2: Iomap cleanups and improvements Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 06/11] iomap: Add write_{begin,end} iomap operations Andreas Gruenbacher
2018-05-15  1:11   ` Dave Chinner [this message]
2018-05-15  7:22     ` Christoph Hellwig
2018-05-15  8:16       ` Andreas Gruenbacher
2018-05-18 16:04         ` Christoph Hellwig
2018-05-25 17:58           ` Andreas Grünbacher
2018-05-28 13:02             ` Christoph Hellwig
2018-05-14 15:36 ` [PATCH v4 07/11] gfs2: iomap buffered write support Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 08/11] gfs2: gfs2_extent_length cleanup Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 09/11] gfs2: iomap direct I/O support Andreas Gruenbacher
2018-05-15  7:31   ` Christoph Hellwig
2018-05-16 20:36     ` Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 10/11] gfs2: Remove gfs2_write_{begin,end} Andreas Gruenbacher
2018-05-14 15:36 ` [PATCH v4 11/11] iomap: Complete partial direct I/O writes synchronously Andreas Gruenbacher
2018-05-15  7:24   ` Christoph Hellwig
2018-05-16 20:27     ` Andreas Gruenbacher
2018-05-18 15:56       ` Christoph Hellwig
2018-05-18 18:35         ` Andreas Grünbacher

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=20180515011147.GF23861@dastard \
    --to=david@fromorbit.com \
    --cc=agruenba@redhat.com \
    --cc=cluster-devel@redhat.com \
    --cc=dchinner@redhat.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --subject='Re: [PATCH v4 06/11] iomap: Add write_{begin,end} iomap operations' \
    /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

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