nvdimm.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: "Darrick J. Wong" <djwong@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Shiyang Ruan <ruansy.fnst@fujitsu.com>,
	linux-xfs@vger.kernel.org, dan.j.williams@intel.com,
	david@fromorbit.com, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, nvdimm@lists.linux.dev,
	rgoldwyn@suse.de, viro@zeniv.linux.org.uk, willy@infradead.org
Subject: Re: [PATCH v9 7/8] xfs: support CoW in fsdax mode
Date: Fri, 17 Sep 2021 08:33:04 -0700	[thread overview]
Message-ID: <20210917153304.GB10250@magnolia> (raw)
In-Reply-To: <20210916063251.GE13306@lst.de>

On Thu, Sep 16, 2021 at 08:32:51AM +0200, Christoph Hellwig wrote:
> On Wed, Sep 15, 2021 at 05:22:27PM -0700, Darrick J. Wong wrote:
> > >  		xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
> > >  		ret = dax_iomap_fault(vmf, pe_size, &pfn, NULL,
> > >  				(write_fault && !vmf->cow_page) ?
> > > -				 &xfs_direct_write_iomap_ops :
> > > -				 &xfs_read_iomap_ops);
> > > +					&xfs_dax_write_iomap_ops :
> > > +					&xfs_read_iomap_ops);
> > 
> > Hmm... I wonder if this should get hoisted to a "xfs_dax_iomap_fault"
> > wrapper like you did for xfs_iomap_zero_range?
> 
> This has just a single users, so the classic argument won't apply.  That
> being said __xfs_filemap_fault is a complete mess to due the calling
> conventions of the various VFS methods multiplexed into it.  So yes,
> splitting out a xfs_dax_iomap_fault to wrap the above plus the
> dax_finish_sync_fault call might not actually be a bad idea nevertheless.

Agree.

> > > +	struct xfs_inode	*ip = XFS_I(inode);
> > > +	/*
> > > +	 * Usually we use @written to indicate whether the operation was
> > > +	 * successful.  But it is always positive or zero.  The CoW needs the
> > > +	 * actual error code from actor().  So, get it from
> > > +	 * iomap_iter->processed.
> > 
> > Hm.  All six arguments are derived from the struct iomap_iter, so maybe
> > it makes more sense to pass that in?  I'll poke around with this more
> > tomorrow.
> 
> I'd argue against just changing the calling conventions for ->iomap_end
> now.  The original iter patches from willy allowed passing a single
> next callback combinging iomap_begin and iomap_end in a way that with
> a little magic we can avoid the indirect calls entirely.  I think we'll
> need to experiment with that that a bit and see if is worth the effort
> first.  I plan to do that but I might not get to it immediate.  If some
> else wants to take over I'm fine with that.

Ah, I forgot that.  Yay Etch-a-Sketch brain. <shake> -ENODATA ;)

> > >  static int
> > >  xfs_buffered_write_iomap_begin(
> > 
> > Also, we have an related request to drop the EXPERIMENTAL tag for
> > non-DAX reflink.  Whichever patch enables dax+reflink for xfs needs to
> > make it clear that reflink + any possibility of DAX emits an
> > EXPERIMENTAL warning.
> 
> More importantly before we can merge this series we also need the VM
> level support for reflink-aware reverse mapping.  So while this series
> here is no in a good enough shape I don't see how we could merge it
> without that other series as we'd have to disallow mmap for reflink+dax
> files otherwise.

I've forgotten why we need mm level reverse mapping again?  The pmem
poison stuff can use ->media_failure (or whatever it was called,
memory_failure?) to find all the owners and notify them.  Was there
some other accounting reason that fell out of my brain?

I'm more afraid of 'sharing pages between files needs mm support'
sparking another multi-year folioesque fight with the mm people.

--D

  reply	other threads:[~2021-09-17 15:33 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-15 10:44 [PATCH v9 0/8] fsdax,xfs: Add reflink&dedupe support for fsdax Shiyang Ruan
2021-09-15 10:44 ` [PATCH v9 1/8] fsdax: Output address in dax_iomap_pfn() and rename it Shiyang Ruan
2021-09-16  0:09   ` Darrick J. Wong
2021-09-16  1:36     ` Shiyang Ruan
2021-09-15 10:44 ` [PATCH v9 2/8] fsdax: Introduce dax_iomap_cow_copy() Shiyang Ruan
2021-09-15 10:44 ` [PATCH v9 3/8] fsdax: Replace mmap entry in case of CoW Shiyang Ruan
2021-09-16 14:51   ` kernel test robot
2021-09-15 10:44 ` [PATCH v9 4/8] fsdax: Convert dax_iomap_zero to iter model Shiyang Ruan
2021-09-16  0:11   ` Darrick J. Wong
2021-09-16  6:15   ` Christoph Hellwig
2021-09-15 10:44 ` [PATCH v9 5/8] fsdax: Add dax_iomap_cow_copy() for dax_iomap_zero Shiyang Ruan
2021-09-16  6:16   ` Christoph Hellwig
2021-09-16  8:49     ` Shiyang Ruan
2021-09-16 22:55       ` Darrick J. Wong
2021-09-15 10:44 ` [PATCH v9 6/8] fsdax: Dedup file range to use a compare function Shiyang Ruan
2021-09-16  6:17   ` Christoph Hellwig
2021-09-15 10:45 ` [PATCH v9 7/8] xfs: support CoW in fsdax mode Shiyang Ruan
2021-09-16  0:22   ` Darrick J. Wong
2021-09-16  6:32     ` Christoph Hellwig
2021-09-17 15:33       ` Darrick J. Wong [this message]
2021-09-21  8:14         ` Christoph Hellwig
2021-09-16  6:23   ` Christoph Hellwig
2021-09-16  8:51     ` Shiyang Ruan
2021-09-15 10:45 ` [PATCH v9 8/8] xfs: Add dax dedupe support Shiyang Ruan
2021-09-16  0:30   ` Darrick J. Wong
2021-09-16  4:01     ` Shiyang Ruan
2021-09-16  4:18       ` Darrick J. Wong

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=20210917153304.GB10250@magnolia \
    --to=djwong@kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=david@fromorbit.com \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=nvdimm@lists.linux.dev \
    --cc=rgoldwyn@suse.de \
    --cc=ruansy.fnst@fujitsu.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH v9 7/8] xfs: support CoW in fsdax mode' \
    /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).