Re: [PATCH 1/3] xfs: open code insert range extent split helper

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Dec 17, 2019 at 10:02:42AM -0700, Allison Collins wrote:
> On 12/13/19 10:12 AM, Brian Foster wrote:
> > The insert range operation currently splits the extent at the target
> > offset in a separate transaction and lock cycle from the one that
> > shifts extents. In preparation for reworking insert range into an
> > atomic operation, lift the code into the caller so it can be easily
> > condensed to a single rolling transaction and lock cycle and
> > eliminate the helper. No functional changes.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> 
> Looks ok to me.
> 
> Reviewed by: Allison Collins <allison.henderson@oracle.com>

"Reviewed-by", with dash?

--D

> > ---
> >   fs/xfs/libxfs/xfs_bmap.c | 32 ++------------------------------
> >   fs/xfs/libxfs/xfs_bmap.h |  3 ++-
> >   fs/xfs/xfs_bmap_util.c   | 14 +++++++++++++-
> >   3 files changed, 17 insertions(+), 32 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index a9ad1f991ba3..2bba0f983e4f 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -6021,8 +6021,8 @@ xfs_bmap_insert_extents(
> >    * @split_fsb is a block where the extents is split.  If split_fsb lies in a
> >    * hole or the first block of extents, just return 0.
> >    */
> > -STATIC int
> > -xfs_bmap_split_extent_at(
> > +int
> > +xfs_bmap_split_extent(
> >   	struct xfs_trans	*tp,
> >   	struct xfs_inode	*ip,
> >   	xfs_fileoff_t		split_fsb)
> > @@ -6138,34 +6138,6 @@ xfs_bmap_split_extent_at(
> >   	return error;
> >   }
> > -int
> > -xfs_bmap_split_extent(
> > -	struct xfs_inode        *ip,
> > -	xfs_fileoff_t           split_fsb)
> > -{
> > -	struct xfs_mount        *mp = ip->i_mount;
> > -	struct xfs_trans        *tp;
> > -	int                     error;
> > -
> > -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write,
> > -			XFS_DIOSTRAT_SPACE_RES(mp, 0), 0, 0, &tp);
> > -	if (error)
> > -		return error;
> > -
> > -	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > -	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> > -
> > -	error = xfs_bmap_split_extent_at(tp, ip, split_fsb);
> > -	if (error)
> > -		goto out;
> > -
> > -	return xfs_trans_commit(tp);
> > -
> > -out:
> > -	xfs_trans_cancel(tp);
> > -	return error;
> > -}
> > -
> >   /* Deferred mapping is only for real extents in the data fork. */
> >   static bool
> >   xfs_bmap_is_update_needed(
> > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> > index 14d25e0b7d9c..f3259ad5c22c 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.h
> > +++ b/fs/xfs/libxfs/xfs_bmap.h
> > @@ -222,7 +222,8 @@ int	xfs_bmap_can_insert_extents(struct xfs_inode *ip, xfs_fileoff_t off,
> >   int	xfs_bmap_insert_extents(struct xfs_trans *tp, struct xfs_inode *ip,
> >   		xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
> >   		bool *done, xfs_fileoff_t stop_fsb);
> > -int	xfs_bmap_split_extent(struct xfs_inode *ip, xfs_fileoff_t split_offset);
> > +int	xfs_bmap_split_extent(struct xfs_trans *tp, struct xfs_inode *ip,
> > +		xfs_fileoff_t split_offset);
> >   int	xfs_bmapi_reserve_delalloc(struct xfs_inode *ip, int whichfork,
> >   		xfs_fileoff_t off, xfs_filblks_t len, xfs_filblks_t prealloc,
> >   		struct xfs_bmbt_irec *got, struct xfs_iext_cursor *cur,
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 2efd78a9719e..829ab1a804c9 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1139,7 +1139,19 @@ xfs_insert_file_space(
> >   	 * is not the starting block of extent, we need to split the extent at
> >   	 * stop_fsb.
> >   	 */
> > -	error = xfs_bmap_split_extent(ip, stop_fsb);
> > +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write,
> > +			XFS_DIOSTRAT_SPACE_RES(mp, 0), 0, 0, &tp);
> > +	if (error)
> > +		return error;
> > +
> > +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> > +
> > +	error = xfs_bmap_split_extent(tp, ip, stop_fsb);
> > +	if (error)
> > +		goto out_trans_cancel;
> > +
> > +	error = xfs_trans_commit(tp);
> >   	if (error)
> >   		return error;
> >