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:
> > On Mon, Sep 18, 2017 at 08:26:29AM -0700, Christoph Hellwig wrote:
> > > 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@xxxxxx>
> > > ---
> > > 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! :)
> > > + 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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
