From: Christoph Hellwig <hch@lst.de>
To: Shiyang Ruan <ruansy.fnst@fujitsu.com>
Cc: djwong@kernel.org, hch@lst.de, 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 v8 6/7] xfs: support CoW in fsdax mode
Date: Thu, 2 Sep 2021 09:43:08 +0200 [thread overview]
Message-ID: <20210902074308.GE13867@lst.de> (raw)
In-Reply-To: <20210829122517.1648171-7-ruansy.fnst@fujitsu.com>
On Sun, Aug 29, 2021 at 08:25:16PM +0800, Shiyang Ruan wrote:
> In fsdax mode, WRITE and ZERO on a shared extent need CoW performed.
> After that, new allocated extents needs to be remapped to the file. Add
> an implementation of ->iomap_end() for dax write ops to do the remapping
> work.
Please split the new dax infrastructure from the XFS changes.
> static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, pfn_t *pfnp,
> - int *iomap_errp, const struct iomap_ops *ops)
> + int *iomap_errp, const struct iomap_ops *ops)
> {
> struct address_space *mapping = vmf->vma->vm_file->f_mapping;
> XA_STATE(xas, &mapping->i_pages, vmf->pgoff);
> @@ -1631,7 +1664,7 @@ static bool dax_fault_check_fallback(struct vm_fault *vmf, struct xa_state *xas,
> }
>
> static vm_fault_t dax_iomap_pmd_fault(struct vm_fault *vmf, pfn_t *pfnp,
> - const struct iomap_ops *ops)
> + const struct iomap_ops *ops)
These looks like unrelated whitespace changes.
> -static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> +loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> {
> const struct iomap *iomap = &iter->iomap;
> const struct iomap *srcmap = iomap_iter_srcmap(iter);
> @@ -918,6 +918,7 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>
> return written;
> }
> +EXPORT_SYMBOL_GPL(iomap_zero_iter);
I don't see why this would have to be exported.
> + unsigned flags,
> + struct iomap *iomap)
> +{
> + int error = 0;
> + struct xfs_inode *ip = XFS_I(inode);
> + bool cow = xfs_is_cow_inode(ip);
The cow variable is only used once, so I think we can drop it.
> + const struct iomap_iter *iter =
> + container_of(iomap, typeof(*iter), iomap);
Please comment this as it is a little unusual.
> +
> + if (cow) {
> + if (iter->processed <= 0)
> + xfs_reflink_cancel_cow_range(ip, pos, length, true);
> + else
> + error = xfs_reflink_end_cow(ip, pos, iter->processed);
> + }
> + return error ?: iter->processed;
The ->iomap_end convention is to return 0 or a negative error code.
Also i'd much prefer to just spell this out in a normal sequential way:
if (!xfs_is_cow_inode(ip))
return 0;
if (iter->processed <= 0) {
xfs_reflink_cancel_cow_range(ip, pos, length, true);
return 0;
}
return xfs_reflink_end_cow(ip, pos, iter->processed);
> +static inline int
> +xfs_iomap_zero_range(
> + struct xfs_inode *ip,
> + loff_t pos,
> + loff_t len,
> + bool *did_zero)
> +{
> + struct inode *inode = VFS_I(ip);
> +
> + return IS_DAX(inode)
> + ? dax_iomap_zero_range(inode, pos, len, did_zero,
> + &xfs_dax_write_iomap_ops)
> + : iomap_zero_range(inode, pos, len, did_zero,
> + &xfs_buffered_write_iomap_ops);
> +}
if (IS_DAX(inode))
return dax_iomap_zero_range(inode, pos, len, did_zero,
&xfs_dax_write_iomap_ops);
return iomap_zero_range(inode, pos, len, did_zero,
&xfs_buffered_write_iomap_ops);
> +static inline int
> +xfs_iomap_truncate_page(
> + struct xfs_inode *ip,
> + loff_t pos,
> + bool *did_zero)
> +{
> + struct inode *inode = VFS_I(ip);
> +
> + return IS_DAX(inode)
> + ? dax_iomap_truncate_page(inode, pos, did_zero,
> + &xfs_dax_write_iomap_ops)
> + : iomap_truncate_page(inode, pos, did_zero,
> + &xfs_buffered_write_iomap_ops);
> +}
Same here.
next prev parent reply other threads:[~2021-09-02 7:43 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-29 12:25 [PATCH v8 0/7] fsdax,xfs: Add reflink&dedupe support for fsdax Shiyang Ruan
2021-08-29 12:25 ` [PATCH v8 1/7] fsdax: Output address in dax_iomap_pfn() and rename it Shiyang Ruan
2021-08-29 12:25 ` [PATCH v8 2/7] fsdax: Introduce dax_iomap_cow_copy() Shiyang Ruan
2021-08-29 12:25 ` [PATCH v8 3/7] fsdax: Replace mmap entry in case of CoW Shiyang Ruan
2021-09-02 7:27 ` Christoph Hellwig
2021-08-29 12:25 ` [PATCH v8 4/7] fsdax: Add dax_iomap_cow_copy() for dax_iomap_zero Shiyang Ruan
2021-09-02 7:32 ` Christoph Hellwig
2021-08-29 12:25 ` [PATCH v8 5/7] fsdax: Dedup file range to use a compare function Shiyang Ruan
2021-09-02 7:34 ` Christoph Hellwig
2021-08-29 12:25 ` [PATCH v8 6/7] xfs: support CoW in fsdax mode Shiyang Ruan
2021-09-02 7:43 ` Christoph Hellwig [this message]
2021-09-02 15:33 ` Darrick J. Wong
2021-08-29 12:25 ` [PATCH v8 7/7] xfs: Add dax dedupe support Shiyang Ruan
2021-09-02 7:43 ` 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=20210902074308.GE13867@lst.de \
--to=hch@lst.de \
--cc=dan.j.williams@intel.com \
--cc=david@fromorbit.com \
--cc=djwong@kernel.org \
--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 \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.