On Sat, Sep 24, 2016 at 08:19:20AM -0700, Christoph Hellwig wrote:
> Instead enhance xfs_reflink_find_cow_mapping to properly deal with the
> fact that we might not have a COW mapping at the offset, and just
> return false in this case.
>
> This avoids code duplication, makes xfs_reflink_find_cow_mapping more
> robust, and last but not least halves the number of extent map lookups
> needed in COW writeback operations.
Hooray!
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_aops.c | 28 ++++++++++++++++------------
> fs/xfs/xfs_reflink.c | 42 +++++++++++-------------------------------
> fs/xfs/xfs_reflink.h | 3 +--
> 3 files changed, 28 insertions(+), 45 deletions(-)
>
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index d8ce18b..1933803 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -379,10 +379,13 @@ xfs_map_blocks(
> end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + count);
> offset_fsb = XFS_B_TO_FSBT(mp, offset);
>
> - if (type == XFS_IO_COW)
> - error = xfs_reflink_find_cow_mapping(ip, offset, imap,
> - &need_alloc);
> - else {
> + if (type == XFS_IO_COW) {
> + if (!xfs_reflink_find_cow_mapping(ip, offset, imap,
> + &need_alloc)) {
> + WARN_ON_ONCE(1);
> + return -EIO;
> + }
I squinted at this for a second but then realized it goes away in the
next patch anyway.
> + } else {
> error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
> imap, &nimaps, bmapi_flags);
> /*
> @@ -712,13 +715,14 @@ xfs_is_cow_io(
> struct xfs_inode *ip,
> xfs_off_t offset)
> {
> - bool is_cow;
> + struct xfs_bmbt_irec imap;
> + bool is_cow, need_alloc;
>
> if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb))
> return false;
>
> xfs_ilock(ip, XFS_ILOCK_SHARED);
> - is_cow = xfs_reflink_is_cow_pending(ip, offset);
> + is_cow = xfs_reflink_find_cow_mapping(ip, offset, &imap, &need_alloc);
> xfs_iunlock(ip, XFS_ILOCK_SHARED);
>
> return is_cow;
> @@ -1316,12 +1320,12 @@ __xfs_get_blocks(
> end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)offset + size);
> offset_fsb = XFS_B_TO_FSBT(mp, offset);
>
> - if (create && direct)
> - is_cow = xfs_reflink_is_cow_pending(ip, offset);
> - if (is_cow)
> - error = xfs_reflink_find_cow_mapping(ip, offset, &imap,
> - &need_alloc);
> - else {
> + if (create && direct) {
> + is_cow = xfs_reflink_find_cow_mapping(ip, offset, &imap,
> + &need_alloc);
> + }
> +
> + if (!is_cow) {
> error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
> &imap, &nimaps, XFS_BMAPI_ENTIRE);
> /*
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 6f87b1e..a1ba7f5 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -429,26 +429,31 @@ advloop:
> }
>
> /*
> - * Determine if there's a CoW reservation at a byte offset of an inode.
> + * Find the CoW reservation (and whether or not it needs block allocation)
> + * for a given byte offset of a file.
> */
> bool
> -xfs_reflink_is_cow_pending(
> +xfs_reflink_find_cow_mapping(
> struct xfs_inode *ip,
> - xfs_off_t offset)
> + xfs_off_t offset,
> + struct xfs_bmbt_irec *imap,
> + bool *need_alloc)
> {
> + struct xfs_bmbt_irec irec;
> struct xfs_ifork *ifp;
> struct xfs_bmbt_rec_host *gotp;
> - struct xfs_bmbt_irec irec;
> xfs_fileoff_t bno;
> xfs_extnum_t idx;
>
> + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
> +
> if (!xfs_is_reflink_inode(ip))
> return false;
>
> + /* Find the extent in the CoW fork. */
> ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> bno = XFS_B_TO_FSBT(ip->i_mount, offset);
> gotp = xfs_iext_bno_to_ext(ifp, bno, &idx);
> -
> if (!gotp)
> return false;
>
> @@ -456,31 +461,6 @@ xfs_reflink_is_cow_pending(
> if (bno >= irec.br_startoff + irec.br_blockcount ||
> bno < irec.br_startoff)
> return false;
> - return true;
> -}
> -
> -/*
> - * Find the CoW reservation (and whether or not it needs block allocation)
> - * for a given byte offset of a file.
> - */
> -int
> -xfs_reflink_find_cow_mapping(
> - struct xfs_inode *ip,
> - xfs_off_t offset,
> - struct xfs_bmbt_irec *imap,
> - bool *need_alloc)
> -{
> - struct xfs_bmbt_irec irec;
> - struct xfs_ifork *ifp;
> - struct xfs_bmbt_rec_host *gotp;
> - xfs_fileoff_t bno;
> - xfs_extnum_t idx;
> -
> - /* Find the extent in the CoW fork. */
> - ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> - bno = XFS_B_TO_FSBT(ip->i_mount, offset);
> - gotp = xfs_iext_bno_to_ext(ifp, bno, &idx);
> - xfs_bmbt_get_all(gotp, &irec);
>
> trace_xfs_reflink_find_cow_mapping(ip, offset, 1, XFS_IO_OVERWRITE,
> &irec);
> @@ -489,7 +469,7 @@ xfs_reflink_find_cow_mapping(
> *imap = irec;
> *need_alloc = !!(isnullstartblock(irec.br_startblock));
>
> - return 0;
> + return true;
Otherwise looks resonable enough, so
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> }
>
> /*
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index 398e726..6519f19 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -30,8 +30,7 @@ extern int xfs_reflink_reserve_cow_range(struct xfs_inode *ip,
> xfs_fileoff_t offset_fsb, xfs_fileoff_t end_fsb);
> extern int xfs_reflink_allocate_cow_range(struct xfs_inode *ip, xfs_off_t pos,
> xfs_off_t len);
> -extern bool xfs_reflink_is_cow_pending(struct xfs_inode *ip, xfs_off_t offset);
> -extern int xfs_reflink_find_cow_mapping(struct xfs_inode *ip, xfs_off_t offset,
> +extern bool xfs_reflink_find_cow_mapping(struct xfs_inode *ip, xfs_off_t offset,
> struct xfs_bmbt_irec *imap, bool *need_alloc);
> extern int xfs_reflink_trim_irec_to_next_cow(struct xfs_inode *ip,
> xfs_fileoff_t offset_fsb, struct xfs_bmbt_irec *imap);
> --
> 2.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
