Re: [PATCH] ext4: serialize unlocked dio reads with truncate

[Jan Kara <jack@suse.cz>]

On Mon 03-09-12 20:40:48, Dmitry Monakhov wrote:
> Current serialization will works only for DIO which holds
> i_mutex, but nonlocked DIO following race is possable:
> 
> dio_nolock_read_task            truncate_task
> 				->ext4_setattr()
> 				 ->inode_dio_wait()
> ->ext4_ext_direct_IO
>   ->ext4_ind_direct_IO
>     ->__blockdev_direct_IO
>       ->ext4_get_block
> 				 ->truncate_setsize()
> 				 ->ext4_truncate()
> 				 #alloc truncated blocks
> 				 #to other inode
>       ->submit_io()
>      #INFORMATION LEAK
  Right, that looks like a bug. Just isn't the "unlocked DIO" also
problematic because if we have enough aggressive readers, callers doing
inode_dio_wait() are waiting forever (I've just tried that with the value
of forever == several minutes).

  Also there is similar data exposure possible when direct IO read races
with block allocation, isn't it?

  Hum, and there seems to be also potential data corruption issue with
direct IO overwrites racing with truncate:
Like:
 dio write                      truncate_task
 ->ext4_ext_direct_IO
  ->overwrite == 1
   ->down_read(&EXT4_I(inode)->i_data_sem);
   ->mutex_unlock(&inode->i_mutex);
 				->ext4_setattr()
 				 ->inode_dio_wait()
 				 ->truncate_setsize()
 				 ->ext4_truncate()
				  ->down_write(&EXT4_I(inode)->i_data_sem);
   ->__blockdev_direct_IO
    ->ext4_get_block
    ->submit_io()
   ->up_read(&EXT4_I(inode)->i_data_sem);
 				  # truncate data blocks, allocate them to
 				  # other inode - bad stuff happens because
				  # dio is still in flight.

  Anyway your patch makes things better so I'm fine with it (feel free to
add Reviewed-by: Jan Kara <jack@suse.cz>). Just it seems direct IO locking
is rather broken in general...

								Honza

> In order to serialize with unlocked DIO reads we have to
> rearange wait sequance
  ^^^ rearrange  ^^^ sequence

> 1) update i_size first
> 2) wait for outstanding DIO requests
> 3) and only after that truncate inode blocks
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext4/inode.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d12d30e..ee534ab 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4304,8 +4304,6 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
>  	}
>  
>  	if (attr->ia_valid & ATTR_SIZE) {
> -		inode_dio_wait(inode);
> -
>  		if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
>  			struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>  
> @@ -4355,6 +4353,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
>  	if (attr->ia_valid & ATTR_SIZE) {
>  		if (attr->ia_size != i_size_read(inode))
>  			truncate_setsize(inode, attr->ia_size);
> +		inode_dio_wait(inode);
>  		ext4_truncate(inode);
>  	}
>  
> -- 
> 1.7.7.6
> 
> --
> 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 <jack@suse.cz>
SUSE Labs, CR