On Monday 17 August 2020 12:21:23 PM IST Christoph Hellwig wrote:
> > +int
> > +xfs_iext_count_may_overflow(
> > + struct xfs_inode *ip,
> > + int whichfork,
> > + int nr_to_add)
> > +{
> > + struct xfs_ifork *ifp;
> > + uint64_t max_exts = 0;
> > + uint64_t nr_exts;
> > +
> > + switch (whichfork) {
> > + case XFS_DATA_FORK:
> > + max_exts = MAXEXTNUM;
> > + break;
> > +
> > + case XFS_ATTR_FORK:
> > + max_exts = MAXAEXTNUM;
> > + break;
> > +
> > + default:
> > + ASSERT(0);
> > + break;
> > + }
> > +
> > + ifp = XFS_IFORK_PTR(ip, whichfork);
> > + nr_exts = ifp->if_nextents + nr_to_add;
> > +
> > + if (nr_exts > max_exts)
> > + return -EFBIG;
> > +
> > + return 0;
> > +}
>
> Maybe it's just me, but I would structure this very different (just
> cosmetic differences, though). First add a:
>
> static inline uint32_t xfs_max_extents(int whichfork)
> {
> return XFS_ATTR_FORK ? MAXAEXTNUM : MAXEXTNUM;
> }
>
> to have a single place that determines the max number of extents.
>
> And the simplify the helper down to:
>
> int
> xfs_iext_count_may_overflow(
> struct xfs_inode *ip,
> int whichfork,
> int nr_to_add)
> {
> struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
> uint64_t max_exts = xfs_max_extents(whichfork);
> uint64_t nr_exts;
>
> if (check_add_overflow(ifp->if_nextents, nr_to_add, &nr_exts) ||
> nr_exts > max_exts))
> return -EFBIG;
> return 0;
> }
>
> which actually might be small enough for an inline function now.
>
I agree. I will make the suggested changes in the next version.
--
chandan
