On Wednesday 19 August 2020 3:27:46 AM IST Darrick J. Wong wrote:
> 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)
>
That is perfect. Thanks for the suggestion. I will add that in the next
version of this patchset.
--
chandan
