Re: [PATCH] ext4: Fix DIO locking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2008-02-06 at 17:06 +0100, Jan Kara wrote:
>   Hi,
> 
>   this patch fixes lock inversion in ext4 direct IO path. The similar patch
> has already been accepted for ext3. Mingming, will you put it into ext4
> patch queue? Thanks.
> 

Thanks! Added to the ext4 patch queue.

Mingming
> 								Honza
> -- 
> Jan Kara <jack@xxxxxxx>
> SUSE Labs, CR
> ---
> 
> We cannot start transaction in ext4_direct_IO() and just let it last during the
> whole write because dio_get_page() acquires mmap_sem which ranks above
> transaction start (e.g. because we have dependency chain
> mmap_sem->PageLock->journal_start, or because we update atime while holding
> mmap_sem) and thus deadlocks could happen. We solve the problem by starting
> a transaction separately for each ext4_get_block() call.
> 
> We *could* have a problem that we allocate a block and before its data are
> written out the machine crashes and thus we expose stale data. But that
> does not happen because for hole-filling generic code falls back to buffered
> writes and for file extension, we add inode to orphan list and thus in
> case of crash, journal replay will truncate inode back to the original size.
> 
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index bb717cb..1948b97 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -895,7 +895,16 @@ out:
>  	return err;
>  }
> 
> -#define DIO_CREDITS (EXT4_RESERVE_TRANS_BLOCKS + 32)
> +/* Maximum number of blocks we map for direct IO at once. */
> +#define DIO_MAX_BLOCKS 4096
> +/*
> + * Number of credits we need for writing DIO_MAX_BLOCKS:
> + * We need sb + group descriptor + bitmap + inode -> 4
> + * For B blocks with A block pointers per block we need:
> + * 1 (triple ind.) + (B/A/A + 2) (doubly ind.) + (B/A + 2) (indirect).
> + * If we plug in 4096 for B and 256 for A (for 1KB block size), we get 25.
> + */
> +#define DIO_CREDITS 25
> 
>  int ext4_get_blocks_wrap(handle_t *handle, struct inode *inode, sector_t block,
>  			unsigned long max_blocks, struct buffer_head *bh,
> @@ -942,49 +951,31 @@ static int ext4_get_block(struct inode *inode, sector_t iblock,
>  			struct buffer_head *bh_result, int create)
>  {
>  	handle_t *handle = ext4_journal_current_handle();
> -	int ret = 0;
> +	int ret = 0, started = 0;
>  	unsigned max_blocks = bh_result->b_size >> inode->i_blkbits;
> 
> -	if (!create)
> -		goto get_block;		/* A read */
> -
> -	if (max_blocks == 1)
> -		goto get_block;		/* A single block get */
> -
> -	if (handle->h_transaction->t_state == T_LOCKED) {
> -		/*
> -		 * Huge direct-io writes can hold off commits for long
> -		 * periods of time.  Let this commit run.
> -		 */
> -		ext4_journal_stop(handle);
> -		handle = ext4_journal_start(inode, DIO_CREDITS);
> -		if (IS_ERR(handle))
> +	
> +	if (create && !handle) {	/* Direct IO write... */
> +		if (max_blocks > DIO_MAX_BLOCKS)
> +			max_blocks = DIO_MAX_BLOCKS;
> +		handle = ext4_journal_start(inode, DIO_CREDITS +
> +			      2 * EXT4_QUOTA_TRANS_BLOCKS(inode->i_sb));
> +		if (IS_ERR(handle)) {
>  			ret = PTR_ERR(handle);
> -		goto get_block;
> -	}
> -
> -	if (handle->h_buffer_credits <= EXT4_RESERVE_TRANS_BLOCKS) {
> -		/*
> -		 * Getting low on buffer credits...
> -		 */
> -		ret = ext4_journal_extend(handle, DIO_CREDITS);
> -		if (ret > 0) {
> -			/*
> -			 * Couldn't extend the transaction.  Start a new one.
> -			 */
> -			ret = ext4_journal_restart(handle, DIO_CREDITS);
> +			goto out;
>  		}
> +		started = 1;
>  	}
> 
> -get_block:
> -	if (ret == 0) {
> -		ret = ext4_get_blocks_wrap(handle, inode, iblock,
> +	ret = ext4_get_blocks_wrap(handle, inode, iblock,
>  					max_blocks, bh_result, create, 0);
> -		if (ret > 0) {
> -			bh_result->b_size = (ret << inode->i_blkbits);
> -			ret = 0;
> -		}
> +	if (ret > 0) {
> +		bh_result->b_size = (ret << inode->i_blkbits);
> +		ret = 0;
>  	}
> +	if (started)
> +		ext4_journal_stop(handle);
> +out:
>  	return ret;
>  }
> 
> @@ -1674,7 +1665,8 @@ static int ext4_releasepage(struct page *page, gfp_t wait)
>   * if the machine crashes during the write.
>   *
>   * If the O_DIRECT write is intantiating holes inside i_size and the machine
> - * crashes then stale disk data _may_ be exposed inside the file.
> + * crashes then stale disk data _may_ be exposed inside the file. But current
> + * VFS code falls back into buffered path in that case so we are safe.
>   */
>  static ssize_t ext4_direct_IO(int rw, struct kiocb *iocb,
>  			const struct iovec *iov, loff_t offset,
> @@ -1683,7 +1675,7 @@ static ssize_t ext4_direct_IO(int rw, struct kiocb *iocb,
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file->f_mapping->host;
>  	struct ext4_inode_info *ei = EXT4_I(inode);
> -	handle_t *handle = NULL;
> +	handle_t *handle;
>  	ssize_t ret;
>  	int orphan = 0;
>  	size_t count = iov_length(iov, nr_segs);
> @@ -1691,17 +1683,21 @@ static ssize_t ext4_direct_IO(int rw, struct kiocb *iocb,
>  	if (rw == WRITE) {
>  		loff_t final_size = offset + count;
> 
> -		handle = ext4_journal_start(inode, DIO_CREDITS);
> -		if (IS_ERR(handle)) {
> -			ret = PTR_ERR(handle);
> -			goto out;
> -		}
>  		if (final_size > inode->i_size) {
> +			/* Credits for sb + inode write */
> +			handle = ext4_journal_start(inode, 2);
> +			if (IS_ERR(handle)) {
> +				ret = PTR_ERR(handle);
> +				goto out;
> +			}
>  			ret = ext4_orphan_add(handle, inode);
> -			if (ret)
> -				goto out_stop;
> +			if (ret) {
> +				ext4_journal_stop(handle);
> +				goto out;
> +			}
>  			orphan = 1;
>  			ei->i_disksize = inode->i_size;
> +			ext4_journal_stop(handle);
>  		}
>  	}
> 
> @@ -1709,18 +1705,21 @@ static ssize_t ext4_direct_IO(int rw, struct kiocb *iocb,
>  				 offset, nr_segs,
>  				 ext4_get_block, NULL);
> 
> -	/*
> -	 * Reacquire the handle: ext4_get_block() can restart the transaction
> -	 */
> -	handle = ext4_journal_current_handle();
> -
> -out_stop:
> -	if (handle) {
> +	if (orphan) {
>  		int err;
> 
> -		if (orphan && inode->i_nlink)
> +		/* Credits for sb + inode write */
> +		handle = ext4_journal_start(inode, 2);
> +		if (IS_ERR(handle)) {
> +			/* This is really bad luck. We've written the data
> +			 * but cannot extend i_size. Bail out and pretend
> +			 * the write failed... */
> +			ret = PTR_ERR(handle);
> +			goto out;
> +		}
> +		if (inode->i_nlink)
>  			ext4_orphan_del(handle, inode);
> -		if (orphan && ret > 0) {
> +		if (ret > 0) {
>  			loff_t end = offset + ret;
>  			if (end > inode->i_size) {
>  				ei->i_disksize = end;
> -
> 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

-
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

[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux