Re: [PATCH 1/2] xfs: dynamic speculative EOF preallocation

[Dave Chinner <david@fromorbit.com>]

On Thu, Oct 14, 2010 at 12:22:45PM -0500, Alex Elder wrote:
> On Mon, 2010-10-04 at 21:13 +1100, Dave Chinner wrote:
> > @@ -427,11 +431,25 @@ xfs_iomap_eof_want_preallocate(
> >  			if ((imap[n].br_startblock != HOLESTARTBLOCK) &&
> >  			    (imap[n].br_startblock != DELAYSTARTBLOCK))
> >  				return 0;
> > +
> >  			start_fsb += imap[n].br_blockcount;
> >  			count_fsb -= imap[n].br_blockcount;
> > +
> > +			/* count delalloc blocks beyond EOF */
> > +			if (imap[n].br_startblock == DELAYSTARTBLOCK)
> > +				found_delalloc += imap[n].br_blockcount;
> >  		}
> >  	}
> > -	*prealloc = 1;
> 
> At this point, count_fsb will be 0 (a necessary condition
> for loop termination, since count_fsb is unsigned).  Since
> found_delalloc is initially 0 (and is also unsigned), we
> can safely assume that found_delalloc >= count_fsb.  The
> only case in which found_delalloc <= count_fsb is if
> found_delalloc is also 0 (a case you cover separately,
> first, below).
> 
> Furthermore, *prealloc was assigned the value 0 at the top.
> So I think this bottom section can be simplified to:
> 	if (!found_delalloc)
> 		*prealloc = 1;

I'll have a look at this - there was some reason for the second
case, but the code has changed since I needed it and, as you
suggest, it might not be needed anymore.

> >  	error = xfs_iomap_eof_want_preallocate(mp, ip, offset, count,
> >  				ioflag, imap, XFS_WRITE_IMAPS, &prealloc);
> >  	if (error)
> > @@ -476,9 +495,25 @@ xfs_iomap_write_delay(
> >  
> >  retry:
> >  	if (prealloc) {
> > +		xfs_fileoff_t	alloc_blocks = 0;
> > +		/*
> > +		 * If we don't have a user specified preallocation size, dynamically
> > +		 * increase the preallocation size as we do more preallocation.
> > +		 * Cap the maximum size at a single extent.
> > +		 */
> > +		if (!(mp->m_flags & XFS_MOUNT_DFLT_IOSIZE)) {
> 
> I note that this is circumventing the special code in
> xfs_set_rw_sizes() that tries to set up a different (smaller)
> size in the event the "sync" (generic) mount option was supplied
> (indicated by XFS_MOUNT_SYNC).  If that is a good thing, then I
> suggest the code in xfs_set_rw_sizes() go away.  But it would be
> good to have the case for making that change stated.

The new code based on file size is a bit different. It still
triggers off the absence of his flag, but it now uses the default
sizes as the minimum speculative allocation size.

> I guess the reason one might want the "allocsize" mount
> option now becomes the opposite of why one might have
> wanted it before.  I.e., it would be used to *reduce*
> the size of the preallocated range beyond EOF, which I
> could envision might be reasonable in some circumstances.

It now becomes the minimum preallocation size, rather than both the
minimum and the maximum....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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