[Top] [All Lists]

Re: [PATCH] xfs: fix delalloc quota accounting on failure

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfs: fix delalloc quota accounting on failure
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 9 May 2012 08:35:05 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <4FA92DA7.4090809@xxxxxxxxxxx>
References: <1336474133-27732-1-git-send-email-david@xxxxxxxxxxxxx> <4FA92DA7.4090809@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, May 08, 2012 at 09:28:55AM -0500, Eric Sandeen wrote:
> On 5/8/12 5:48 AM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > xfstest 270 was causing quota reservations way beyond what was sane
> > (ten to hundreds of TB) for a 4GB filesystem. There's a sign problem
> > in the error handling path of xfs_bmapi_reserve_delalloc() because
> > xfs_trans_unreserve_quota_nblks() simple negates the value passed -
> > which doesn't work for an unsigned variable. This causes
> > reservations of close to 2^32 block instead of removing a
> > reservation of a handful of blocks.
> > 
> > Fix the same problem in the other xfs_trans_unreserve_quota_nblks()
> > callers where unsigned integer variables are used, too.
> > 
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Ouch!
> Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> as far as it goes, but a couple thoughts:
> 1) Should the cast be done in the macro so new callers don't get tripped up?
> 2) Should we just remove the ninos argument from the macro?  It's always 
> passed as 0 (and could potentially suffer the same problem)
> something like:

3) I'm going to remove the problematic macro altogether.

I just wanted to get this fix out there while I look at the other
quota problems I've found. Hint: xfstests _check_quota_usage()
doesn't actually work *at all* on XFS (always passes without
actually checking quotas), and so it has been hiding problems with
either the tests that use it or the quota code for some time....


Dave Chinner

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