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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
