lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <alpine.LFD.2.00.1212031136040.6336@localhost>
Date:	Mon, 3 Dec 2012 11:36:36 +0100 (CET)
From:	Lukáš Czerner <lczerner@...hat.com>
To:	"Theodore Ts'o" <tytso@....edu>
cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: restructure ext4_ext_direct_IO()

On Thu, 29 Nov 2012, Theodore Ts'o wrote:

> Date: Thu, 29 Nov 2012 21:17:06 -0500
> From: Theodore Ts'o <tytso@....edu>
> To: Ext4 Developers List <linux-ext4@...r.kernel.org>
> Cc: Theodore Ts'o <tytso@....edu>
> Subject: [PATCH] ext4: restructure ext4_ext_direct_IO()
> 
> Remove a level of indentation by moving the DIO read and extending
> write case to the beginning of the file.  This results in no actual
> programmatic changes to the file, but makes it easier to
> read/understand.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@....edu>

Looks good.

Reviewed-by: Lukas Czerner <lczerner@...hat.com>

Thanks!
-Lukas

> ---
>  fs/ext4/inode.c | 211 +++++++++++++++++++++++++++-----------------------------
>  1 file changed, 103 insertions(+), 108 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index cf5d30a..91a2496 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2927,10 +2927,10 @@ retry:
>   * fall back to buffered IO.
>   *
>   * For holes, we fallocate those blocks, mark them as uninitialized
> - * If those blocks were preallocated, we mark sure they are splited, but
> + * If those blocks were preallocated, we mark sure they are split, but
>   * still keep the range to write as uninitialized.
>   *
> - * The unwrritten extents will be converted to written when DIO is completed.
> + * The unwritten extents will be converted to written when DIO is completed.
>   * For async direct IO, since the IO may still pending when return, we
>   * set up an end_io call back function, which will do the conversion
>   * when async direct IO completed.
> @@ -2948,125 +2948,120 @@ static ssize_t ext4_ext_direct_IO(int rw, struct kiocb *iocb,
>  	struct inode *inode = file->f_mapping->host;
>  	ssize_t ret;
>  	size_t count = iov_length(iov, nr_segs);
> -
> +	int overwrite = 0;
> +	get_block_t *get_block_func = NULL;
> +	int dio_flags = 0;
>  	loff_t final_size = offset + count;
> -	if (rw == WRITE && final_size <= inode->i_size) {
> -		int overwrite = 0;
> -		get_block_t *get_block_func = NULL;
> -		int dio_flags = 0;
>  
> -		BUG_ON(iocb->private == NULL);
> +	/* Use the old path for reads and writes beyond i_size. */
> +	if (rw != WRITE || final_size > inode->i_size)
> +		return ext4_ind_direct_IO(rw, iocb, iov, offset, nr_segs);
>  
> -		/* If we do a overwrite dio, i_mutex locking can be released */
> -		overwrite = *((int *)iocb->private);
> +	BUG_ON(iocb->private == NULL);
>  
> -		if (overwrite) {
> -			atomic_inc(&inode->i_dio_count);
> -			down_read(&EXT4_I(inode)->i_data_sem);
> -			mutex_unlock(&inode->i_mutex);
> -		}
> +	/* If we do a overwrite dio, i_mutex locking can be released */
> +	overwrite = *((int *)iocb->private);
>  
> -		/*
> - 		 * We could direct write to holes and fallocate.
> -		 *
> - 		 * Allocated blocks to fill the hole are marked as uninitialized
> - 		 * to prevent parallel buffered read to expose the stale data
> - 		 * before DIO complete the data IO.
> -		 *
> - 		 * As to previously fallocated extents, ext4 get_block
> - 		 * will just simply mark the buffer mapped but still
> - 		 * keep the extents uninitialized.
> - 		 *
> -		 * for non AIO case, we will convert those unwritten extents
> -		 * to written after return back from blockdev_direct_IO.
> -		 *
> -		 * for async DIO, the conversion needs to be defered when
> -		 * the IO is completed. The ext4 end_io callback function
> -		 * will be called to take care of the conversion work.
> -		 * Here for async case, we allocate an io_end structure to
> -		 * hook to the iocb.
> - 		 */
> -		iocb->private = NULL;
> -		ext4_inode_aio_set(inode, NULL);
> -		if (!is_sync_kiocb(iocb)) {
> -			ext4_io_end_t *io_end =
> -				ext4_init_io_end(inode, GFP_NOFS);
> -			if (!io_end) {
> -				ret = -ENOMEM;
> -				goto retake_lock;
> -			}
> -			io_end->flag |= EXT4_IO_END_DIRECT;
> -			iocb->private = io_end;
> -			/*
> -			 * we save the io structure for current async
> -			 * direct IO, so that later ext4_map_blocks()
> -			 * could flag the io structure whether there
> -			 * is a unwritten extents needs to be converted
> -			 * when IO is completed.
> -			 */
> -			ext4_inode_aio_set(inode, io_end);
> -		}
> +	if (overwrite) {
> +		atomic_inc(&inode->i_dio_count);
> +		down_read(&EXT4_I(inode)->i_data_sem);
> +		mutex_unlock(&inode->i_mutex);
> +	}
>  
> -		if (overwrite) {
> -			get_block_func = ext4_get_block_write_nolock;
> -		} else {
> -			get_block_func = ext4_get_block_write;
> -			dio_flags = DIO_LOCKING;
> +	/*
> +	 * We could direct write to holes and fallocate.
> +	 *
> +	 * Allocated blocks to fill the hole are marked as
> +	 * uninitialized to prevent parallel buffered read to expose
> +	 * the stale data before DIO complete the data IO.
> +	 *
> +	 * As to previously fallocated extents, ext4 get_block will
> +	 * just simply mark the buffer mapped but still keep the
> +	 * extents uninitialized.
> +	 *
> +	 * For non AIO case, we will convert those unwritten extents
> +	 * to written after return back from blockdev_direct_IO.
> +	 *
> +	 * For async DIO, the conversion needs to be deferred when the
> +	 * IO is completed. The ext4 end_io callback function will be
> +	 * called to take care of the conversion work.  Here for async
> +	 * case, we allocate an io_end structure to hook to the iocb.
> +	 */
> +	iocb->private = NULL;
> +	ext4_inode_aio_set(inode, NULL);
> +	if (!is_sync_kiocb(iocb)) {
> +		ext4_io_end_t *io_end = ext4_init_io_end(inode, GFP_NOFS);
> +		if (!io_end) {
> +			ret = -ENOMEM;
> +			goto retake_lock;
>  		}
> -		ret = __blockdev_direct_IO(rw, iocb, inode,
> -					 inode->i_sb->s_bdev, iov,
> -					 offset, nr_segs,
> -					 get_block_func,
> -					 ext4_end_io_dio,
> -					 NULL,
> -					 dio_flags);
> -
> -		if (iocb->private)
> -			ext4_inode_aio_set(inode, NULL);
> +		io_end->flag |= EXT4_IO_END_DIRECT;
> +		iocb->private = io_end;
>  		/*
> -		 * The io_end structure takes a reference to the inode,
> -		 * that structure needs to be destroyed and the
> -		 * reference to the inode need to be dropped, when IO is
> -		 * complete, even with 0 byte write, or failed.
> -		 *
> -		 * In the successful AIO DIO case, the io_end structure will be
> -		 * desctroyed and the reference to the inode will be dropped
> -		 * after the end_io call back function is called.
> -		 *
> -		 * In the case there is 0 byte write, or error case, since
> -		 * VFS direct IO won't invoke the end_io call back function,
> -		 * we need to free the end_io structure here.
> +		 * we save the io structure for current async direct
> +		 * IO, so that later ext4_map_blocks() could flag the
> +		 * io structure whether there is a unwritten extents
> +		 * needs to be converted when IO is completed.
>  		 */
> -		if (ret != -EIOCBQUEUED && ret <= 0 && iocb->private) {
> -			ext4_free_io_end(iocb->private);
> -			iocb->private = NULL;
> -		} else if (ret > 0 && !overwrite && ext4_test_inode_state(inode,
> -						EXT4_STATE_DIO_UNWRITTEN)) {
> -			int err;
> -			/*
> -			 * for non AIO case, since the IO is already
> -			 * completed, we could do the conversion right here
> -			 */
> -			err = ext4_convert_unwritten_extents(inode,
> -							     offset, ret);
> -			if (err < 0)
> -				ret = err;
> -			ext4_clear_inode_state(inode, EXT4_STATE_DIO_UNWRITTEN);
> -		}
> +		ext4_inode_aio_set(inode, io_end);
> +	}
>  
> -	retake_lock:
> -		/* take i_mutex locking again if we do a ovewrite dio */
> -		if (overwrite) {
> -			inode_dio_done(inode);
> -			up_read(&EXT4_I(inode)->i_data_sem);
> -			mutex_lock(&inode->i_mutex);
> -		}
> +	if (overwrite) {
> +		get_block_func = ext4_get_block_write_nolock;
> +	} else {
> +		get_block_func = ext4_get_block_write;
> +		dio_flags = DIO_LOCKING;
> +	}
> +	ret = __blockdev_direct_IO(rw, iocb, inode,
> +				   inode->i_sb->s_bdev, iov,
> +				   offset, nr_segs,
> +				   get_block_func,
> +				   ext4_end_io_dio,
> +				   NULL,
> +				   dio_flags);
> +
> +	if (iocb->private)
> +		ext4_inode_aio_set(inode, NULL);
> +	/*
> +	 * The io_end structure takes a reference to the inode, that
> +	 * structure needs to be destroyed and the reference to the
> +	 * inode need to be dropped, when IO is complete, even with 0
> +	 * byte write, or failed.
> +	 *
> +	 * In the successful AIO DIO case, the io_end structure will
> +	 * be destroyed and the reference to the inode will be dropped
> +	 * after the end_io call back function is called.
> +	 *
> +	 * In the case there is 0 byte write, or error case, since VFS
> +	 * direct IO won't invoke the end_io call back function, we
> +	 * need to free the end_io structure here.
> +	 */
> +	if (ret != -EIOCBQUEUED && ret <= 0 && iocb->private) {
> +		ext4_free_io_end(iocb->private);
> +		iocb->private = NULL;
> +	} else if (ret > 0 && !overwrite && ext4_test_inode_state(inode,
> +						EXT4_STATE_DIO_UNWRITTEN)) {
> +		int err;
> +		/*
> +		 * for non AIO case, since the IO is already
> +		 * completed, we could do the conversion right here
> +		 */
> +		err = ext4_convert_unwritten_extents(inode,
> +						     offset, ret);
> +		if (err < 0)
> +			ret = err;
> +		ext4_clear_inode_state(inode, EXT4_STATE_DIO_UNWRITTEN);
> +	}
>  
> -		return ret;
> +retake_lock:
> +	/* take i_mutex locking again if we do a ovewrite dio */
> +	if (overwrite) {
> +		inode_dio_done(inode);
> +		up_read(&EXT4_I(inode)->i_data_sem);
> +		mutex_lock(&inode->i_mutex);
>  	}
>  
> -	/* for write the the end of file case, we fall back to old way */
> -	return ext4_ind_direct_IO(rw, iocb, iov, offset, nr_segs);
> +	return ret;
>  }
>  
>  static ssize_t ext4_direct_IO(int rw, struct kiocb *iocb,
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ