* [PATCH] xfs: fix delalloc quota accounting on failure @ 2012-05-08 10:48 Dave Chinner 2012-05-08 14:28 ` Eric Sandeen 2012-05-08 14:31 ` Christoph Hellwig 0 siblings, 2 replies; 5+ messages in thread From: Dave Chinner @ 2012-05-08 10:48 UTC (permalink / raw) To: xfs 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: fix delalloc quota accounting on failure 2012-05-08 10:48 [PATCH] xfs: fix delalloc quota accounting on failure Dave Chinner @ 2012-05-08 14:28 ` Eric Sandeen 2012-05-08 14:38 ` Eric Sandeen 2012-05-08 22:35 ` Dave Chinner 2012-05-08 14:31 ` Christoph Hellwig 1 sibling, 2 replies; 5+ messages in thread From: Eric Sandeen @ 2012-05-08 14:28 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: fix delalloc quota accounting on failure 2012-05-08 14:28 ` Eric Sandeen @ 2012-05-08 14:38 ` Eric Sandeen 2012-05-08 22:35 ` Dave Chinner 1 sibling, 0 replies; 5+ messages in thread From: Eric Sandeen @ 2012-05-08 14:38 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: fix delalloc quota accounting on failure 2012-05-08 14:28 ` Eric Sandeen 2012-05-08 14:38 ` Eric Sandeen @ 2012-05-08 22:35 ` Dave Chinner 1 sibling, 0 replies; 5+ messages in thread From: Dave Chinner @ 2012-05-08 22:35 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: fix delalloc quota accounting on failure 2012-05-08 10:48 [PATCH] xfs: fix delalloc quota accounting on failure Dave Chinner 2012-05-08 14:28 ` Eric Sandeen @ 2012-05-08 14:31 ` Christoph Hellwig 1 sibling, 0 replies; 5+ messages in thread From: Christoph Hellwig @ 2012-05-08 14:31 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-05-08 22:35 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-08 10:48 [PATCH] xfs: fix delalloc quota accounting on failure Dave Chinner 2012-05-08 14:28 ` Eric Sandeen 2012-05-08 14:38 ` Eric Sandeen 2012-05-08 22:35 ` Dave Chinner 2012-05-08 14:31 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox