Re: [PATCH V4 09/10] xfs: Check for extent overflow when swapping extents

Chandan Babu R
2020-09-19T09:44:22Z

On Friday 18 September 2020 9:14:45 PM IST Darrick J. Wong wrote:
> On Fri, Sep 18, 2020 at 03:17:58PM +0530, Chandan Babu R wrote:
> > Removing an initial range of source/donor file's extent and adding a new
> > extent (from donor/source file) in its place will cause extent count to
> > increase by 1.
> >
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> > fs/xfs/libxfs/xfs_bmap.c | 18 +++++++++---------
> > fs/xfs/libxfs/xfs_bmap.h | 1 +
> > fs/xfs/libxfs/xfs_inode_fork.h | 7 +++++++
> > fs/xfs/xfs_bmap_util.c | 17 +++++++++++++++++
> > 4 files changed, 34 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 51c2d2690f05..9c665e379dfc 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -6104,15 +6104,6 @@ xfs_bmap_split_extent(
> > return error;
> > }
> >
> > -/* Deferred mapping is only for real extents in the data fork. */
> > -static bool
> > -xfs_bmap_is_update_needed(
> > - struct xfs_bmbt_irec *bmap)
> > -{
> > - return bmap->br_startblock != HOLESTARTBLOCK &&
> > - bmap->br_startblock != DELAYSTARTBLOCK;
> > -}
> > -
> > /* Record a bmap intent. */
> > static int
> > __xfs_bmap_add(
> > @@ -6144,6 +6135,15 @@ __xfs_bmap_add(
> > return 0;
> > }
> >
> > +/* Deferred mapping is only for real extents in the data fork. */
> > +bool
> > +xfs_bmap_is_update_needed(
> > + struct xfs_bmbt_irec *bmap)
> > +{
> > + return bmap->br_startblock != HOLESTARTBLOCK &&
> > + bmap->br_startblock != DELAYSTARTBLOCK;
> > +}
>
> I think the predicate you want below is xfs_bmap_is_real_extent().

Yes, that is indeed correct. I will fix this one too.

>
> (I think that mostly because I'm going to kill this predicate entirely
> in a patch for the next cycle, because it is redundant and
> _is_real_extent is a better name.)
>
> --D
>
> > +
> > /* Map an extent into a file. */
> > void
> > xfs_bmap_map_extent(
> > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> > index e1bd484e5548..60fbe184d5f4 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.h
> > +++ b/fs/xfs/libxfs/xfs_bmap.h
> > @@ -263,6 +263,7 @@ struct xfs_bmap_intent {
> > struct xfs_bmbt_irec bi_bmap;
> > };
> >
> > +bool xfs_bmap_is_update_needed(struct xfs_bmbt_irec *bmap);
> > int xfs_bmap_finish_one(struct xfs_trans *tp, struct xfs_inode *ip,
> > enum xfs_bmap_intent_type type, int whichfork,
> > xfs_fileoff_t startoff, xfs_fsblock_t startblock,
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index ded3c1b56c94..837c01595439 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -102,6 +102,13 @@ struct xfs_ifork {
> > #define XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written) \
> > (((smap_real) ? 1 : 0) + ((dmap_written) ? 1 : 0))
> >
> > +/*
> > + * Removing an initial range of source/donor file's extent and adding a new
> > + * extent (from donor/source file) in its place will cause extent count to
> > + * increase by 1.
> > + */
> > +#define XFS_IEXT_SWAP_RMAP_CNT (1)
> > +
> > /*
> > * Fork handling.
> > */
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 0776abd0103c..542f990247c4 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -28,6 +28,7 @@
> > #include "xfs_icache.h"
> > #include "xfs_iomap.h"
> > #include "xfs_reflink.h"
> > +#include "xfs_bmap.h"
> >
> > /* Kernel only BMAP related definitions and functions */
> >
> > @@ -1407,6 +1408,22 @@ xfs_swap_extent_rmap(
> > irec.br_blockcount);
> > trace_xfs_swap_extent_rmap_remap_piece(tip, &uirec);
> >
> > + if (xfs_bmap_is_update_needed(&uirec)) {
> > + error = xfs_iext_count_may_overflow(ip,
> > + XFS_DATA_FORK,
> > + XFS_IEXT_SWAP_RMAP_CNT);
> > + if (error)
> > + goto out;
> > + }
> > +
> > + if (xfs_bmap_is_update_needed(&irec)) {
> > + error = xfs_iext_count_may_overflow(tip,
> > + XFS_DATA_FORK,
> > + XFS_IEXT_SWAP_RMAP_CNT);
> > + if (error)
> > + goto out;
> > + }
> > +
> > /* Remove the mapping from the donor file. */
> > xfs_bmap_unmap_extent(tp, tip, &uirec);
> >
>

--
chandan