Re: [PATCH] bdev: Do not return EBUSY if bdev discard races with write

[Maxim Levitsky <mlevitsk@redhat.com>]

On Thu, 2021-01-07 at 16:40 +0100, Jan Kara wrote:
> blkdev_fallocate() tries to detect whether a discard raced with an
> overlapping write by calling invalidate_inode_pages2_range(). However
> this check can give both false negatives (when writing using direct IO
> or when writeback already writes out the written pagecache range) and
> false positives (when write is not actually overlapping but ends in the
> same page when blocksize < pagesize). This actually causes issues for
> qemu which is getting confused by EBUSY errors.
> 
> Fix the problem by removing this conflicting write detection since it is
> inherently racy and thus of little use anyway.
> 
> Reported-by: Maxim Levitsky <mlevitsk@redhat.com>
> CC: "Darrick J. Wong" <darrick.wong@oracle.com>
> Link: https://lore.kernel.org/qemu-devel/20201111153913.41840-1-mlevitsk@redhat.com
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/block_dev.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 3e5b02f6606c..a97f43b49839 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1797,13 +1797,11 @@ static long blkdev_fallocate(struct file *file, int mode, loff_t start,
>  		return error;
>  
>  	/*
> -	 * Invalidate again; if someone wandered in and dirtied a page,
> -	 * the caller will be given -EBUSY.  The third argument is
> -	 * inclusive, so the rounding here is safe.
> +	 * Invalidate the page cache again; if someone wandered in and dirtied
> +	 * a page, we just discard it - userspace has no way of knowing whether
> +	 * the write happened before or after discard completing...
>  	 */
> -	return invalidate_inode_pages2_range(bdev->bd_inode->i_mapping,
> -					     start >> PAGE_SHIFT,
> -					     end >> PAGE_SHIFT);
> +	return truncate_bdev_range(bdev, file->f_mode, start, end);
>  }


But what happens if write and discard don't overlap? Won't we
discard the written data in this case?


Best regards,
	Maxim Levitsky


>  
>  const struct file_operations def_blk_fops = {