xfs
[Top] [All Lists]

Re: [PATCH 27/30] libxfs: work around do_div() not handling 32 bit numer

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 27/30] libxfs: work around do_div() not handling 32 bit numerators
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 5 Nov 2013 12:55:53 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131104091643.GG23564@xxxxxxxxxxxxx>
References: <1383107481-28937-1-git-send-email-david@xxxxxxxxxxxxx> <1383107481-28937-28-git-send-email-david@xxxxxxxxxxxxx> <20131104091643.GG23564@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Nov 04, 2013 at 01:16:43AM -0800, Christoph Hellwig wrote:
> On Wed, Oct 30, 2013 at 03:31:18PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > The libxfs dquot buffer code uses do_div() with a 32 bit numerator.
> > This gives incorrect results as do_div() passes the numerator by
> > reference as a pointer to a 64 bit value. Hence it does the division
> > using 32 bits of garbage gives the wrong result. The kernel code
> > handles 32 bit numerators just fine, so this patch is a temporary
> > workaround in the dquot buffer code until we fix do_div() to handle
> > 32 bit numerators correctly.
> 
> The right fix is to simply stop using do_div here, both in kernel and
> userspace.

Oh, right, I was thinking that BBTOB() resulted in a u64 value like
BTOBB() and BTOBBT() do. It doesn't have a built in cast, so takes
whatever type the source variable has.

> >  xfs_calc_dquots_per_chunk(
> >     struct xfs_mount        *mp,
> >     unsigned int            nbblks) /* basic block units */
> >  {
> > -   unsigned int    ndquots;
> > +   uint64_t        ndquots;
> >  
> >     ASSERT(nbblks > 0);
> >     ndquots = BBTOB(nbblks);
> >     do_div(ndquots, sizeof(xfs_dqblk_t));
> >  
> > -   return ndquots;
> > +   return (int)ndquots;
> 
>       ndquots = BBTOB(nbblks) / sizeof(xfs_dqblk_t);

Yup, that should work.

> Also the current xfs_calc_dquots_per_chunk doesn't use the mp argument
> anyway, but all callers do.  I think in the end we'd want to take that
> in from the calers end end up with a version that includes the
> check from the quotainfo.  But for getting userspace up and running my
> above minimal version should do it.

Yes, changed to fix. I'll drive further cleanups from the kernel
side here...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>