Re: [RFC PATCH] ext4: Fix fallocate to update the file size in each transaction.

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

On Fri, Mar 07, 2008 at 09:45:06AM -0600, Eric Sandeen wrote:
> Aneesh Kumar K.V wrote:
> > ext4_fallocate need to update file size in each transaction. Otherwise
> > ife we crash the file size won't be updated. We were also not marking
> > the inode dirty after updating file size before. 
> 
> This led to losing the size update when unmounted...
> 
> > Also when we try to
> > retry allocation due to ENOSPC make sure we reset the variable ret so
> > that we actually do a retry.
> 
> That's a few alsos.  Should this be more than one patch when committed?
> 
> -Eric
> 
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > ---
> >  fs/ext4/extents.c |   78 +++++++++++++++++++++--------------------------------
> >  1 files changed, 31 insertions(+), 47 deletions(-)
> > 
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index dcdf92a..09dd3c5 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -2783,6 +2783,26 @@ int ext4_ext_writepage_trans_blocks(struct inode *inode, int num)
> >  	return needed;
> >  }
> >  
> > +static void ext4_falloc_update_inode(struct inode *inode,
> > +					int mode, loff_t new_size)
> > +{
> > +	struct timespec now;
> > +
> > +	now = current_fs_time(inode->i_sb);
> > +	if (!timespec_equal(&inode->i_ctime, &now))
> > +		inode->i_ctime = now;
> 
> This used to depend on blocks actually being allocated; it looks like it
> doesn't anymore?

I am still not clear about the requirement here. The earlier code
check for greater than current file size, which was confusing because we
could very well convert a hole to a prealloc area. How about updating
i_ctime if the buffer head is new ?.


> 
> > +	/*
> > +	 * Update only when preallocation was requested beyond
> > +	 * the file size.
> > +	 */
> > +	if (!(mode & FALLOC_FL_KEEP_SIZE) &&
> >  			break;

.....

> >  		}
> > -		if (ret > 0) {
> > -			/* check wrap through sign-bit/zero here */
> > -			if ((block + ret) < 0 || (block + ret) < block) {
> > -				ret = -EIO;
> > -				ext4_mark_inode_dirty(handle, inode);
> > -				ret2 = ext4_journal_stop(handle);
> > -				break;
> > -			}
> > -			if (buffer_new(&map_bh) && ((block + ret) >
> > -			    (EXT4_BLOCK_ALIGN(i_size_read(inode), blkbits)
> > -			    >> blkbits)))
> > -					nblocks = nblocks + ret;
> > -		}
> > +		if ((block + ret) >= (EXT4_BLOCK_ALIGN(offset + len,
> > +						blkbits) >> blkbits))
> > +			new_size = offset + len;
> > +		else
> > +			new_size = (block + ret) << blkbits;
> 
> Since we called ext4_get_blocks_wrap with max_blocks, will we really end
> up with more blocks allocated than we asked for?  And do we no longer
> need the wrap checks?  (I'd expect that to be tested higher up with the
> original offset+len requested, no?)

ext4_get_blocks_wrap had confusing returns. Mingming actually fixed it
in the previous patch. We can actually return more bytes than requested
if we are calling ext4_get_blocks_wrap in read mode for an falloc area.
Well that is not really the case here. But having in >=  is ok. 


The wrap check there was not needed. The needed wrap check is to make sure
whether we are crossing the allowed max file size. That is done in
sys_fallocate.



> 
> > -		/* Update ctime if new blocks get allocated */
> > -		if (nblocks) {
> > -			struct timespec now;
> > -
> > -			now = current_fs_time(inode->i_sb);

...

-aneesh