From: "Darrick J. Wong" <firstname.lastname@example.org> To: Christoph Hellwig <email@example.com> Cc: Shiyang Ruan <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com 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
next prev parent 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --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).