| Message ID | 20191213171258.36934-2-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: hold ilock across insert and collapse range | expand |
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> > --- > 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; > >
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; > >
On Fri, Dec 13, 2019 at 12:12:56PM -0500, 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, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --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; > > -- > 2.20.1 >
On Fri, Dec 13, 2019 at 12:12:56PM -0500, 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. The helper already is rather questionable as-is, so even without looking at the following patches: Reviewed-by: Christoph Hellwig <hch@lst.de>
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;
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> --- 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(-)