On Wed, Aug 19, 2020 at 07:49:33AM +1000, Dave Chinner wrote:
> On Mon, Aug 17, 2020 at 07:53:07AM +0100, Christoph Hellwig wrote:
> > On Fri, Aug 14, 2020 at 01:38:25PM +0530, Chandan Babu R wrote:
> > > When adding a new data extent (without modifying an inode's existing
> > > extents) the extent count increases only by 1. This commit checks for
> > > extent count overflow in such cases.
> > >
> > > Signed-off-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
> > > ---
> > > fs/xfs/libxfs/xfs_bmap.c | 8 ++++++++
> > > fs/xfs/libxfs/xfs_inode_fork.h | 2 ++
> > > fs/xfs/xfs_bmap_util.c | 5 +++++
> > > fs/xfs/xfs_dquot.c | 8 +++++++-
> > > fs/xfs/xfs_iomap.c | 5 +++++
> > > fs/xfs/xfs_rtalloc.c | 5 +++++
> > > 6 files changed, 32 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > > index 9c40d5971035..e64f645415b1 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > > @@ -4527,6 +4527,14 @@ xfs_bmapi_convert_delalloc(
> > > return error;
> > >
> > > xfs_ilock(ip, XFS_ILOCK_EXCL);
> > > +
> > > + if (whichfork == XFS_DATA_FORK) {
> >
> > Should we add COW fork special casing to xfs_iext_count_may_overflow
> > instead?
That seems like a reasonable idea.
> >
> > > + error = xfs_iext_count_may_overflow(ip, whichfork,
> > > + XFS_IEXT_ADD_CNT);
> >
> > I find the XFS_IEXT_ADD_CNT define very confusing. An explicit 1 passed
> > for a counter parameter makes a lot more sense to me.
>
> I explicitly asked Chandan to convert all the magic numbers
> sprinkled in the previous patch to defined values. It was impossible
> to know whether the intended value was correct when it's just an
> open coded number because we don't know what the number actually
> stands for. And, in future, if we change the behaviour of a specific
> operation, then we only have to change a single value rather than
> having to track down and determine if every magic "1" is for an
> extent add operation or something different.
I prefer named flags over magic numbers too, though this named constant
doesn't have a comment describing what it does, and "ADD_CNT" doesn't
really tell me much. The subsequent patches have comments, so maybe
this should just become:
/*
* Worst-case increase in the fork extent count when we're adding a
* single extent to a fork and there's no possibility of splitting an
* existing mapping.
*/
#define XFS_IEXT_ADD_NOSPLIT (1)
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
