[PATCH] xfs: fix delalloc quota accounting on failure

[PATCH] xfs: fix delalloc quota accounting on failure

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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@redhat.com>
---
 fs/xfs/xfs_bmap.c     |    2 +-
 fs/xfs/xfs_iomap.c    |    2 +-
 fs/xfs/xfs_vnodeops.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 9006656..2115146 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -4526,7 +4526,7 @@ out_unreserve_blocks:
 		xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS, alen, 0);
 out_unreserve_quota:
 	if (XFS_IS_QUOTA_ON(mp))
-		xfs_trans_unreserve_quota_nblks(NULL, ip, alen, 0, rt ?
+		xfs_trans_unreserve_quota_nblks(NULL, ip, (long)alen, 0, rt ?
 				XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
 	return error;
 }
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 756f093..973dff6 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -246,7 +246,7 @@ out_unlock:
 
 out_bmap_cancel:
 	xfs_bmap_cancel(&free_list);
-	xfs_trans_unreserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
+	xfs_trans_unreserve_quota_nblks(tp, ip, (long)qblocks, 0, quota_flag);
 out_trans_cancel:
 	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
 	goto out_unlock;
diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
index 57515e2..c22f4e0 100644
--- a/fs/xfs/xfs_vnodeops.c
+++ b/fs/xfs/xfs_vnodeops.c
@@ -1916,7 +1916,7 @@ xfs_alloc_file_space(
 
 error0:	/* Cancel bmap, unlock inode, unreserve quota blocks, cancel trans */
 	xfs_bmap_cancel(&free_list);
-	xfs_trans_unreserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
+	xfs_trans_unreserve_quota_nblks(tp, ip, (long)qblocks, 0, quota_flag);
 
 error1:	/* Just cancel transaction */
 	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
-- 
1.7.10

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

On 5/8/12 5:48 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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@redhat.com>

Ouch!

Reviewed-by: Eric Sandeen <sandeen@redhat.com>
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)


> ---
>  fs/xfs/xfs_bmap.c     |    2 +-
>  fs/xfs/xfs_iomap.c    |    2 +-
>  fs/xfs/xfs_vnodeops.c |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> index 9006656..2115146 100644
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
> @@ -4526,7 +4526,7 @@ out_unreserve_blocks:
>  		xfs_icsb_modify_counters(mp, XFS_SBS_FDBLOCKS, alen, 0);
>  out_unreserve_quota:
>  	if (XFS_IS_QUOTA_ON(mp))
> -		xfs_trans_unreserve_quota_nblks(NULL, ip, alen, 0, rt ?
> +		xfs_trans_unreserve_quota_nblks(NULL, ip, (long)alen, 0, rt ?
>  				XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS);
>  	return error;
>  }
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 756f093..973dff6 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -246,7 +246,7 @@ out_unlock:
>  
>  out_bmap_cancel:
>  	xfs_bmap_cancel(&free_list);
> -	xfs_trans_unreserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
> +	xfs_trans_unreserve_quota_nblks(tp, ip, (long)qblocks, 0, quota_flag);
>  out_trans_cancel:
>  	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
>  	goto out_unlock;
> diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> index 57515e2..c22f4e0 100644
> --- a/fs/xfs/xfs_vnodeops.c
> +++ b/fs/xfs/xfs_vnodeops.c
> @@ -1916,7 +1916,7 @@ xfs_alloc_file_space(
>  
>  error0:	/* Cancel bmap, unlock inode, unreserve quota blocks, cancel trans */
>  	xfs_bmap_cancel(&free_list);
> -	xfs_trans_unreserve_quota_nblks(tp, ip, qblocks, 0, quota_flag);
> +	xfs_trans_unreserve_quota_nblks(tp, ip, (long)qblocks, 0, quota_flag);
>  
>  error1:	/* Just cancel transaction */
>  	xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Christoph Hellwig]

On Tue, May 08, 2012 at 08:48:53PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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.

eww.  Wouldn't it be better to make handle that inside
xfs_trans_unreserve_quota_nblks?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

On 5/8/12 9:28 AM, Eric Sandeen wrote:
> On 5/8/12 5:48 AM, Dave Chinner wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>>
>> 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@redhat.com>
> 
> Ouch!
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 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



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

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@redhat.com>
> > 
> > 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@redhat.com>
> 
> Ouch!
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 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....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs