Re: [PATCH] xfs: prevent 32bit overflow in space reservation

[Dave Chinner <david@fromorbit.com>]

On Thu, Sep 02, 2010 at 10:51:19AM -0500, Alex Elder wrote:
> On Thu, 2010-09-02 at 15:17 +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > If we attempt to preallocate more than 2^32 blocks of space in a
> > single syscall, the transaction block reservation will overflow
> > leading to a hangs in the superblock block accounting code. This
> > is trivially reproduced with xfs_io. Fix the problem by capping the
> > allocation reservation to the maximum number of blocks a single
> > xfs_bmapi() call can allocate (2^21 blocks).
> 
> This looks OK, but I have two comments, below.
> 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_vnodeops.c |   12 +++++++++---
> >  1 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c
> > index 66d585c..91dd9c8 100644
> > --- a/fs/xfs/xfs_vnodeops.c
> > +++ b/fs/xfs/xfs_vnodeops.c
> > @@ -2299,15 +2299,21 @@ xfs_alloc_file_space(
> >  			e = allocatesize_fsb;
> >  		}
> >  
> > +		/*
> > +		 * we can't allocate more than @nimaps extents at a time,
> > +		 * so prevent a 32bit overflow on the transaction reserve
> > +		 * by trying to reserve > 16TB worth of blocks for the
> > +		 * preallocation.
> > +
> 
> This comment could use rewording.  How about something like:
> 
> 	A 32-bit block count limits the amount of space that can
> 	be reserved in a transaction, so we need to limit the
> 	number of blocks reserved to avoid overflow.  We can't
> 	allocate more than @nimaps extents (whose size won't
> 	exceed 32 bits) at a time anyway, so use that to enforce
> 	the limit.

Ok, make sense - I'll reword it.

> > 		 */
> > +		resblks = min_t(xfs_fileoff_t, (e - s), (MAXEXTLEN * nimaps));
> 
> I guess it's clear that MAXEXTLEN fits in 32 bits because of
> sizeof (xfs_extlen_t).

True, but if sizeof(xfs_extlen_t) was the limiting factor,
then the mulitply could still cause 32bit overflows.

The real reason is that MAXEXTLEN defines the maximum extent length
supported by the on disk bmap btree record format. The record format
defines the extent length in FSBs to be:

#define MAXEXTLEN       ((xfs_extlen_t)0x001fffff)      /* 21 bits */

and as such fits easily into the 32 bit limit.

> And inspection shows that nimaps is
> just 1, so this does the 32-bit limiting.  But that just
> seems indirect.

nimaps can be up to:

#define XFS_BMAP_MAX_NMAP 4

So if we change the loop to do more allocations per loop, then
the code will already handle it correctly. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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