On Mon, Sep 17, 2018 at 10:53:54PM +0200, Christoph Hellwig wrote:
> Introduce a new xfs_delalloc_iomap_ops instance that is passed to
> iomap_apply when we are doing delayed allocations. This keeps the code
> more neatly separated for future work.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_bmap_util.c | 3 +-
> fs/xfs/xfs_file.c | 6 +-
> fs/xfs/xfs_iomap.c | 177 ++++++++++++++++++++---------------------
> fs/xfs/xfs_iomap.h | 1 +
> fs/xfs/xfs_iops.c | 4 +-
> fs/xfs/xfs_reflink.c | 2 +-
> 6 files changed, 95 insertions(+), 98 deletions(-)
>
...
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 9079196bbc35..1bfc40ce581a 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -512,8 +512,16 @@ xfs_iomap_prealloc_size(
> return alloc_blocks;
> }
>
> +static int xfs_file_iomap_begin(struct inode *inode, loff_t offset,
> + loff_t length, unsigned flags, struct iomap *iomap);
> +
> +/*
> + * Start writing to a file, allocating blocks using delayed allocations if
> + * needed. Note that in case of doing COW overwrites the iomap needs to return
> + * the address of the existing data.
> + */
> static int
> -xfs_file_iomap_begin_delay(
> +xfs_delalloc_iomap_begin(
Ok, but wouldn't something like xfs_buffered_iomap_begin/end() be a
better name? Technically a real extent may already exist in this
codepath, so I find the delalloc name kind of confusing.
> struct inode *inode,
> loff_t offset,
> loff_t count,
...
> @@ -658,6 +671,73 @@ xfs_file_iomap_begin_delay(
> return error;
> }
>
> +static int
> +xfs_delalloc_iomap_end(
> + struct inode *inode,
> + loff_t offset,
> + loff_t length,
> + ssize_t written,
> + unsigned flags,
> + struct iomap *iomap)
> +{
> + struct xfs_inode *ip = XFS_I(inode);
> + struct xfs_mount *mp = ip->i_mount;
> + xfs_fileoff_t start_fsb;
> + xfs_fileoff_t end_fsb;
> + int error = 0;
> +
> + if (iomap->type != IOMAP_DELALLOC)
> + return 0;
Any reason we don't continue to filter out !IOMAP_WRITE as well?
Brian
> +
> + /*
> + * Behave as if the write failed if drop writes is enabled. Set the NEW
> + * flag to force delalloc cleanup.
> + */
> + if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_DROP_WRITES)) {
> + iomap->flags |= IOMAP_F_NEW;
> + written = 0;
> + }
> +
> + /*
> + * start_fsb refers to the first unused block after a short write. If
> + * nothing was written, round offset down to point at the first block in
> + * the range.
> + */
> + if (unlikely(!written))
> + start_fsb = XFS_B_TO_FSBT(mp, offset);
> + else
> + start_fsb = XFS_B_TO_FSB(mp, offset + written);
> + end_fsb = XFS_B_TO_FSB(mp, offset + length);
> +
> + /*
> + * Trim delalloc blocks if they were allocated by this write and we
> + * didn't manage to write the whole range.
> + *
> + * We don't need to care about racing delalloc as we hold i_mutex
> + * across the reserve/allocate/unreserve calls. If there are delalloc
> + * blocks in the range, they are ours.
> + */
> + if ((iomap->flags & IOMAP_F_NEW) && start_fsb < end_fsb) {
> + truncate_pagecache_range(VFS_I(ip), XFS_FSB_TO_B(mp, start_fsb),
> + XFS_FSB_TO_B(mp, end_fsb) - 1);
> +
> + error = xfs_bmap_punch_delalloc_range(ip, start_fsb,
> + end_fsb - start_fsb);
> + if (error && !XFS_FORCED_SHUTDOWN(mp)) {
> + xfs_alert(mp, "%s: unable to clean up ino %lld",
> + __func__, ip->i_ino);
> + return error;
> + }
> + }
> +
> + return 0;
> +}
> +
> +const struct iomap_ops xfs_delalloc_iomap_ops = {
> + .iomap_begin = xfs_delalloc_iomap_begin,
> + .iomap_end = xfs_delalloc_iomap_end,
> +};
> +
> /*
> * Pass in a delayed allocate extent, convert it to real extents;
> * return to the caller the extent we create which maps on top of
> @@ -1028,16 +1108,13 @@ xfs_file_iomap_begin(
> bool shared = false;
> unsigned lockmode;
>
> + ASSERT(!(flags & (IOMAP_WRITE | IOMAP_ZERO)) ||
> + (flags & IOMAP_DIRECT) || IS_DAX(inode));
> + ASSERT(!(flags & IOMAP_ZERO) || XFS_IS_REALTIME_INODE(ip));
> +
> if (XFS_FORCED_SHUTDOWN(mp))
> return -EIO;
>
> - if ((flags & (IOMAP_WRITE | IOMAP_ZERO)) && !(flags & IOMAP_DIRECT) &&
> - !IS_DAX(inode) && !XFS_IS_REALTIME_INODE(ip)) {
> - /* Reserve delalloc blocks for regular writeback. */
> - return xfs_file_iomap_begin_delay(inode, offset, length, flags,
> - iomap);
> - }
> -
> /*
> * Lock the inode in the manner required for the specified operation and
> * check for as many conditions that would result in blocking as
> @@ -1070,15 +1147,6 @@ xfs_file_iomap_begin(
> if (!(flags & (IOMAP_WRITE | IOMAP_ZERO)))
> goto out_found;
>
> - /*
> - * Don't need to allocate over holes when doing zeroing operations,
> - * unless we need to COW and have an existing extent.
> - */
> - if ((flags & IOMAP_ZERO) &&
> - (!xfs_is_reflink_inode(ip) ||
> - !needs_cow_for_zeroing(&imap, nimaps)))
> - goto out_found;
> -
> /*
> * Break shared extents if necessary. Checks for non-blocking IO have
> * been done up front, so we don't need to do them here.
> @@ -1148,81 +1216,8 @@ xfs_file_iomap_begin(
> return error;
> }
>
> -static int
> -xfs_file_iomap_end_delalloc(
> - struct xfs_inode *ip,
> - loff_t offset,
> - loff_t length,
> - ssize_t written,
> - struct iomap *iomap)
> -{
> - struct xfs_mount *mp = ip->i_mount;
> - xfs_fileoff_t start_fsb;
> - xfs_fileoff_t end_fsb;
> - int error = 0;
> -
> - /*
> - * Behave as if the write failed if drop writes is enabled. Set the NEW
> - * flag to force delalloc cleanup.
> - */
> - if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_DROP_WRITES)) {
> - iomap->flags |= IOMAP_F_NEW;
> - written = 0;
> - }
> -
> - /*
> - * start_fsb refers to the first unused block after a short write. If
> - * nothing was written, round offset down to point at the first block in
> - * the range.
> - */
> - if (unlikely(!written))
> - start_fsb = XFS_B_TO_FSBT(mp, offset);
> - else
> - start_fsb = XFS_B_TO_FSB(mp, offset + written);
> - end_fsb = XFS_B_TO_FSB(mp, offset + length);
> -
> - /*
> - * Trim delalloc blocks if they were allocated by this write and we
> - * didn't manage to write the whole range.
> - *
> - * We don't need to care about racing delalloc as we hold i_mutex
> - * across the reserve/allocate/unreserve calls. If there are delalloc
> - * blocks in the range, they are ours.
> - */
> - if ((iomap->flags & IOMAP_F_NEW) && start_fsb < end_fsb) {
> - truncate_pagecache_range(VFS_I(ip), XFS_FSB_TO_B(mp, start_fsb),
> - XFS_FSB_TO_B(mp, end_fsb) - 1);
> -
> - error = xfs_bmap_punch_delalloc_range(ip, start_fsb,
> - end_fsb - start_fsb);
> - if (error && !XFS_FORCED_SHUTDOWN(mp)) {
> - xfs_alert(mp, "%s: unable to clean up ino %lld",
> - __func__, ip->i_ino);
> - return error;
> - }
> - }
> -
> - return 0;
> -}
> -
> -static int
> -xfs_file_iomap_end(
> - struct inode *inode,
> - loff_t offset,
> - loff_t length,
> - ssize_t written,
> - unsigned flags,
> - struct iomap *iomap)
> -{
> - if ((flags & IOMAP_WRITE) && iomap->type == IOMAP_DELALLOC)
> - return xfs_file_iomap_end_delalloc(XFS_I(inode), offset,
> - length, written, iomap);
> - return 0;
> -}
> -
> const struct iomap_ops xfs_iomap_ops = {
> .iomap_begin = xfs_file_iomap_begin,
> - .iomap_end = xfs_file_iomap_end,
> };
>
> static int
> diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
> index c6170548831b..fe4cde2c30c9 100644
> --- a/fs/xfs/xfs_iomap.h
> +++ b/fs/xfs/xfs_iomap.h
> @@ -42,6 +42,7 @@ xfs_aligned_fsb_count(
> }
>
> extern const struct iomap_ops xfs_iomap_ops;
> +extern const struct iomap_ops xfs_delalloc_iomap_ops;
> extern const struct iomap_ops xfs_xattr_iomap_ops;
>
> #endif /* __XFS_IOMAP_H__*/
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index c3e74f9128e8..fb8035e3ba0b 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -865,10 +865,10 @@ xfs_setattr_size(
> if (newsize > oldsize) {
> trace_xfs_zero_eof(ip, oldsize, newsize - oldsize);
> error = iomap_zero_range(inode, oldsize, newsize - oldsize,
> - &did_zeroing, &xfs_iomap_ops);
> + &did_zeroing, &xfs_delalloc_iomap_ops);
> } else {
> error = iomap_truncate_page(inode, newsize, &did_zeroing,
> - &xfs_iomap_ops);
> + &xfs_delalloc_iomap_ops);
> }
>
> if (error)
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index e0111d31140b..ac94ace45424 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1393,7 +1393,7 @@ xfs_reflink_dirty_extents(
> if (fpos + flen > isize)
> flen = isize - fpos;
> error = iomap_file_dirty(VFS_I(ip), fpos, flen,
> - &xfs_iomap_ops);
> + &xfs_delalloc_iomap_ops);
> xfs_ilock(ip, XFS_ILOCK_EXCL);
> if (error)
> goto out;
> --
> 2.18.0
>
