On Thu, Sep 21, 2017 at 03:35:06PM +0200, Christoph Hellwig wrote:

On Wed, Sep 20, 2017 at 04:00:36PM -0700, Darrick J. Wong wrote:

quoted

On Mon, Sep 18, 2017 at 08:26:29AM -0700, Christoph Hellwig wrote:

quoted

Currently getbmap uses xfs_bmapi_read to query the extent map, and then
fixes up various bits that are eventually reported to userspace.

This patch instead rewrites it to use xfs_iext_lookup_extent and
xfs_iext_get_extent to iteratively process the extent map.  This not
only avoids the need to allocate a map for the returned xfs_bmbt_irec
structures but also greatly simplified the code.

There are two intentional behavior changes compared to the old code:

 - the current code reports unwritten extents that don't directly border
   a written one as unwritten even when not passing the BMV_IF_PREALLOC
   option, contrary to the documentation.  The new code requires the
   BMV_IF_PREALLOC flag to report the unwrittent extent bit.
 - The new code does never merges consecutive extents, unlike the old
   code that sometimes does it based on the boundaries of the
   xfs_bmapi_read calls.  Note that the extent merging behavior was
   entirely undocumented.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_bmap_util.c | 525 ++++++++++++++++++++-----------------------------
 1 file changed, 208 insertions(+), 317 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index cd9a5400ba4f..a87d05978c92 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c

@@ -403,125 +403,103 @@ xfs_bmap_count_blocks(
 	return 0;
 }
 
-/*
- * returns 1 for success, 0 if we failed to map the extent.
- */
-STATIC int
-xfs_getbmapx_fix_eof_hole(
-	xfs_inode_t		*ip,		/* xfs incore inode pointer */
-	int			whichfork,
-	struct getbmapx		*out,		/* output structure */
-	int			prealloced,	/* this is a file with
-						 * preallocated data space */
-	int64_t			end,		/* last block requested */
-	xfs_fsblock_t		startblock,
-	bool			moretocome)
+static int
+xfs_getbmap_report_one(
+	struct xfs_inode	*ip,
+	struct getbmapx		*bmv,
+	struct getbmapx		*out,
+	int64_t			bmv_end,
+	struct xfs_bmbt_irec	*got)
 {
-	int64_t			fixlen;
-	xfs_mount_t		*mp;		/* file system mount point */
-	xfs_ifork_t		*ifp;		/* inode fork pointer */
-	xfs_extnum_t		lastx;		/* last extent pointer */
-	xfs_fileoff_t		fileblock;
-
-	if (startblock == HOLESTARTBLOCK) {
-		mp = ip->i_mount;
-		out->bmv_block = -1;
-		fixlen = XFS_FSB_TO_BB(mp, XFS_B_TO_FSB(mp, XFS_ISIZE(ip)));
-		fixlen -= out->bmv_offset;
-		if (prealloced && out->bmv_offset + out->bmv_length == end) {
-			/* Came to hole at EOF. Trim it. */
-			if (fixlen <= 0)
-				return 0;
-			out->bmv_length = fixlen;
-		}
+	struct getbmapx		*p = out + bmv->bmv_entries;
+	bool			shared = false, trimmed = false;
+	int			error;
+
+	error = xfs_reflink_trim_around_shared(ip, got, &shared, &trimmed);
+	if (error)
+		return error;
+
+	if (isnullstartblock(got->br_startblock) ||
+	    got->br_startblock == DELAYSTARTBLOCK) {
+		/*
+		 * Delalloc extents that start beyond EOF can occur due to
+		 * speculative EOF allocation when the delalloc extent is larger
+		 * than the largest freespace extent at conversion time.  These
+		 * extents cannot be converted by data writeback, so can exist
+		 * here even if we are not supposed to be finding delalloc
+		 * extents.
+		 */
+		if (got->br_startoff < XFS_B_TO_FSB(ip->i_mount, XFS_ISIZE(ip)))
+			ASSERT((bmv->bmv_iflags & BMV_IF_DELALLOC) != 0);
+
+		p->bmv_oflags |= BMV_OF_DELALLOC;
+		p->bmv_block = -2;

Could you please turn the special bmv_block values (-2 for delayed
allocation, -1 for hole) into defined constants in xfs_fs.h?

I'm particularly cranky about bmv_block == -1 since there isn't even a
BMV_OF_ flag for holes.

I can prepare a patch for it, but I don't want to throw random cleanups
into this series which I need as a preparation for the extent list
rework.

Yes, please! :)

quoted

quoted

+	if (got->br_state == XFS_EXT_UNWRITTEN &&
+	    (bmv->bmv_iflags & BMV_IF_PREALLOC))
+		p->bmv_oflags |= BMV_OF_PREALLOC;

Am I the only one who thought (from the xfs_bmap manpage) that you're
supposed to BMV_IF_PREALLOC if you want the output to contain prealloc
extents, and omit the flag if you don't want them?

Versus what the kernel actually does, which seems to be to merge extents
together if you don't pass the flag:

$ xfs_io -c 'bmap -vvvv' moo
moo:
 EXT: FILE-OFFSET      BLOCK-RANGE          AG AG-OFFSET        TOTAL
   0: [0..39]:         335288488..335288527  7 (736424..736463)    40

$ xfs_io -c 'bmap -vvvv -p' moo
moo:
 EXT: FILE-OFFSET      BLOCK-RANGE          AG AG-OFFSET        TOTAL FLAGS
   0: [0..7]:          335288488..335288495  7 (736424..736431)     8 000000
   1: [8..39]:         335288496..335288527  7 (736432..736463)    32 010000

Eh.  I guess the old code would report prealloc extents, it just doesn't
flag them, so this is ok.

The old code even flags them if there is no normal extent to merge them
with, but I consider that a bug I didn't want to follow in the new
code.  E.g. try creating a sparse file and just preallocate an extent
in it, and it will be marked as preallocated.

I never understood the point of the BMV_IF_PREALLOC flag - why would
we ever want to not report preallocated extents?  We also set
the new BMV_OF_SHARED unconditionally for example.

I don't really understand the bmap behavior either, but I do get a whiff
of 'historical reasons' :)

--D

--
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