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@xxxxxxxxxx>
Looks good. You can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
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
