Re: [PATCH] ext4: fix reservation release on invalidatepage for delalloc fs

[Jan Kara <jack@suse.cz>]

On Thu 04-06-15 10:25:01, Lukas Czerner wrote:
> On delalloc enabled file system on invalidatepage operation
> in ext4_da_page_release_reservation() we want to clear the delayed
> buffer and remove the extent covering the delayed buffer from the extent
> status tree.
> 
> However currently there is a bug where on the systems with page size >
> block size we will always remove extents from the start of the page
> regardless where the actual delayed buffers are positioned in the page.
> This leads to the errors like this:
> 
> EXT4-fs warning (device loop0): ext4_da_release_space:1225:
> ext4_da_release_space: ino 13, to_free 1 with only 0 reserved data
> blocks
> 
> This however can cause data loss on writeback time if the file system is
> in ENOSPC condition because we're releasing reservation for someones
> else delayed buffer.
> 
> Fix this by only removing extents that corresponds to the part of the
> page we want to invalidate.
> 
> This problem is reproducible by the following fio receipt (however I was
> only able to reproduce it with fio-2.1 or older.
> 
> [global]
> bs=8k
> iodepth=1024
> iodepth_batch=60
> randrepeat=1
> size=1m
> directory=/mnt/test
> numjobs=20
> [job1]
> ioengine=sync
> bs=1k
> direct=1
> rw=randread
> filename=file1:file2
> [job2]
> ioengine=libaio
> rw=randwrite
> direct=1
> filename=file1:file2
> [job3]
> bs=1k
> ioengine=posixaio
> rw=randwrite
> direct=1
> filename=file1:file2
> [job5]
> bs=1k
> ioengine=sync
> rw=randread
> filename=file1:file2
> [job7]
> ioengine=libaio
> rw=randwrite
> filename=file1:file2
> [job8]
> ioengine=posixaio
> rw=randwrite
> filename=file1:file2
> [job10]
> ioengine=mmap
> rw=randwrite
> bs=1k
> filename=file1:file2
> [job11]
> ioengine=mmap
> rw=randwrite
> direct=1
> filename=file1:file2
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/inode.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 0554b0b..46f4a49 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1342,7 +1342,7 @@ static void ext4_da_page_release_reservation(struct page *page,
>  					     unsigned int offset,
>  					     unsigned int length)
>  {
> -	int to_release = 0;
> +	int to_release = 0, contiguous_blks = 0;
>  	struct buffer_head *head, *bh;
>  	unsigned int curr_off = 0;
>  	struct inode *inode = page->mapping->host;
> @@ -1363,14 +1363,23 @@ static void ext4_da_page_release_reservation(struct page *page,
>  
>  		if ((offset <= curr_off) && (buffer_delay(bh))) {
>  			to_release++;
> +			contiguous_blks++;
>  			clear_buffer_delay(bh);
> +		} else if (contiguous_blks) {
> +			lblk = page->index <<
> +			       (PAGE_CACHE_SHIFT - inode->i_blkbits);
> +			lblk += (curr_off >> inode->i_blkbits) -
> +				contiguous_blks;
> +			ext4_es_remove_extent(inode, lblk, contiguous_blks);
> +			contiguous_blks = 0;
>  		}
>  		curr_off = next_off;
>  	} while ((bh = bh->b_this_page) != head);
>  
> -	if (to_release) {
> +	if (contiguous_blks) {
>  		lblk = page->index << (PAGE_CACHE_SHIFT - inode->i_blkbits);
> -		ext4_es_remove_extent(inode, lblk, to_release);
> +		lblk += (curr_off >> inode->i_blkbits) - contiguous_blks;
> +		ext4_es_remove_extent(inode, lblk, contiguous_blks);
>  	}
>  
>  	/* If we have released all the blocks belonging to a cluster, then we
> -- 
> 1.8.3.1
> 
> --
> 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