Patchwork [RFC] Insure direct IO writes do not use the page cache

login
register
mail settings
Submitter Curt Wohlgemuth
Date July 29, 2009, 12:28 a.m.
Message ID <6601abe90907281728h22be79fenc68a16b578e28a91@mail.gmail.com>
Download mbox | patch
Permalink /patch/30327/
State New
Headers show

Comments

Curt Wohlgemuth - July 29, 2009, 12:28 a.m.
This insures that direct IO writes to fallocate'd file space do not use the
page cache.


      Signed-off-by: Curt Wohlgemuth <curtw@google.com>

---

I implemented Aneesh's ideas for this solution, but have some questions.

I've verified that the page cache isn't  used, regardless of whether
FALLOC_FL_KEEP_SIZE is used in the fallocate() call or not.

The changes:
   - New inode state flag to indicate that a DIO write is ongoing

   - ext4_ext_convert_to_initialized() will mark any new extent it creates
     as uninitialized, if this new EXT4_STATE_DIO_WRITE flag is set.

   - ext4_direct_IO() will set this flag for any write.

     It now calls blockdev_direct_IO_own_locking() to do the I/O.

     After return from blockdev_direct_IO_own_locking() it clears the flag
     and calls a new routine to mark all extents containing the returned
     blocks as initialized.

I'm a bit uncertain about the use of DIO_OWN_LOCKING; I looked at the XFS
code to see if it acquired any other locks while using this flag, but didn't
see any.  Suggestions/corrections welcome.


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Curt Wohlgemuth - July 29, 2009, 4:10 p.m.
Although replying to self is somewhat bad etiquette...

I've found at least one issue with this patch:  Although the semantics
seem correct, since the late-converted-to-init extents are not merged
with neighbors, you can easily end up with thousands of extents :-( .
Each write to fallocate'd space results in its own initialized extent.

I'm not sure how expensive it would be to merge the extents when they
are converted to initialized after the DIO write goes through.

Curt


On Tue, Jul 28, 2009 at 5:28 PM, Curt Wohlgemuth<curtw@google.com> wrote:
> This insures that direct IO writes to fallocate'd file space do not use the
> page cache.
>
>
>      Signed-off-by: Curt Wohlgemuth <curtw@google.com>
>
> ---
>
> I implemented Aneesh's ideas for this solution, but have some questions.
>
> I've verified that the page cache isn't  used, regardless of whether
> FALLOC_FL_KEEP_SIZE is used in the fallocate() call or not.
>
> The changes:
>   - New inode state flag to indicate that a DIO write is ongoing
>
>   - ext4_ext_convert_to_initialized() will mark any new extent it creates
>     as uninitialized, if this new EXT4_STATE_DIO_WRITE flag is set.
>
>   - ext4_direct_IO() will set this flag for any write.
>
>     It now calls blockdev_direct_IO_own_locking() to do the I/O.
>
>     After return from blockdev_direct_IO_own_locking() it clears the flag
>     and calls a new routine to mark all extents containing the returned
>     blocks as initialized.
>
> I'm a bit uncertain about the use of DIO_OWN_LOCKING; I looked at the XFS
> code to see if it acquired any other locks while using this flag, but didn't
> see any.  Suggestions/corrections welcome.
>
>
> diff -Naur orig/fs/ext4/ext4.h new/fs/ext4/ext4.h
> --- orig/fs/ext4/ext4.h 2009-07-28 17:02:25.000000000 -0700
> +++ new/fs/ext4/ext4.h  2009-07-28 17:02:19.000000000 -0700
> @@ -272,6 +272,7 @@
>  #define EXT4_STATE_XATTR               0x00000004 /* has in-inode xattrs */
>  #define EXT4_STATE_NO_EXPAND           0x00000008 /* No space for expansion */
>  #define EXT4_STATE_DA_ALLOC_CLOSE      0x00000010 /* Alloc DA blks on close */
> +#define EXT4_STATE_DIO_WRITE           0x00000020 /* This is a DIO write */
>
>  /* Used to pass group descriptor data when online resize is done */
>  struct ext4_new_group_input {
> diff -Naur orig/fs/ext4/ext4_extents.h new/fs/ext4/ext4_extents.h
> --- orig/fs/ext4/ext4_extents.h 2009-07-28 17:02:40.000000000 -0700
> +++ new/fs/ext4/ext4_extents.h  2009-07-28 17:02:34.000000000 -0700
> @@ -242,5 +242,7 @@
>                                                ext4_lblk_t *, ext4_fsblk_t *);
>  extern void ext4_ext_drop_refs(struct ext4_ext_path *);
>  extern int ext4_ext_check_inode(struct inode *inode);
> +extern int ext4_ext_mark_extents_init(struct inode *inode, loff_t offset,
> +                                                       int nr_bytes);
>  #endif /* _EXT4_EXTENTS */
>
> diff -Naur orig/fs/ext4/extents.c new/fs/ext4/extents.c
> --- orig/fs/ext4/extents.c      2009-07-28 17:02:54.000000000 -0700
> +++ new/fs/ext4/extents.c       2009-07-28 17:02:49.000000000 -0700
> @@ -2563,6 +2563,13 @@
>                        ex3->ee_block = cpu_to_le32(iblock);
>                        ext4_ext_store_pblock(ex3, newblock);
>                        ex3->ee_len = cpu_to_le16(allocated);
> +
> +                       /* For direct I/O, we mark this as uninitialized, and
> +                        * set it as initialized when we finish the direct
> +                        * IO. */
> +                       if (EXT4_I(inode)->i_state & EXT4_STATE_DIO_WRITE)
> +                               ext4_ext_mark_uninitialized(ex3);
> +
>                        err = ext4_ext_insert_extent(handle, inode, path, ex3);
>                        if (err == -ENOSPC) {
>                                err =  ext4_ext_zeroout(inode, &orig_ex);
> @@ -2698,6 +2705,13 @@
>        ex2->ee_block = cpu_to_le32(iblock);
>        ext4_ext_store_pblock(ex2, newblock);
>        ex2->ee_len = cpu_to_le16(allocated);
> +
> +       /* For direct I/O, we mark this as uninitialized, and
> +        * set it as initialized when we finish the direct
> +        * IO. */
> +       if (EXT4_I(inode)->i_state & EXT4_STATE_DIO_WRITE)
> +               ext4_ext_mark_uninitialized(ex2);
> +
>        if (ex2 != ex)
>                goto insert;
>        /*
> @@ -2763,6 +2777,109 @@
>        return err;
>  }
>
> +
> +static int handle_uninit_extent(struct ext4_ext_path *path,
> +                               struct inode *inode,
> +                               struct ext4_extent *ex)
> +{
> +       handle_t *handle;
> +       int credits;
> +       int started;
> +       int depth = ext_depth(inode);
> +       int ret = 0;
> +
> +       if (ext4_ext_is_uninitialized(ex)) {
> +               handle = ext4_journal_current_handle();
> +               started = 0;
> +
> +               if (!handle) {
> +                       credits =
> +                            ext4_chunk_trans_blocks(inode, 1);
> +                       handle = ext4_journal_start(inode,
> +                                                   credits);
> +                       if (IS_ERR(handle)) {
> +                               ret = PTR_ERR(handle);
> +                               goto out;
> +                       }
> +                       started = 1;
> +               }
> +               /* Mark as initialized */
> +               ex->ee_len = cpu_to_le16(ext4_ext_get_actual_len(ex));
> +
> +               ret = ext4_ext_dirty(handle, inode, path + depth);
> +
> +               if (started)
> +                       ext4_journal_stop(handle);
> +       }
> +out:
> +       return ret;
> +}
> +
> +/* Called after direct I/O writes, to make sure that all extents are marked
> + * as initialized. */
> +int ext4_ext_mark_extents_init(struct inode *inode,
> +                               loff_t offset,
> +                               int nr_bytes)
> +{
> +       struct ext4_ext_path *path = NULL;
> +       struct ext4_extent *ex;
> +       int depth;
> +       int nr_blocks = nr_bytes >> inode->i_blkbits;
> +       ext4_lblk_t first_block = offset >> inode->i_blkbits;
> +       ext4_lblk_t block;
> +       int ret;
> +
> +       /* Handle the extent for the first block */
> +       path  = ext4_ext_find_extent(inode, first_block, NULL);
> +       if (IS_ERR(path)) {
> +               ret = PTR_ERR(path);
> +               path = NULL;
> +               goto out;
> +       }
> +       depth = ext_depth(inode);
> +       ex    = path[depth].p_ext;
> +
> +       ret = handle_uninit_extent(path, inode, ex);
> +       if (ret != 0) {
> +               goto out;
> +       }
> +
> +       /* Check the rest of the blocks; if they're in different
> +        * extents, handle those as well. */
> +       for (block = first_block + 1;
> +            block < first_block + nr_blocks;
> +            block++) {
> +
> +               /* Only look for new extents now */
> +               if (block >= ex->ee_block &&
> +                               block < (ex->ee_block + ex->ee_len))
> +                       continue;
> +
> +               ext4_ext_drop_refs(path);
> +               path  = ext4_ext_find_extent(inode, first_block, path);
> +               if (IS_ERR(path)) {
> +                       ret = PTR_ERR(path);
> +                       path = NULL;
> +                       goto out;
> +               }
> +               depth = ext_depth(inode);
> +               ex    = path[depth].p_ext;
> +
> +               ret = handle_uninit_extent(path, inode, ex);
> +               if (ret != 0) {
> +                       goto out;
> +               }
> +       }
> +out:
> +       if (path) {
> +               ext4_ext_drop_refs(path);
> +               kfree(path);
> +       }
> +
> +       return ret;
> +}
> +
> +
>  /*
>  * Block allocation/map/preallocation routine for extents based files
>  *
> diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
> --- orig/fs/ext4/inode.c        2009-07-28 17:02:04.000000000 -0700
> +++ new/fs/ext4/inode.c 2009-07-28 17:23:59.000000000 -0700
> @@ -3318,11 +3318,33 @@
>                        ei->i_disksize = inode->i_size;
>                        ext4_journal_stop(handle);
>                }
> -       }
> +               /* This prevents initializing extents too early */
> +               if (ei->i_flags & EXT4_EXTENTS_FL)
> +                       ei->i_state |= EXT4_STATE_DIO_WRITE;
> +       }
> +
> +       ret = blockdev_direct_IO_own_locking(rw, iocb, inode,
> +                                               inode->i_sb->s_bdev, iov,
> +                                               offset, nr_segs,
> +                                               ext4_get_block, NULL);
> +
> +       /* We now need to look at all extents for the blocks that were
> +        * written out, and mark them as initialized.  Note that they've
> +        * already been converted, simply not yet marked as init. */
> +       if (rw == WRITE && ei->i_flags & EXT4_EXTENTS_FL) {
> +               BUG_ON((ei->i_state & EXT4_STATE_DIO_WRITE) == 0);
> +               ei->i_state |= ~EXT4_STATE_DIO_WRITE;
> +
> +               if (ret > 0) {
> +                       int ret2;
>
> -       ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
> -                                offset, nr_segs,
> -                                ext4_get_block, NULL);
> +                       ret2 = ext4_ext_mark_extents_init(inode, offset, ret);
> +                       if (ret2 != 0) {
> +                               ret = ret2;
> +                               goto out;
> +                       }
> +               }
> +       }
>
>        if (orphan) {
>                int err;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen - July 29, 2009, 5:18 p.m.
Curt Wohlgemuth wrote:
> Although replying to self is somewhat bad etiquette...

nah :)

> I've found at least one issue with this patch:  Although the semantics
> seem correct, since the late-converted-to-init extents are not merged
> with neighbors, you can easily end up with thousands of extents :-( .
> Each write to fallocate'd space results in its own initialized extent.
> 
> I'm not sure how expensive it would be to merge the extents when they
> are converted to initialized after the DIO write goes through.
> 
> Curt
> 

hm I think I've seen other cases where things don't get merged as well
as I'd expect.

I haven't replied to the first mail yet because I have a lot of
remembering to do about xfs first, but I'm fairly certain that at least
your use of blockdev_direct_IO_own_locking() is not correct.  See for
example all the comments around __blockdev_direct_IO about i_mutex, and
all the xfs_ilock/xfs_iolock calls in xfs_read/xfs_write.

There is a lot of locking for the fs to handle if you want to go that route.

Also, IIRC xfs does the conversion to written (vs. unwritten) extents in
an IO completion handler, just FWIW.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen - July 29, 2009, 5:41 p.m.
Eric Sandeen wrote:

...

> I haven't replied to the first mail yet because I have a lot of
> remembering to do about xfs first, but I'm fairly certain that at least
> your use of blockdev_direct_IO_own_locking() is not correct.  See for
> example all the comments around __blockdev_direct_IO about i_mutex, and
> all the xfs_ilock/xfs_iolock calls in xfs_read/xfs_write.

Oh and i_alloc_sem too, forgot about that one :)

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mingming Cao - July 29, 2009, 5:47 p.m.
On Tue, 2009-07-28 at 17:28 -0700, Curt Wohlgemuth wrote:
> This insures that direct IO writes to fallocate'd file space do not use the
> page cache.
> 
> 
>       Signed-off-by: Curt Wohlgemuth <curtw@google.com>
> 
> ---
> 
> I implemented Aneesh's ideas for this solution, but have some questions.
> 
> I've verified that the page cache isn't  used, regardless of whether
> FALLOC_FL_KEEP_SIZE is used in the fallocate() call or not.
> 
> The changes:
>    - New inode state flag to indicate that a DIO write is ongoing
> 
>    - ext4_ext_convert_to_initialized() will mark any new extent it creates
>      as uninitialized, if this new EXT4_STATE_DIO_WRITE flag is set.
> 
>    - ext4_direct_IO() will set this flag for any write.
> 
>      It now calls blockdev_direct_IO_own_locking() to do the I/O.
> 
>      After return from blockdev_direct_IO_own_locking() it clears the flag
>      and calls a new routine to mark all extents containing the returned
>      blocks as initialized.
> 
> I'm a bit uncertain about the use of DIO_OWN_LOCKING; I looked at the XFS
> code to see if it acquired any other locks while using this flag, but didn't
> see any.  Suggestions/corrections welcome.
> 

I was coding up something different approach, but you beat me first:)

I looked at your patch briefly, I am not sure about using
DIO_OWN_LOCKING directly. For writes, that requires filesystem to handle
the race between the buffered IO and direct IO, assuming i_mutex is not
hold for write. But in ext4 case, since we use the generic routine, we
already hold the inode's i_mutex for write. Not sure if that would cause
any locking problem though...

XFS seems hold the i_mutex if it founds there are dirty pages in cache,
but in the case of no dirty pages, it will drop the lock completely.
during the DIO IO the lock is not hold at all.

Mingming
> 
> diff -Naur orig/fs/ext4/ext4.h new/fs/ext4/ext4.h
> --- orig/fs/ext4/ext4.h	2009-07-28 17:02:25.000000000 -0700
> +++ new/fs/ext4/ext4.h	2009-07-28 17:02:19.000000000 -0700
> @@ -272,6 +272,7 @@
>  #define EXT4_STATE_XATTR		0x00000004 /* has in-inode xattrs */
>  #define EXT4_STATE_NO_EXPAND		0x00000008 /* No space for expansion */
>  #define EXT4_STATE_DA_ALLOC_CLOSE	0x00000010 /* Alloc DA blks on close */
> +#define EXT4_STATE_DIO_WRITE		0x00000020 /* This is a DIO write */
> 
>  /* Used to pass group descriptor data when online resize is done */
>  struct ext4_new_group_input {
> diff -Naur orig/fs/ext4/ext4_extents.h new/fs/ext4/ext4_extents.h
> --- orig/fs/ext4/ext4_extents.h	2009-07-28 17:02:40.000000000 -0700
> +++ new/fs/ext4/ext4_extents.h	2009-07-28 17:02:34.000000000 -0700
> @@ -242,5 +242,7 @@
>  						ext4_lblk_t *, ext4_fsblk_t *);
>  extern void ext4_ext_drop_refs(struct ext4_ext_path *);
>  extern int ext4_ext_check_inode(struct inode *inode);
> +extern int ext4_ext_mark_extents_init(struct inode *inode, loff_t offset,
> +							int nr_bytes);
>  #endif /* _EXT4_EXTENTS */
> 
> diff -Naur orig/fs/ext4/extents.c new/fs/ext4/extents.c
> --- orig/fs/ext4/extents.c	2009-07-28 17:02:54.000000000 -0700
> +++ new/fs/ext4/extents.c	2009-07-28 17:02:49.000000000 -0700
> @@ -2563,6 +2563,13 @@
>  			ex3->ee_block = cpu_to_le32(iblock);
>  			ext4_ext_store_pblock(ex3, newblock);
>  			ex3->ee_len = cpu_to_le16(allocated);
> +
> +			/* For direct I/O, we mark this as uninitialized, and
> +			 * set it as initialized when we finish the direct
> +			 * IO. */
> +			if (EXT4_I(inode)->i_state & EXT4_STATE_DIO_WRITE)
> +				ext4_ext_mark_uninitialized(ex3);
> +
>  			err = ext4_ext_insert_extent(handle, inode, path, ex3);
>  			if (err == -ENOSPC) {
>  				err =  ext4_ext_zeroout(inode, &orig_ex);
> @@ -2698,6 +2705,13 @@
>  	ex2->ee_block = cpu_to_le32(iblock);
>  	ext4_ext_store_pblock(ex2, newblock);
>  	ex2->ee_len = cpu_to_le16(allocated);
> +
> +	/* For direct I/O, we mark this as uninitialized, and
> +	 * set it as initialized when we finish the direct
> +	 * IO. */
> +	if (EXT4_I(inode)->i_state & EXT4_STATE_DIO_WRITE)
> +		ext4_ext_mark_uninitialized(ex2);
> +
>  	if (ex2 != ex)
>  		goto insert;
>  	/*
> @@ -2763,6 +2777,109 @@
>  	return err;
>  }
> 
> +
> +static int handle_uninit_extent(struct ext4_ext_path *path,
> +				struct inode *inode,
> +				struct ext4_extent *ex)
> +{
> +	handle_t *handle;
> +	int credits;
> +	int started;
> +	int depth = ext_depth(inode);
> +	int ret = 0;
> +
> +	if (ext4_ext_is_uninitialized(ex)) {
> +		handle = ext4_journal_current_handle();
> +		started = 0;
> +
> +		if (!handle) {
> +			credits =
> +			     ext4_chunk_trans_blocks(inode, 1);
> +			handle = ext4_journal_start(inode,
> +						    credits);
> +			if (IS_ERR(handle)) {
> +				ret = PTR_ERR(handle);
> +				goto out;
> +			}
> +			started = 1;
> +		}
> +		/* Mark as initialized */
> +		ex->ee_len = cpu_to_le16(ext4_ext_get_actual_len(ex));
> +
> +		ret = ext4_ext_dirty(handle, inode, path + depth);
> +
> +		if (started)
> +			ext4_journal_stop(handle);
> +	}
> +out:
> +	return ret;
> +}
> +
> +/* Called after direct I/O writes, to make sure that all extents are marked
> + * as initialized. */
> +int ext4_ext_mark_extents_init(struct inode *inode,
> +				loff_t offset,
> +				int nr_bytes)
> +{
> +	struct ext4_ext_path *path = NULL;
> +	struct ext4_extent *ex;
> +	int depth;
> +	int nr_blocks = nr_bytes >> inode->i_blkbits;
> +	ext4_lblk_t first_block = offset >> inode->i_blkbits;
> +	ext4_lblk_t block;
> +	int ret;
> +
> +	/* Handle the extent for the first block */
> +	path  = ext4_ext_find_extent(inode, first_block, NULL);
> +	if (IS_ERR(path)) {
> +		ret = PTR_ERR(path);
> +		path = NULL;
> +		goto out;
> +	}
> +	depth = ext_depth(inode);
> +	ex    = path[depth].p_ext;
> +
> +	ret = handle_uninit_extent(path, inode, ex);
> +	if (ret != 0) {
> +		goto out;
> +	}
> +
> +	/* Check the rest of the blocks; if they're in different
> +	 * extents, handle those as well. */
> +	for (block = first_block + 1;
> +	     block < first_block + nr_blocks;
> +	     block++) {
> +
> +		/* Only look for new extents now */
> +		if (block >= ex->ee_block &&
> +				block < (ex->ee_block + ex->ee_len))
> +			continue;
> +
> +		ext4_ext_drop_refs(path);
> +		path  = ext4_ext_find_extent(inode, first_block, path);
> +		if (IS_ERR(path)) {
> +			ret = PTR_ERR(path);
> +			path = NULL;
> +			goto out;
> +		}
> +		depth = ext_depth(inode);
> +		ex    = path[depth].p_ext;
> +
> +		ret = handle_uninit_extent(path, inode, ex);
> +		if (ret != 0) {
> +			goto out;
> +		}
> +	}
> +out:
> +	if (path) {
> +		ext4_ext_drop_refs(path);
> +		kfree(path);
> +	}
> +
> +	return ret;
> +}
> +
> +
>  /*
>   * Block allocation/map/preallocation routine for extents based files
>   *
> diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
> --- orig/fs/ext4/inode.c	2009-07-28 17:02:04.000000000 -0700
> +++ new/fs/ext4/inode.c	2009-07-28 17:23:59.000000000 -0700
> @@ -3318,11 +3318,33 @@
>  			ei->i_disksize = inode->i_size;
>  			ext4_journal_stop(handle);
>  		}
> -	}
> +		/* This prevents initializing extents too early */
> +		if (ei->i_flags & EXT4_EXTENTS_FL)
> +			ei->i_state |= EXT4_STATE_DIO_WRITE;
> +	}
> +
> +	ret = blockdev_direct_IO_own_locking(rw, iocb, inode,
> +						inode->i_sb->s_bdev, iov,
> +						offset, nr_segs,
> +						ext4_get_block, NULL);
> +
> +	/* We now need to look at all extents for the blocks that were
> +	 * written out, and mark them as initialized.  Note that they've
> +	 * already been converted, simply not yet marked as init. */
> +	if (rw == WRITE && ei->i_flags & EXT4_EXTENTS_FL) {
> +		BUG_ON((ei->i_state & EXT4_STATE_DIO_WRITE) == 0);
> +		ei->i_state |= ~EXT4_STATE_DIO_WRITE;
> +
> +		if (ret > 0) {
> +			int ret2;
> 
> -	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
> -				 offset, nr_segs,
> -				 ext4_get_block, NULL);
> +			ret2 = ext4_ext_mark_extents_init(inode, offset, ret);
> +			if (ret2 != 0) {
> +				ret = ret2;
> +				goto out;
> +			}
> +		}
> +	}
> 
>  	if (orphan) {
>  		int err;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - July 29, 2009, 6:10 p.m.
On Tue, Jul 28, 2009 at 05:28:05PM -0700, Curt Wohlgemuth wrote:
> This insures that direct IO writes to fallocate'd file space do not use the
> page cache.

This isn't a full review, but I haven't found the time to write a
complete summary of all of the issues around direct I/O yet; but since
people are working on the solutions, I thought I'd raise a few issues
that I noticed while doing a quick read-through of the patch.  It's
not a full review, sorry --- ENOTIME.

One thing that we really need to do is in handle_uninit_extent(), but
a comment that it's only going to be used by at the end of direct I/O,
and then put in a call to ext4_handle_sync() to mark the handle as
synchronous.  That way, the DIO write won't return until the journal
transaction which commits the metadata operation is complete.

This is going to make the DIO write performance with a journal have
horrendous performance, but at least it will be correct.
(Specifically, DIO writes have the semantics that if the alignment
constraints are respected, the write is supposed to be synchronous ---
which means that if the write completes, and the system crashes
immediately afterwards, the data has to be accessible after the system
reboots.  If the extent tree change isn't committed, then the written
data won't be visible to userspace, so it might as well be lost.)  I
don't think we need to do anything special in the no journal case,
since by definition without a journal metadata updates aren't
guaranteed to be up to date.

I think I've only mentioned this on the weekly ext4 concall, so let me
fill in the optimizations I have in mind that should hopefully make
DIO less painful when writing into preallocated space. 

1) If extent tree block in question isn't already part of a journalled
transaction, and there is space in the extent block so we don't have
to split the extent tree node (which would require allocating a new
block), we can update the extent block in place, bypassing the
journal.  This will allow us to avoid waiting for the journal commit.

2) We can modify the ext4_ext_convert_to_initialized() to be more
aggressive about initializing data blocks if we know we are doing DIO,
since zero'ing an aligned 16 to 32 blocks and then waiting for the
journal commit once is cheaper than converting the extent one block at
a time and waiting for the journal commit after each block write.

Does that make sense?

       	   	       	   	   	  - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen - July 29, 2009, 7:48 p.m.
Eric Sandeen wrote:
...

> Also, IIRC xfs does the conversion to written (vs. unwritten) extents in
> an IO completion handler, just FWIW.

After talking w/ Jeff Moyer, realized that w/o using the io completion,
there's a race with AIO.

Userspace will get notified that the write is done before the extents
get flipped to initialized ....

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mingming Cao - July 29, 2009, 10:17 p.m.
On Wed, 2009-07-29 at 14:48 -0500, Eric Sandeen wrote:
> Eric Sandeen wrote:
> ...
> 
> > Also, IIRC xfs does the conversion to written (vs. unwritten) extents in
> > an IO completion handler, just FWIW.
> 
> After talking w/ Jeff Moyer, realized that w/o using the io completion,
> there's a race with AIO.
> 
> Userspace will get notified that the write is done before the extents
> get flipped to initialized ....

Yes, for the AIO case, the DIO submit the IO and returns without waiting
for IO to complete. If we do conversion in ext4_direct_IO, then we could
convert the extents before the real IO hit to disk. Could cause stale
data exposed if crash before the data written to disk.
> 
> -Eric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aneesh Kumar K.V - July 30, 2009, 11:06 a.m.
On Tue, Jul 28, 2009 at 05:28:05PM -0700, Curt Wohlgemuth wrote:
> This insures that direct IO writes to fallocate'd file space do not use the
> page cache.
> 
> 
>       Signed-off-by: Curt Wohlgemuth <curtw@google.com>
> 
> ---
> 
> I implemented Aneesh's ideas for this solution, but have some questions.
> 
> I've verified that the page cache isn't  used, regardless of whether
> FALLOC_FL_KEEP_SIZE is used in the fallocate() call or not.
> 
> The changes:
>    - New inode state flag to indicate that a DIO write is ongoing
> 
>    - ext4_ext_convert_to_initialized() will mark any new extent it creates
>      as uninitialized, if this new EXT4_STATE_DIO_WRITE flag is set.

That will have issues with a parallel get_block due to writepages. ext4_da_writepages
will end up calling get_block on uninit extent without holding inode->i_mutex. So
you can have a direct_IO -> get_block and a writepages -> get_block going together
now if we mark extent as uninit based on a inode flag we will have to fix the
writepages call path also. Instead of inode flag you may want to track the extent
(you can look at the patch from Chris Mason implementing data=guarded for ext3. Chris
Mason's patch track buffer_head what you want is to track extent. And convert the
extent to init using end_io call back. 


> 
>    - ext4_direct_IO() will set this flag for any write.
> 
>      It now calls blockdev_direct_IO_own_locking() to do the I/O.
> 
>      After return from blockdev_direct_IO_own_locking() it clears the flag
>      and calls a new routine to mark all extents containing the returned
>      blocks as initialized.
> 
> I'm a bit uncertain about the use of DIO_OWN_LOCKING; I looked at the XFS
> code to see if it acquired any other locks while using this flag, but didn't
> see any.  Suggestions/corrections welcome.

-aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara - July 30, 2009, 6:30 p.m.
> On Tue, Jul 28, 2009 at 05:28:05PM -0700, Curt Wohlgemuth wrote:
> > This insures that direct IO writes to fallocate'd file space do not use the
> > page cache.
> 
> This isn't a full review, but I haven't found the time to write a
> complete summary of all of the issues around direct I/O yet; but since
> people are working on the solutions, I thought I'd raise a few issues
> that I noticed while doing a quick read-through of the patch.  It's
> not a full review, sorry --- ENOTIME.
> 
> One thing that we really need to do is in handle_uninit_extent(), but
> a comment that it's only going to be used by at the end of direct I/O,
> and then put in a call to ext4_handle_sync() to mark the handle as
> synchronous.  That way, the DIO write won't return until the journal
> transaction which commits the metadata operation is complete.
> 
> This is going to make the DIO write performance with a journal have
> horrendous performance, but at least it will be correct.
> (Specifically, DIO writes have the semantics that if the alignment
> constraints are respected, the write is supposed to be synchronous ---
> which means that if the write completes, and the system crashes
> immediately afterwards, the data has to be accessible after the system
> reboots.  If the extent tree change isn't committed, then the written
> data won't be visible to userspace, so it might as well be lost.)  I
> don't think we need to do anything special in the no journal case,
> since by definition without a journal metadata updates aren't
> guaranteed to be up to date.
> 
> I think I've only mentioned this on the weekly ext4 concall, so let me
> fill in the optimizations I have in mind that should hopefully make
> DIO less painful when writing into preallocated space. 
> 
> 1) If extent tree block in question isn't already part of a journalled
> transaction, and there is space in the extent block so we don't have
> to split the extent tree node (which would require allocating a new
> block), we can update the extent block in place, bypassing the
> journal.  This will allow us to avoid waiting for the journal commit.
  I have to say I'm a bit worried about modify-in-place tricks - it's
not trivial to make sure buffer is not part of any transaction in the
journal, since the buffer head could have been evicted from memory, but
the transaction still is not fully checkpointed. Hence in memory, you
don't have any evidence of the fact that if the machine crashes, your
modify-in-place gets overwritten by journal-replay.

> 2) We can modify the ext4_ext_convert_to_initialized() to be more
> aggressive about initializing data blocks if we know we are doing DIO,
> since zero'ing an aligned 16 to 32 blocks and then waiting for the
> journal commit once is cheaper than converting the extent one block at
> a time and waiting for the journal commit after each block write.
  Definitely. I'm not following the discussion too much in detail but
what seems to me is the following could work:
  The direct IO path would first send all the data to disk to the
desired location (get_block wouldn't do any conversion, just map blocks).
When this is done, we convert all the touched extents to initialized ones
from ext4_direct_IO, update i_size if needed, and wait for transaction
commit.

								Honza
Eric Sandeen - July 30, 2009, 6:39 p.m.
Jan Kara wrote:
>> On Tue, Jul 28, 2009 at 05:28:05PM -0700, Curt Wohlgemuth wrote:

...

>> 2) We can modify the ext4_ext_convert_to_initialized() to be more
>> aggressive about initializing data blocks if we know we are doing DIO,
>> since zero'ing an aligned 16 to 32 blocks and then waiting for the
>> journal commit once is cheaper than converting the extent one block at
>> a time and waiting for the journal commit after each block write.
>   Definitely. I'm not following the discussion too much in detail but
> what seems to me is the following could work:
>   The direct IO path would first send all the data to disk to the
> desired location (get_block wouldn't do any conversion, just map blocks).
> When this is done, we convert all the touched extents to initialized ones
> from ext4_direct_IO, update i_size if needed, and wait for transaction
> commit.
> 
> 								Honza

This is all about right, but it's tricky, because right now, get_block
is called in the direct IO path from get_more_blocks(), and it's called
with create == 0 unless OWN_LOCKING is specified.  If we do get_block w/
create == 0 and find prealloc'd blocks, then we're given back unmapped
buffer heads. This looks like a hole, and so DIO falls back to buffered.

Right now the only way to get create == 1 sent to get_blocks via
directio is to do OWN_LOCKING, which implies... we have to do our own
locking, and it'll take some time to get it right I think.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara - July 30, 2009, 6:44 p.m.
On Thu 30-07-09 13:39:12, Eric Sandeen wrote:
> Jan Kara wrote:
> >> On Tue, Jul 28, 2009 at 05:28:05PM -0700, Curt Wohlgemuth wrote:
> 
> ...
> 
> >> 2) We can modify the ext4_ext_convert_to_initialized() to be more
> >> aggressive about initializing data blocks if we know we are doing DIO,
> >> since zero'ing an aligned 16 to 32 blocks and then waiting for the
> >> journal commit once is cheaper than converting the extent one block at
> >> a time and waiting for the journal commit after each block write.
> >   Definitely. I'm not following the discussion too much in detail but
> > what seems to me is the following could work:
> >   The direct IO path would first send all the data to disk to the
> > desired location (get_block wouldn't do any conversion, just map blocks).
> > When this is done, we convert all the touched extents to initialized ones
> > from ext4_direct_IO, update i_size if needed, and wait for transaction
> > commit.
> > 
> > 								Honza
> 
> This is all about right, but it's tricky, because right now, get_block
> is called in the direct IO path from get_more_blocks(), and it's called
> with create == 0 unless OWN_LOCKING is specified.  If we do get_block w/
> create == 0 and find prealloc'd blocks, then we're given back unmapped
> buffer heads. This looks like a hole, and so DIO falls back to buffered.
>
> Right now the only way to get create == 1 sent to get_blocks via
> directio is to do OWN_LOCKING, which implies... we have to do our own
> locking, and it'll take some time to get it right I think.
  But the get_block function called by get_more_blocks() is specified in
ext4_direct_IO. So we can provide it with a special direct_IO version of
get_block function which happily maps also uninitialized extents... It's a
slight hack, but maintainable IMHO.

									Honza
Eric Sandeen - July 30, 2009, 7:16 p.m.
Jan Kara wrote:
> On Thu 30-07-09 13:39:12, Eric Sandeen wrote:
...

							Honza
>> This is all about right, but it's tricky, because right now, get_block
>> is called in the direct IO path from get_more_blocks(), and it's called
>> with create == 0 unless OWN_LOCKING is specified.  If we do get_block w/
>> create == 0 and find prealloc'd blocks, then we're given back unmapped
>> buffer heads. This looks like a hole, and so DIO falls back to buffered.
>>
>> Right now the only way to get create == 1 sent to get_blocks via
>> directio is to do OWN_LOCKING, which implies... we have to do our own
>> locking, and it'll take some time to get it right I think.
>   But the get_block function called by get_more_blocks() is specified in
> ext4_direct_IO. So we can provide it with a special direct_IO version of
> get_block function which happily maps also uninitialized extents... It's a
> slight hack, but maintainable IMHO.

Hm, perhaps.  xfs does have a xfs_get_blocks_direct() that it uses for DIO.

Although returning mapped unwritten blocks ... well, we'll have to be
sure it doesn't cause any problems elsewhere.  :)

-Eric

> 									Honza

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - July 30, 2009, 8:33 p.m.
On Thu, Jul 30, 2009 at 08:30:53PM +0200, Jan Kara wrote:
>   I have to say I'm a bit worried about modify-in-place tricks - it's
> not trivial to make sure buffer is not part of any transaction in the
> journal, since the buffer head could have been evicted from memory, but
> the transaction still is not fully checkpointed. Hence in memory, you
> don't have any evidence of the fact that if the machine crashes, your
> modify-in-place gets overwritten by journal-replay.

Yeah, good point; tracking which blocks might get overwritten on a
journal replay is tough.  What we *could* do that would make this easier
is to insert a revoke record for all extent tree blocks after the
blocks have been written to disk (since at that point there's no need
for that block to be replayed).

Whether or not this optimization is worth it largely depends on time
between how many blocks are getting allocated using fallocate(), and
what the average number of blocks are that get written at a time by
the application (normally enterprise databases) when write into the
unitialized area.  If the average size is say, 32k, and the amount of
space they allocate is say, 32 megs, then without doing any special
DIO optimization, on average we will end up having to do 1024
synchronous waits on a journal commit.  If the database doesn't use
any fallocates at all, then it will have to do a 32 meg write to
initialize the area, followed by 32 megs of data writes, written
randomly 32k at a time.

So being aggressive with pre-zeroing extra datablocks when we convert
uninit extents to initialized extents mean that we still have to do
some percentage of zero'izing data writes combined with the extra
journal traffic, so it's likely we haven't reduced the total disk
bandwidth by much, and the latency improvements of not having to do
the 32meg zero writes gets offset with the data=ordered latency hits
when we do the journal commit.

So it would seem to me that if we really want to get the full benefit
of preallocation in the DIO case, we really do need to think about
seeing if it's possible bypass the journal. 

It may be useful here to write a benchmark that simulates the behavior
of an eneterprise database using fallocate, so we can see what the
performance hit is of making sure we don't lose data on a crash, and
then how much of that performance hit we can claw back with various
optimizations.

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Curt Wohlgemuth - July 31, 2009, 4:10 p.m.
Thanks to all for ideas and corrections for my original patch.  I'd like to
summarize the issues that I've seen raised:

1. Using blockdev_direct_IO_own_locking() as it stands, without additional
   locking in the ext4 code, is incorrect.

2. The conversion from uninit to initialized extents should be done in an IO
   completion handler.

3. When the uninit-to-init extents are converted, the handle must be marked
   as synchronous.

   But this will make DIO writes (to fallocated space) with a journal have
   bad performance.

4. Ted mentioned some optimizations possible for extent conversion (when the
   extent block isn't part of a transaction, and no new block is required).
   Jan says that verifying that the extent block is not part of a
   transaction can be difficult.

   Also we could increase the extent size that we're willing to zero out the
   data blocks for.

5. Aneesh mentioned that we could use extent tracking a la Chris Mason's
   patch for data=guarded (I confess, I haven't looked at this yet).

6. Jan's other thought is to use a new ext4_get_blocks_direct() routine as
   the get_block callback to blockdev_direct_IO() -- so no use of
   _own_locking().  This would simply return blocks from uninit extents;
   extent conversion (including possible splitting) would then be done in
   ext4_direct_IO().

7. Ted's last comment is about the tradeoffs between getting the journal
   transaction correct vs aggressive zeroout of data blocks -- seeing if
   it's possible to bypass the journal in the case of preallocated DIO
   writes.

Looking through these, it seems to me that there are two major problems:

   a. How to correctly do extent conversion in the face of locking issues and
      races with other requests (e.g. AIO)

   b. How to efficiently do this extent conversion in the face of correct
      journal semantics.

Have I missed anything?

Jan's idea of a new get_block callback for DIO seems like the simplest
solution to (a) above.  No locking changes would seem to be needed, I think.
Does this seem reasonable?

Problem (b) is one that I would defer to others with more experience with
journals than I have.

Thanks,
Curt
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mingming Cao - July 31, 2009, 5:58 p.m.
On Thu, 2009-07-30 at 16:33 -0400, Theodore Tso wrote:
> On Thu, Jul 30, 2009 at 08:30:53PM +0200, Jan Kara wrote:
> >   I have to say I'm a bit worried about modify-in-place tricks - it's
> > not trivial to make sure buffer is not part of any transaction in the
> > journal, since the buffer head could have been evicted from memory, but
> > the transaction still is not fully checkpointed. Hence in memory, you
> > don't have any evidence of the fact that if the machine crashes, your
> > modify-in-place gets overwritten by journal-replay.
> 
> Yeah, good point; tracking which blocks might get overwritten on a
> journal replay is tough.  What we *could* do that would make this easier
> is to insert a revoke record for all extent tree blocks after the
> blocks have been written to disk (since at that point there's no need
> for that block to be replayed).
> 
> Whether or not this optimization is worth it largely depends on time
> between how many blocks are getting allocated using fallocate(), and
> what the average number of blocks are that get written at a time by
> the application (normally enterprise databases) when write into the
> unitialized area.  If the average size is say, 32k, and the amount of
> space they allocate is say, 32 megs, then without doing any special
> DIO optimization, on average we will end up having to do 1024
> synchronous waits on a journal commit.  If the database doesn't use
> any fallocates at all, then it will have to do a 32 meg write to
> initialize the area, followed by 32 megs of data writes, written
> randomly 32k at a time.
> 
> So being aggressive with pre-zeroing extra datablocks when we convert
> uninit extents to initialized extents mean that we still have to do
> some percentage of zero'izing data writes combined with the extra
> journal traffic, so it's likely we haven't reduced the total disk
> bandwidth by much, and the latency improvements of not having to do
> the 32meg zero writes gets offset with the data=ordered latency hits
> when we do the journal commit.
> 
> So it would seem to me that if we really want to get the full benefit
> of preallocation in the DIO case, we really do need to think about
> seeing if it's possible bypass the journal. 
> 
> It may be useful here to write a benchmark that simulates the behavior
> of an eneterprise database using fallocate, so we can see what the
> performance hit is of making sure we don't lose data on a crash, and
> then how much of that performance hit we can claw back with various
> optimizations.
> 

Eric and I looked at xfs code together the other day, xfs code did not
ensure DIO sync metadata (conversion) before return back to userspace.
It does ensure the workqueue kickoff the conversion and journal commit,
but it seems not waiting for it to complete. This seems confirmed by xfs
expert on IRC, who expressed DIO means only bypass page cache, but not
necessarily means sync on data and metadata unless file is opened with
SYNC mode. 


Mingming


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Rubin - July 31, 2009, 6:03 p.m.
On Fri, Jul 31, 2009 at 10:58 AM, Mingming<cmm@us.ibm.com> wrote:
> Eric and I looked at xfs code together the other day, xfs code did not
> ensure DIO sync metadata (conversion) before return back to userspace.
> It does ensure the workqueue kickoff the conversion and journal commit,
> but it seems not waiting for it to complete. This seems confirmed by xfs
> expert on IRC, who expressed DIO means only bypass page cache, but not
> necessarily means sync on data and metadata unless file is opened with
> SYNC mode.

This will our expectations for DIO on ext4 also.

mrubin
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Rubin - July 31, 2009, 6:03 p.m.
On Fri, Jul 31, 2009 at 11:03 AM, Michael Rubin<mrubin@google.com> wrote:
> This will our expectations for DIO on ext4 also.

<sigh>

I meant "This will fulfill our expectations for DIO on ext4 also."

mrubin
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara - Aug. 3, 2009, 9:36 a.m.
On Thu 30-07-09 16:33:51, Theodore Tso wrote:
> On Thu, Jul 30, 2009 at 08:30:53PM +0200, Jan Kara wrote:
> >   I have to say I'm a bit worried about modify-in-place tricks - it's
> > not trivial to make sure buffer is not part of any transaction in the
> > journal, since the buffer head could have been evicted from memory, but
> > the transaction still is not fully checkpointed. Hence in memory, you
> > don't have any evidence of the fact that if the machine crashes, your
> > modify-in-place gets overwritten by journal-replay.
> 
> Yeah, good point; tracking which blocks might get overwritten on a
> journal replay is tough.  What we *could* do that would make this easier
> is to insert a revoke record for all extent tree blocks after the
> blocks have been written to disk (since at that point there's no need
> for that block to be replayed).
  Hmm, but will this help you? You'd have to wait for revoke records to
commit before you can be sure that journal replay won't overwrite your
in-place changes.
  Looking at the O_DIRECT semantics, I don't think nobody really requires
the data being on disk after the write() returns and we crash - in
particular if we extend the file, the write will be just an ordinary
buffered write so in practice, it behaves like this already. Given the fact
that only a bit special applications use O_DIRECT, I think we can afford to
make a reservation that O_DIRECT writes even to a preallocated space do not
have any special data-consistency guarantees.

									Honza

Patch

diff -Naur orig/fs/ext4/ext4.h new/fs/ext4/ext4.h
--- orig/fs/ext4/ext4.h	2009-07-28 17:02:25.000000000 -0700
+++ new/fs/ext4/ext4.h	2009-07-28 17:02:19.000000000 -0700
@@ -272,6 +272,7 @@ 
 #define EXT4_STATE_XATTR		0x00000004 /* has in-inode xattrs */
 #define EXT4_STATE_NO_EXPAND		0x00000008 /* No space for expansion */
 #define EXT4_STATE_DA_ALLOC_CLOSE	0x00000010 /* Alloc DA blks on close */
+#define EXT4_STATE_DIO_WRITE		0x00000020 /* This is a DIO write */

 /* Used to pass group descriptor data when online resize is done */
 struct ext4_new_group_input {
diff -Naur orig/fs/ext4/ext4_extents.h new/fs/ext4/ext4_extents.h
--- orig/fs/ext4/ext4_extents.h	2009-07-28 17:02:40.000000000 -0700
+++ new/fs/ext4/ext4_extents.h	2009-07-28 17:02:34.000000000 -0700
@@ -242,5 +242,7 @@ 
 						ext4_lblk_t *, ext4_fsblk_t *);
 extern void ext4_ext_drop_refs(struct ext4_ext_path *);
 extern int ext4_ext_check_inode(struct inode *inode);
+extern int ext4_ext_mark_extents_init(struct inode *inode, loff_t offset,
+							int nr_bytes);
 #endif /* _EXT4_EXTENTS */

diff -Naur orig/fs/ext4/extents.c new/fs/ext4/extents.c
--- orig/fs/ext4/extents.c	2009-07-28 17:02:54.000000000 -0700
+++ new/fs/ext4/extents.c	2009-07-28 17:02:49.000000000 -0700
@@ -2563,6 +2563,13 @@ 
 			ex3->ee_block = cpu_to_le32(iblock);
 			ext4_ext_store_pblock(ex3, newblock);
 			ex3->ee_len = cpu_to_le16(allocated);
+
+			/* For direct I/O, we mark this as uninitialized, and
+			 * set it as initialized when we finish the direct
+			 * IO. */
+			if (EXT4_I(inode)->i_state & EXT4_STATE_DIO_WRITE)
+				ext4_ext_mark_uninitialized(ex3);
+
 			err = ext4_ext_insert_extent(handle, inode, path, ex3);
 			if (err == -ENOSPC) {
 				err =  ext4_ext_zeroout(inode, &orig_ex);
@@ -2698,6 +2705,13 @@ 
 	ex2->ee_block = cpu_to_le32(iblock);
 	ext4_ext_store_pblock(ex2, newblock);
 	ex2->ee_len = cpu_to_le16(allocated);
+
+	/* For direct I/O, we mark this as uninitialized, and
+	 * set it as initialized when we finish the direct
+	 * IO. */
+	if (EXT4_I(inode)->i_state & EXT4_STATE_DIO_WRITE)
+		ext4_ext_mark_uninitialized(ex2);
+
 	if (ex2 != ex)
 		goto insert;
 	/*
@@ -2763,6 +2777,109 @@ 
 	return err;
 }

+
+static int handle_uninit_extent(struct ext4_ext_path *path,
+				struct inode *inode,
+				struct ext4_extent *ex)
+{
+	handle_t *handle;
+	int credits;
+	int started;
+	int depth = ext_depth(inode);
+	int ret = 0;
+
+	if (ext4_ext_is_uninitialized(ex)) {
+		handle = ext4_journal_current_handle();
+		started = 0;
+
+		if (!handle) {
+			credits =
+			     ext4_chunk_trans_blocks(inode, 1);
+			handle = ext4_journal_start(inode,
+						    credits);
+			if (IS_ERR(handle)) {
+				ret = PTR_ERR(handle);
+				goto out;
+			}
+			started = 1;
+		}
+		/* Mark as initialized */
+		ex->ee_len = cpu_to_le16(ext4_ext_get_actual_len(ex));
+
+		ret = ext4_ext_dirty(handle, inode, path + depth);
+
+		if (started)
+			ext4_journal_stop(handle);
+	}
+out:
+	return ret;
+}
+
+/* Called after direct I/O writes, to make sure that all extents are marked
+ * as initialized. */
+int ext4_ext_mark_extents_init(struct inode *inode,
+				loff_t offset,
+				int nr_bytes)
+{
+	struct ext4_ext_path *path = NULL;
+	struct ext4_extent *ex;
+	int depth;
+	int nr_blocks = nr_bytes >> inode->i_blkbits;
+	ext4_lblk_t first_block = offset >> inode->i_blkbits;
+	ext4_lblk_t block;
+	int ret;
+
+	/* Handle the extent for the first block */
+	path  = ext4_ext_find_extent(inode, first_block, NULL);
+	if (IS_ERR(path)) {
+		ret = PTR_ERR(path);
+		path = NULL;
+		goto out;
+	}
+	depth = ext_depth(inode);
+	ex    = path[depth].p_ext;
+
+	ret = handle_uninit_extent(path, inode, ex);
+	if (ret != 0) {
+		goto out;
+	}
+
+	/* Check the rest of the blocks; if they're in different
+	 * extents, handle those as well. */
+	for (block = first_block + 1;
+	     block < first_block + nr_blocks;
+	     block++) {
+
+		/* Only look for new extents now */
+		if (block >= ex->ee_block &&
+				block < (ex->ee_block + ex->ee_len))
+			continue;
+
+		ext4_ext_drop_refs(path);
+		path  = ext4_ext_find_extent(inode, first_block, path);
+		if (IS_ERR(path)) {
+			ret = PTR_ERR(path);
+			path = NULL;
+			goto out;
+		}
+		depth = ext_depth(inode);
+		ex    = path[depth].p_ext;
+
+		ret = handle_uninit_extent(path, inode, ex);
+		if (ret != 0) {
+			goto out;
+		}
+	}
+out:
+	if (path) {
+		ext4_ext_drop_refs(path);
+		kfree(path);
+	}
+
+	return ret;
+}
+
+
 /*
  * Block allocation/map/preallocation routine for extents based files
  *
diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
--- orig/fs/ext4/inode.c	2009-07-28 17:02:04.000000000 -0700
+++ new/fs/ext4/inode.c	2009-07-28 17:23:59.000000000 -0700
@@ -3318,11 +3318,33 @@ 
 			ei->i_disksize = inode->i_size;
 			ext4_journal_stop(handle);
 		}
-	}
+		/* This prevents initializing extents too early */
+		if (ei->i_flags & EXT4_EXTENTS_FL)
+			ei->i_state |= EXT4_STATE_DIO_WRITE;
+	}
+
+	ret = blockdev_direct_IO_own_locking(rw, iocb, inode,
+						inode->i_sb->s_bdev, iov,
+						offset, nr_segs,
+						ext4_get_block, NULL);
+
+	/* We now need to look at all extents for the blocks that were
+	 * written out, and mark them as initialized.  Note that they've
+	 * already been converted, simply not yet marked as init. */
+	if (rw == WRITE && ei->i_flags & EXT4_EXTENTS_FL) {
+		BUG_ON((ei->i_state & EXT4_STATE_DIO_WRITE) == 0);
+		ei->i_state |= ~EXT4_STATE_DIO_WRITE;
+
+		if (ret > 0) {
+			int ret2;

-	ret = blockdev_direct_IO(rw, iocb, inode, inode->i_sb->s_bdev, iov,
-				 offset, nr_segs,
-				 ext4_get_block, NULL);
+			ret2 = ext4_ext_mark_extents_init(inode, offset, ret);
+			if (ret2 != 0) {
+				ret = ret2;
+				goto out;
+			}
+		}
+	}

 	if (orphan) {
 		int err;