From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 4/4] xfs: allocate direct I/O COW blocks in iomap_begin
Date: Mon, 6 Feb 2017 17:41:49 -0800 [thread overview]
Message-ID: <20170207014149.GE12378@birch.djwong.org> (raw)
In-Reply-To: <20170206074738.13978-5-hch@lst.de>
On Mon, Feb 06, 2017 at 08:47:38AM +0100, Christoph Hellwig wrote:
> Instead of preallocating all the required COW blocks in the high-level
> write code do it inside the iomap code, like we do for all other I/O.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Ok, got all the pieces together and they look ok to me.
I ran this through the dio reflink tests and they pass now.
I'm going to run this series (and all the other stuff I've collected for
4.11) overnight and if nothing screams then you can consider this series:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_file.c | 8 ---
> fs/xfs/xfs_iomap.c | 31 +++++------
> fs/xfs/xfs_reflink.c | 144 +++++++++++++++++++--------------------------------
> fs/xfs/xfs_reflink.h | 4 +-
> fs/xfs/xfs_trace.h | 2 -
> 5 files changed, 66 insertions(+), 123 deletions(-)
>
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index b1f74f1d0b5f..1ff32d34514f 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -561,14 +561,6 @@ xfs_file_dio_aio_write(
> }
>
> trace_xfs_file_direct_write(ip, count, iocb->ki_pos);
> -
> - /* If this is a block-aligned directio CoW, remap immediately. */
> - if (xfs_is_reflink_inode(ip) && !unaligned_io) {
> - ret = xfs_reflink_allocate_cow_range(ip, iocb->ki_pos, count);
> - if (ret)
> - goto out;
> - }
> -
> ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, xfs_dio_write_end_io);
> out:
> xfs_iunlock(ip, iolock);
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 9d811eb1a416..0978a5f0b50c 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -995,37 +995,31 @@ xfs_file_iomap_begin(
> offset_fsb = XFS_B_TO_FSBT(mp, offset);
> end_fsb = XFS_B_TO_FSB(mp, offset + length);
>
> - if (xfs_is_reflink_inode(ip) &&
> - (flags & IOMAP_WRITE) && (flags & IOMAP_DIRECT)) {
> - shared = xfs_reflink_find_cow_mapping(ip, offset, &imap);
> - if (shared) {
> - xfs_iunlock(ip, lockmode);
> - goto alloc_done;
> - }
> - ASSERT(!isnullstartblock(imap.br_startblock));
> - }
> -
> error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb, &imap,
> &nimaps, 0);
> if (error)
> goto out_unlock;
>
> - if ((flags & IOMAP_REPORT) ||
> - (xfs_is_reflink_inode(ip) &&
> - (flags & IOMAP_WRITE) && (flags & IOMAP_DIRECT))) {
> + if (flags & IOMAP_REPORT) {
> /* Trim the mapping to the nearest shared extent boundary. */
> error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
> &trimmed);
> if (error)
> goto out_unlock;
> -
> - ASSERT((flags & IOMAP_REPORT) || !shared);
> }
>
> if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && xfs_is_reflink_inode(ip)) {
> - error = xfs_reflink_reserve_cow(ip, &imap, &shared);
> - if (error)
> - goto out_unlock;
> + if (flags & IOMAP_DIRECT) {
> + /* may drop and re-acquire the ilock */
> + error = xfs_reflink_allocate_cow(ip, &imap, &shared,
> + &lockmode);
> + if (error)
> + goto out_unlock;
> + } else {
> + error = xfs_reflink_reserve_cow(ip, &imap, &shared);
> + if (error)
> + goto out_unlock;
> + }
>
> end_fsb = imap.br_startoff + imap.br_blockcount;
> length = XFS_FSB_TO_B(mp, end_fsb) - offset;
> @@ -1054,7 +1048,6 @@ xfs_file_iomap_begin(
> if (error)
> return error;
>
> -alloc_done:
> iomap->flags = IOMAP_F_NEW;
> trace_xfs_iomap_alloc(ip, offset, length, 0, &imap);
> } else {
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index e577f54beb2f..68d0fbdd0929 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -383,74 +383,75 @@ xfs_reflink_convert_cow(
> }
>
> /* Allocate all CoW reservations covering a range of blocks in a file. */
> -static int
> -__xfs_reflink_allocate_cow(
> +int
> +xfs_reflink_allocate_cow(
> struct xfs_inode *ip,
> - xfs_fileoff_t *offset_fsb,
> - xfs_fileoff_t end_fsb)
> + struct xfs_bmbt_irec *imap,
> + bool *shared,
> + uint *lockmode)
> {
> struct xfs_mount *mp = ip->i_mount;
> - struct xfs_bmbt_irec imap, got;
> + xfs_fileoff_t offset_fsb = imap->br_startoff;
> + xfs_filblks_t count_fsb = imap->br_blockcount;
> + struct xfs_bmbt_irec got;
> struct xfs_defer_ops dfops;
> - struct xfs_trans *tp;
> + struct xfs_trans *tp = NULL;
> xfs_fsblock_t first_block;
> - int nimaps, error, lockmode;
> - bool shared, trimmed;
> + int nimaps, error = 0;
> + bool trimmed;
> xfs_filblks_t resaligned;
> - xfs_extlen_t resblks;
> + xfs_extlen_t resblks = 0;
> xfs_extnum_t idx;
>
> - resaligned = xfs_aligned_fsb_count(*offset_fsb, end_fsb - *offset_fsb,
> - xfs_get_cowextsz_hint(ip));
> - resblks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
> -
> - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
> - if (error)
> - return error;
> -
> - lockmode = XFS_ILOCK_EXCL;
> - xfs_ilock(ip, lockmode);
> +retry:
> + ASSERT(xfs_is_reflink_inode(ip));
> + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
>
> /*
> * Even if the extent is not shared we might have a preallocation for
> * it in the COW fork. If so use it.
> */
> - if (xfs_iext_lookup_extent(ip, ip->i_cowfp, *offset_fsb, &idx, &got) &&
> - got.br_startoff <= *offset_fsb) {
> + if (xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, &idx, &got) &&
> + got.br_startoff <= offset_fsb) {
> + *shared = true;
> +
> /* If we have a real allocation in the COW fork we're done. */
> if (!isnullstartblock(got.br_startblock)) {
> - xfs_trim_extent(&got, *offset_fsb,
> - end_fsb - *offset_fsb);
> - *offset_fsb = got.br_startoff + got.br_blockcount;
> - goto out_trans_cancel;
> + xfs_trim_extent(&got, offset_fsb, count_fsb);
> + *imap = got;
> + goto convert;
> }
> +
> + xfs_trim_extent(imap, got.br_startoff, got.br_blockcount);
> } else {
> - nimaps = 1;
> - error = xfs_bmapi_read(ip, *offset_fsb, end_fsb - *offset_fsb,
> - &imap, &nimaps, 0);
> - if (error)
> - goto out_trans_cancel;
> - ASSERT(nimaps == 1);
> + error = xfs_reflink_trim_around_shared(ip, imap, shared, &trimmed);
> + if (error || !*shared)
> + goto out;
> + }
>
> - /* Trim the mapping to the nearest shared extent boundary. */
> - error = xfs_reflink_trim_around_shared(ip, &imap, &shared,
> - &trimmed);
> - if (error)
> - goto out_trans_cancel;
> + if (!tp) {
> + resaligned = xfs_aligned_fsb_count(imap->br_startoff,
> + imap->br_blockcount, xfs_get_cowextsz_hint(ip));
> + resblks = XFS_DIOSTRAT_SPACE_RES(mp, resaligned);
>
> - if (!shared) {
> - *offset_fsb = imap.br_startoff + imap.br_blockcount;
> - goto out_trans_cancel;
> - }
> + xfs_iunlock(ip, *lockmode);
> + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0, 0, &tp);
> + *lockmode = XFS_ILOCK_EXCL;
> + xfs_ilock(ip, *lockmode);
> +
> + if (error)
> + return error;
>
> - *offset_fsb = imap.br_startoff;
> - end_fsb = imap.br_startoff + imap.br_blockcount;
> + error = xfs_qm_dqattach_locked(ip, 0);
> + if (error)
> + goto out;
> + goto retry;
> }
>
> error = xfs_trans_reserve_quota_nblks(tp, ip, resblks, 0,
> XFS_QMOPT_RES_REGBLKS);
> if (error)
> - goto out_trans_cancel;
> + goto out;
>
> xfs_trans_ijoin(tp, ip, 0);
>
> @@ -458,9 +459,9 @@ __xfs_reflink_allocate_cow(
> nimaps = 1;
>
> /* Allocate the entire reservation as unwritten blocks. */
> - error = xfs_bmapi_write(tp, ip, *offset_fsb, end_fsb - *offset_fsb,
> + error = xfs_bmapi_write(tp, ip, imap->br_startoff, imap->br_blockcount,
> XFS_BMAPI_COWFORK | XFS_BMAPI_PREALLOC, &first_block,
> - resblks, &imap, &nimaps, &dfops);
> + resblks, imap, &nimaps, &dfops);
> if (error)
> goto out_bmap_cancel;
>
> @@ -471,58 +472,17 @@ __xfs_reflink_allocate_cow(
>
> error = xfs_trans_commit(tp);
> if (error)
> - goto out_unlock;
> -
> - *offset_fsb = imap.br_startoff + imap.br_blockcount;
> -
> -out_unlock:
> - xfs_iunlock(ip, lockmode);
> - return error;
> -
> + return error;
> +convert:
> + return xfs_reflink_convert_cow_extent(ip, imap, offset_fsb, count_fsb,
> + &dfops);
> out_bmap_cancel:
> xfs_defer_cancel(&dfops);
> xfs_trans_unreserve_quota_nblks(tp, ip, (long)resblks, 0,
> XFS_QMOPT_RES_REGBLKS);
> -out_trans_cancel:
> - xfs_trans_cancel(tp);
> - goto out_unlock;
> -}
> -
> -/* Allocate all CoW reservations covering a part of a file. */
> -int
> -xfs_reflink_allocate_cow_range(
> - struct xfs_inode *ip,
> - xfs_off_t offset,
> - xfs_off_t count)
> -{
> - struct xfs_mount *mp = ip->i_mount;
> - xfs_fileoff_t offset_fsb = XFS_B_TO_FSBT(mp, offset);
> - xfs_fileoff_t end_fsb = XFS_B_TO_FSB(mp, offset + count);
> - int error;
> -
> - ASSERT(xfs_is_reflink_inode(ip));
> -
> - trace_xfs_reflink_allocate_cow_range(ip, offset, count);
> -
> - /*
> - * Make sure that the dquots are there.
> - */
> - error = xfs_qm_dqattach(ip, 0);
> - if (error)
> - return error;
> -
> - while (offset_fsb < end_fsb) {
> - error = __xfs_reflink_allocate_cow(ip, &offset_fsb, end_fsb);
> - if (error) {
> - trace_xfs_reflink_allocate_cow_range_error(ip, error,
> - _RET_IP_);
> - goto out;
> - }
> - }
> -
> - /* Convert the CoW extents to regular. */
> - error = xfs_reflink_convert_cow(ip, offset, count);
> out:
> + if (tp)
> + xfs_trans_cancel(tp);
> return error;
> }
>
> diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h
> index 1583c4727cb1..33ac9b8db683 100644
> --- a/fs/xfs/xfs_reflink.h
> +++ b/fs/xfs/xfs_reflink.h
> @@ -28,8 +28,8 @@ extern int xfs_reflink_trim_around_shared(struct xfs_inode *ip,
>
> extern int xfs_reflink_reserve_cow(struct xfs_inode *ip,
> struct xfs_bmbt_irec *imap, bool *shared);
> -extern int xfs_reflink_allocate_cow_range(struct xfs_inode *ip,
> - xfs_off_t offset, xfs_off_t count);
> +extern int xfs_reflink_allocate_cow(struct xfs_inode *ip,
> + struct xfs_bmbt_irec *imap, bool *shared, uint *lockmode);
> extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset,
> xfs_off_t count);
> extern bool xfs_reflink_find_cow_mapping(struct xfs_inode *ip, xfs_off_t offset,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 375c5e030e5b..32bad7bff840 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -3248,7 +3248,6 @@ DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_enospc);
> DEFINE_INODE_IREC_EVENT(xfs_reflink_convert_cow);
>
> DEFINE_RW_EVENT(xfs_reflink_reserve_cow);
> -DEFINE_RW_EVENT(xfs_reflink_allocate_cow_range);
>
> DEFINE_SIMPLE_IO_EVENT(xfs_reflink_bounce_dio_write);
> DEFINE_IOMAP_EVENT(xfs_reflink_find_cow_mapping);
> @@ -3258,7 +3257,6 @@ DEFINE_SIMPLE_IO_EVENT(xfs_reflink_cancel_cow_range);
> DEFINE_SIMPLE_IO_EVENT(xfs_reflink_end_cow);
> DEFINE_INODE_IREC_EVENT(xfs_reflink_cow_remap);
>
> -DEFINE_INODE_ERROR_EVENT(xfs_reflink_allocate_cow_range_error);
> DEFINE_INODE_ERROR_EVENT(xfs_reflink_cancel_cow_range_error);
> DEFINE_INODE_ERROR_EVENT(xfs_reflink_end_cow_error);
>
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-02-07 1:41 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-06 7:47 reflink direct I/O improvements V3 Christoph Hellwig
2017-02-06 7:47 ` [PATCH 1/4] xfs: introduce xfs_aligned_fsb_count Christoph Hellwig
2017-02-06 7:47 ` [PATCH 2/4] xfs: return the converted extent in __xfs_reflink_convert_cow Christoph Hellwig
2017-02-06 7:47 ` [PATCH 3/4] xfs: go straight to real allocations for direct I/O COW writes Christoph Hellwig
2017-02-07 1:12 ` Darrick J. Wong
2017-02-06 7:47 ` [PATCH 4/4] xfs: allocate direct I/O COW blocks in iomap_begin Christoph Hellwig
2017-02-07 1:41 ` Darrick J. Wong [this message]
2017-02-09 7:21 ` Christoph Hellwig
2017-02-09 7:53 ` Darrick J. Wong
2017-02-09 7:58 ` Christoph Hellwig
2017-02-09 8:00 ` Christoph Hellwig
2017-02-09 17:22 ` Christoph Hellwig
2017-02-06 18:41 ` reflink direct I/O improvements V3 Darrick J. Wong
2017-02-06 20:55 ` Christoph Hellwig
-- strict thread matches above, loose matches on Subject: below --
2017-02-01 21:42 reflink direct I/O improvements V2 Christoph Hellwig
2017-02-01 21:42 ` [PATCH 4/4] xfs: allocate direct I/O COW blocks in iomap_begin Christoph Hellwig
2017-02-03 0:55 ` Darrick J. Wong
2017-02-03 10:53 ` Christoph Hellwig
2017-02-05 20:13 ` 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=20170207014149.GE12378@birch.djwong.org \
--to=darrick.wong@oracle.com \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.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 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).