On Mon, Dec 03, 2018 at 05:24:54PM -0500, Christoph Hellwig wrote:
> The io_type field contains what is basically a summary of information
> from the inode fork and the imap. But we can just as easily use that
> information directly, simplifying a few bits here and there and
> improving the trace points.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_aops.c | 91 ++++++++++++++++++++------------------------
> fs/xfs/xfs_aops.h | 21 +---------
> fs/xfs/xfs_iomap.c | 8 ++--
> fs/xfs/xfs_reflink.c | 2 +-
> fs/xfs/xfs_trace.h | 28 +++++++-------
> 5 files changed, 62 insertions(+), 88 deletions(-)
>
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index d7275075878e..8fec6fd4c632 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -28,7 +28,7 @@
> */
> struct xfs_writepage_ctx {
> struct xfs_bmbt_irec imap;
> - unsigned int io_type;
> + int fork;
> unsigned int cow_seq;
> struct xfs_ioend *ioend;
> };
> @@ -255,30 +255,20 @@ xfs_end_io(
> */
> error = blk_status_to_errno(ioend->io_bio->bi_status);
> if (unlikely(error)) {
> - switch (ioend->io_type) {
> - case XFS_IO_COW:
> + if (ioend->io_fork == XFS_COW_FORK)
> xfs_reflink_cancel_cow_range(ip, offset, size, true);
> - break;
> - }
> -
> goto done;
> }
>
> /*
> - * Success: commit the COW or unwritten blocks if needed.
> + * Success: commit the COW or unwritten blocks if needed.
> */
> - switch (ioend->io_type) {
> - case XFS_IO_COW:
> + if (ioend->io_fork == XFS_COW_FORK)
> error = xfs_reflink_end_cow(ip, offset, size);
> - break;
> - case XFS_IO_UNWRITTEN:
> - /* writeback should never update isize */
> + else if (ioend->io_state == XFS_EXT_UNWRITTEN)
> error = xfs_iomap_write_unwritten(ip, offset, size, false);
> - break;
> - default:
> + else
> ASSERT(!xfs_ioend_is_append(ioend) || ioend->io_append_trans);
> - break;
> - }
>
> done:
> if (ioend->io_append_trans)
> @@ -293,7 +283,8 @@ xfs_end_bio(
> struct xfs_ioend *ioend = bio->bi_private;
> struct xfs_mount *mp = XFS_I(ioend->io_inode)->i_mount;
>
> - if (ioend->io_type == XFS_IO_UNWRITTEN || ioend->io_type == XFS_IO_COW)
> + if (ioend->io_fork == XFS_COW_FORK ||
> + ioend->io_state == XFS_EXT_UNWRITTEN)
> queue_work(mp->m_unwritten_workqueue, &ioend->io_work);
> else if (ioend->io_append_trans)
> queue_work(mp->m_data_workqueue, &ioend->io_work);
> @@ -313,7 +304,6 @@ xfs_map_blocks(
> xfs_fileoff_t offset_fsb = XFS_B_TO_FSBT(mp, offset), end_fsb;
> xfs_fileoff_t cow_fsb = NULLFILEOFF;
> struct xfs_bmbt_irec imap;
> - int whichfork = XFS_DATA_FORK;
> struct xfs_iext_cursor icur;
> bool imap_valid;
> int error = 0;
> @@ -350,7 +340,7 @@ xfs_map_blocks(
> offset_fsb < wpc->imap.br_startoff + wpc->imap.br_blockcount;
> if (imap_valid &&
> (!xfs_inode_has_cow_data(ip) ||
> - wpc->io_type == XFS_IO_COW ||
> + wpc->fork == XFS_COW_FORK ||
> wpc->cow_seq == READ_ONCE(ip->i_cowfp->if_seq)))
> return 0;
>
> @@ -382,6 +372,9 @@ xfs_map_blocks(
> if (cow_fsb != NULLFILEOFF && cow_fsb <= offset_fsb) {
> wpc->cow_seq = READ_ONCE(ip->i_cowfp->if_seq);
> xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +
> + wpc->fork = XFS_COW_FORK;
> +
> /*
> * Truncate can race with writeback since writeback doesn't
> * take the iolock and truncate decreases the file size before
> @@ -394,11 +387,13 @@ xfs_map_blocks(
> * will kill the contents anyway.
> */
> if (offset > i_size_read(inode)) {
> - wpc->io_type = XFS_IO_HOLE;
> + wpc->imap.br_blockcount = end_fsb - offset_fsb;
> + wpc->imap.br_startoff = offset_fsb;
> + wpc->imap.br_startblock = HOLESTARTBLOCK;
> + wpc->imap.br_state = XFS_EXT_NORM;
> return 0;
> }
> - whichfork = XFS_COW_FORK;
> - wpc->io_type = XFS_IO_COW;
> +
> goto allocate_blocks;
> }
>
> @@ -419,12 +414,14 @@ xfs_map_blocks(
> imap.br_startoff = end_fsb; /* fake a hole past EOF */
> xfs_iunlock(ip, XFS_ILOCK_SHARED);
>
> + wpc->fork = XFS_DATA_FORK;
> +
> if (imap.br_startoff > offset_fsb) {
> /* landed in a hole or beyond EOF */
> imap.br_blockcount = imap.br_startoff - offset_fsb;
> imap.br_startoff = offset_fsb;
> imap.br_startblock = HOLESTARTBLOCK;
> - wpc->io_type = XFS_IO_HOLE;
> + imap.br_state = XFS_EXT_NORM;
> } else {
> /*
> * Truncate to the next COW extent if there is one. This is the
> @@ -436,30 +433,23 @@ xfs_map_blocks(
> cow_fsb < imap.br_startoff + imap.br_blockcount)
> imap.br_blockcount = cow_fsb - imap.br_startoff;
>
> - if (isnullstartblock(imap.br_startblock)) {
> - /* got a delalloc extent */
> - wpc->io_type = XFS_IO_DELALLOC;
> + /* got a delalloc extent? */
> + if (isnullstartblock(imap.br_startblock))
> goto allocate_blocks;
> - }
> -
> - if (imap.br_state == XFS_EXT_UNWRITTEN)
> - wpc->io_type = XFS_IO_UNWRITTEN;
> - else
> - wpc->io_type = XFS_IO_OVERWRITE;
> }
>
> wpc->imap = imap;
> - trace_xfs_map_blocks_found(ip, offset, count, wpc->io_type, &imap);
> + trace_xfs_map_blocks_found(ip, offset, count, wpc->fork, &imap);
> return 0;
> allocate_blocks:
> - error = xfs_iomap_write_allocate(ip, whichfork, offset, &imap,
> + error = xfs_iomap_write_allocate(ip, wpc->fork, offset, &imap,
> &wpc->cow_seq);
> if (error)
> return error;
> - ASSERT(whichfork == XFS_COW_FORK || cow_fsb == NULLFILEOFF ||
> + ASSERT(wpc->fork == XFS_COW_FORK || cow_fsb == NULLFILEOFF ||
> imap.br_startoff + imap.br_blockcount <= cow_fsb);
> wpc->imap = imap;
> - trace_xfs_map_blocks_alloc(ip, offset, count, wpc->io_type, &imap);
> + trace_xfs_map_blocks_alloc(ip, offset, count, wpc->fork, &imap);
> return 0;
> }
>
> @@ -484,7 +474,7 @@ xfs_submit_ioend(
> int status)
> {
> /* Convert CoW extents to regular */
> - if (!status && ioend->io_type == XFS_IO_COW) {
> + if (!status && ioend->io_fork == XFS_COW_FORK) {
> /*
> * Yuk. This can do memory allocation, but is not a
> * transactional operation so everything is done in GFP_KERNEL
> @@ -502,7 +492,8 @@ xfs_submit_ioend(
>
> /* Reserve log space if we might write beyond the on-disk inode size. */
> if (!status &&
> - ioend->io_type != XFS_IO_UNWRITTEN &&
> + (ioend->io_fork == XFS_COW_FORK ||
> + ioend->io_state != XFS_EXT_UNWRITTEN) &&
> xfs_ioend_is_append(ioend) &&
> !ioend->io_append_trans)
> status = xfs_setfilesize_trans_alloc(ioend);
> @@ -531,7 +522,8 @@ xfs_submit_ioend(
> static struct xfs_ioend *
> xfs_alloc_ioend(
> struct inode *inode,
> - unsigned int type,
> + int fork,
> + xfs_exntst_t state,
> xfs_off_t offset,
> struct block_device *bdev,
> sector_t sector)
> @@ -545,7 +537,8 @@ xfs_alloc_ioend(
>
> ioend = container_of(bio, struct xfs_ioend, io_inline_bio);
> INIT_LIST_HEAD(&ioend->io_list);
> - ioend->io_type = type;
> + ioend->io_fork = fork;
> + ioend->io_state = state;
> ioend->io_inode = inode;
> ioend->io_size = 0;
> ioend->io_offset = offset;
> @@ -606,13 +599,15 @@ xfs_add_to_ioend(
> sector = xfs_fsb_to_db(ip, wpc->imap.br_startblock) +
> ((offset - XFS_FSB_TO_B(mp, wpc->imap.br_startoff)) >> 9);
>
> - if (!wpc->ioend || wpc->io_type != wpc->ioend->io_type ||
> + if (!wpc->ioend ||
> + wpc->fork != wpc->ioend->io_fork ||
> + wpc->imap.br_state != wpc->ioend->io_state ||
> sector != bio_end_sector(wpc->ioend->io_bio) ||
> offset != wpc->ioend->io_offset + wpc->ioend->io_size) {
> if (wpc->ioend)
> list_add(&wpc->ioend->io_list, iolist);
> - wpc->ioend = xfs_alloc_ioend(inode, wpc->io_type, offset,
> - bdev, sector);
> + wpc->ioend = xfs_alloc_ioend(inode, wpc->fork,
> + wpc->imap.br_state, offset, bdev, sector);
> }
>
> if (!__bio_try_merge_page(wpc->ioend->io_bio, page, len, poff)) {
> @@ -721,7 +716,7 @@ xfs_writepage_map(
> error = xfs_map_blocks(wpc, inode, file_offset);
> if (error)
> break;
> - if (wpc->io_type == XFS_IO_HOLE)
> + if (wpc->imap.br_startblock == HOLESTARTBLOCK)
> continue;
> xfs_add_to_ioend(inode, file_offset, page, iop, wpc, wbc,
> &submit_list);
> @@ -916,9 +911,7 @@ xfs_vm_writepage(
> struct page *page,
> struct writeback_control *wbc)
> {
> - struct xfs_writepage_ctx wpc = {
> - .io_type = XFS_IO_HOLE,
> - };
> + struct xfs_writepage_ctx wpc = { };
> int ret;
>
> ret = xfs_do_writepage(page, wbc, &wpc);
> @@ -932,9 +925,7 @@ xfs_vm_writepages(
> struct address_space *mapping,
> struct writeback_control *wbc)
> {
> - struct xfs_writepage_ctx wpc = {
> - .io_type = XFS_IO_HOLE,
> - };
> + struct xfs_writepage_ctx wpc = { };
> int ret;
>
> xfs_iflags_clear(XFS_I(mapping->host), XFS_ITRUNCATED);
> diff --git a/fs/xfs/xfs_aops.h b/fs/xfs/xfs_aops.h
> index 494b4338446e..6c2615b83c5d 100644
> --- a/fs/xfs/xfs_aops.h
> +++ b/fs/xfs/xfs_aops.h
> @@ -8,30 +8,13 @@
>
> extern struct bio_set xfs_ioend_bioset;
>
> -/*
> - * Types of I/O for bmap clustering and I/O completion tracking.
> - */
> -enum {
> - XFS_IO_HOLE, /* covers region without any block allocation */
> - XFS_IO_DELALLOC, /* covers delalloc region */
> - XFS_IO_UNWRITTEN, /* covers allocated but uninitialized data */
> - XFS_IO_OVERWRITE, /* covers already allocated extent */
> - XFS_IO_COW, /* covers copy-on-write extent */
> -};
> -
> -#define XFS_IO_TYPES \
> - { XFS_IO_HOLE, "hole" }, \
> - { XFS_IO_DELALLOC, "delalloc" }, \
> - { XFS_IO_UNWRITTEN, "unwritten" }, \
> - { XFS_IO_OVERWRITE, "overwrite" }, \
> - { XFS_IO_COW, "CoW" }
> -
> /*
> * Structure for buffered I/O completions.
> */
> struct xfs_ioend {
> struct list_head io_list; /* next ioend in chain */
> - unsigned int io_type; /* delalloc / unwritten */
> + int io_fork; /* inode fork written back */
> + xfs_exntst_t io_state; /* extent state */
> struct inode *io_inode; /* file being written to */
> size_t io_size; /* size of the extent */
> xfs_off_t io_offset; /* offset in the file */
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 27c93b5f029d..32a7c169e096 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -575,7 +575,7 @@ xfs_file_iomap_begin_delay(
> goto out_unlock;
> }
>
> - trace_xfs_iomap_found(ip, offset, count, 0, &got);
> + trace_xfs_iomap_found(ip, offset, count, XFS_DATA_FORK, &got);
> goto done;
> }
>
> @@ -647,7 +647,7 @@ xfs_file_iomap_begin_delay(
> * them out if the write happens to fail.
> */
> iomap->flags |= IOMAP_F_NEW;
> - trace_xfs_iomap_alloc(ip, offset, count, 0, &got);
> + trace_xfs_iomap_alloc(ip, offset, count, XFS_DATA_FORK, &got);
> done:
> if (isnullstartblock(got.br_startblock))
> got.br_startblock = DELAYSTARTBLOCK;
> @@ -1139,7 +1139,7 @@ xfs_file_iomap_begin(
> return error;
>
> iomap->flags |= IOMAP_F_NEW;
> - trace_xfs_iomap_alloc(ip, offset, length, 0, &imap);
> + trace_xfs_iomap_alloc(ip, offset, length, XFS_DATA_FORK, &imap);
>
> out_finish:
> if (xfs_ipincount(ip) && (ip->i_itemp->ili_fsync_fields
> @@ -1155,7 +1155,7 @@ xfs_file_iomap_begin(
> out_found:
> ASSERT(nimaps);
> xfs_iunlock(ip, lockmode);
> - trace_xfs_iomap_found(ip, offset, length, 0, &imap);
> + trace_xfs_iomap_found(ip, offset, length, XFS_DATA_FORK, &imap);
> goto out_finish;
>
> out_unlock:
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 322a852ce284..a8c32632090c 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1148,7 +1148,7 @@ xfs_reflink_remap_blocks(
> break;
> ASSERT(nimaps == 1);
>
> - trace_xfs_reflink_remap_imap(src, srcoff, len, XFS_IO_OVERWRITE,
> + trace_xfs_reflink_remap_imap(src, srcoff, len, XFS_DATA_FORK,
> &imap);
>
> /* Translate imap into the destination file. */
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 8a6532aae779..870865913bd8 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -1210,15 +1210,15 @@ DEFINE_READPAGE_EVENT(xfs_vm_readpages);
>
> DECLARE_EVENT_CLASS(xfs_imap_class,
> TP_PROTO(struct xfs_inode *ip, xfs_off_t offset, ssize_t count,
> - int type, struct xfs_bmbt_irec *irec),
> - TP_ARGS(ip, offset, count, type, irec),
> + int whichfork, struct xfs_bmbt_irec *irec),
> + TP_ARGS(ip, offset, count, whichfork, irec),
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(xfs_ino_t, ino)
> __field(loff_t, size)
> __field(loff_t, offset)
> __field(size_t, count)
> - __field(int, type)
> + __field(int, whichfork)
> __field(xfs_fileoff_t, startoff)
> __field(xfs_fsblock_t, startblock)
> __field(xfs_filblks_t, blockcount)
> @@ -1229,33 +1229,33 @@ DECLARE_EVENT_CLASS(xfs_imap_class,
> __entry->size = ip->i_d.di_size;
> __entry->offset = offset;
> __entry->count = count;
> - __entry->type = type;
> + __entry->whichfork = whichfork;
> __entry->startoff = irec ? irec->br_startoff : 0;
> __entry->startblock = irec ? irec->br_startblock : 0;
> __entry->blockcount = irec ? irec->br_blockcount : 0;
> ),
> TP_printk("dev %d:%d ino 0x%llx size 0x%llx offset 0x%llx count %zd "
> - "type %s startoff 0x%llx startblock %lld blockcount 0x%llx",
> + "fork %s startoff 0x%llx startblock %lld blockcount 0x%llx",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> __entry->ino,
> __entry->size,
> __entry->offset,
> __entry->count,
> - __print_symbolic(__entry->type, XFS_IO_TYPES),
> + __entry->whichfork == XFS_COW_FORK ? "cow" : "data",
> __entry->startoff,
> (int64_t)__entry->startblock,
> __entry->blockcount)
> )
>
> -#define DEFINE_IOMAP_EVENT(name) \
> +#define DEFINE_IMAP_EVENT(name) \
> DEFINE_EVENT(xfs_imap_class, name, \
> TP_PROTO(struct xfs_inode *ip, xfs_off_t offset, ssize_t count, \
> - int type, struct xfs_bmbt_irec *irec), \
> - TP_ARGS(ip, offset, count, type, irec))
> -DEFINE_IOMAP_EVENT(xfs_map_blocks_found);
> -DEFINE_IOMAP_EVENT(xfs_map_blocks_alloc);
> -DEFINE_IOMAP_EVENT(xfs_iomap_alloc);
> -DEFINE_IOMAP_EVENT(xfs_iomap_found);
> + int whichfork, struct xfs_bmbt_irec *irec), \
> + TP_ARGS(ip, offset, count, whichfork, irec))
> +DEFINE_IMAP_EVENT(xfs_map_blocks_found);
> +DEFINE_IMAP_EVENT(xfs_map_blocks_alloc);
> +DEFINE_IMAP_EVENT(xfs_iomap_alloc);
> +DEFINE_IMAP_EVENT(xfs_iomap_found);
>
> DECLARE_EVENT_CLASS(xfs_simple_io_class,
> TP_PROTO(struct xfs_inode *ip, xfs_off_t offset, ssize_t count),
> @@ -3055,7 +3055,7 @@ DEFINE_EVENT(xfs_inode_irec_class, name, \
> DEFINE_INODE_EVENT(xfs_reflink_set_inode_flag);
> DEFINE_INODE_EVENT(xfs_reflink_unset_inode_flag);
> DEFINE_ITRUNC_EVENT(xfs_reflink_update_inode_size);
> -DEFINE_IOMAP_EVENT(xfs_reflink_remap_imap);
> +DEFINE_IMAP_EVENT(xfs_reflink_remap_imap);
> TRACE_EVENT(xfs_reflink_remap_blocks_loop,
> TP_PROTO(struct xfs_inode *src, xfs_fileoff_t soffset,
> xfs_filblks_t len, struct xfs_inode *dest,
> --
> 2.19.1
>
