xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: fix delalloc quota accounting on failure
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Tue, 08 May 2012 09:38:25 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <4FA92DA7.4090809@xxxxxxxxxxx>
References: <1336474133-27732-1-git-send-email-david@xxxxxxxxxxxxx> <4FA92DA7.4090809@xxxxxxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20120428 Thunderbird/12.0.1
On 5/8/12 9:28 AM, 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:
> 
> diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> index b50ec5b..f771838 100644
> --- a/fs/xfs/xfs_quota.h
> +++ b/fs/xfs/xfs_quota.h
> @@ -370,8 +370,8 @@ static inline int xfs_trans_reserve_quota_bydquots(struct 
> xfs_trans *tp,
>  #define xfs_qm_unmount_quotas(mp)
>  #endif /* CONFIG_XFS_QUOTA */
>  
> -#define xfs_trans_unreserve_quota_nblks(tp, ip, nblks, ninos, flags) \
> -     xfs_trans_reserve_quota_nblks(tp, ip, -(nblks), -(ninos), flags)
> +#define xfs_trans_unreserve_quota_nblks(tp, ip, nblks, flags) \
> +     xfs_trans_reserve_quota_nblks(tp, ip, -((long)nblks), 0, flags)
>  #define xfs_trans_reserve_quota(tp, mp, ud, gd, nb, ni, f) \
>       xfs_trans_reserve_quota_bydquots(tp, mp, ud, gd, nb, ni, \
>                               f | XFS_QMOPT_RES_REGBLKS)
> 

There are also 2 other callers that "already" fixed this, sortakinda:

xfs_bunmapi()

                        /* Update realtime/data freespace, unreserve quota */
...
                                (void)xfs_trans_reserve_quota_nblks(NULL,
                                        ip, -((long)del.br_blockcount), 0,
                                        XFS_QMOPT_RES_REGBLKS);

                                (void)xfs_trans_reserve_quota_nblks(NULL,
                                        ip, -((long)del.br_blockcount), 0,
                                        XFS_QMOPT_RES_REGBLKS);

those could be an unreserve call instead, with the sign fix embedded in the 
macro.

And while we're at it it seems nobody calls xfs_trans_reserve_quota_nblks() 
with ninos != 0 so I think that arg could be removed from that function too, 
not just the macro, in another patch.

-Eric



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