From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from bombadil.infradead.org ([65.50.211.133]:41644 "EHLO
        bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750849AbdJAIWl (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Sun, 1 Oct 2017 04:22:41 -0400
Date: Sun, 1 Oct 2017 01:22:40 -0700
From: Christoph Hellwig <hch@infradead.org>
To: Goldwyn Rodrigues <rgoldwyn@suse.de>
Cc: linux-fsdevel@vger.kernel.org, hch@infradead.org,
        willy@infradead.org, Goldwyn Rodrigues <rgoldwyn@suse.com>
Subject: Re: [PATCH v3] Return bytes transferred for partial direct I/O
Message-ID: <20171001082240.GB9299@infradead.org>
References: <20170929155030.7726-1-rgoldwyn@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170929155030.7726-1-rgoldwyn@suse.de>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Fri, Sep 29, 2017 at 10:50:30AM -0500, Goldwyn Rodrigues wrote:
> In case direct I/O encounters an error midway, it returns the error.
> Instead it should be returning the number of bytes transferred so far.
> 
> Test case for filesystems (with ENOSPC):
> 1. Create an almost full filesystem
> 2. Create a file, say /mnt/lastfile, until the filesystem is full.
> 3. Direct write() with count > sizeof /mnt/lastfile.
> 
> Result: write() returns -ENOSPC. However, file content has data written
> in step 3.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Changes since v1:
>  - incorporated iomap and block devices
> 
> Changes since v2:
>  - realized that file size was not increasing when performing a (partial)
>    direct I/O because end_io function was receiving the error instead of
>    size in iomap's dio end io. Fixed.
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 93d088ffc05c..1c6640ffb929 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -421,7 +421,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages)
>  
>  	if (!ret)
>  		ret = blk_status_to_errno(dio->bio.bi_status);
> -	if (likely(!ret))
> +	if (likely(dio->size))
>  		ret = dio->size;
>  
>  	bio_put(&dio->bio);
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 5fa2211e49ae..0fc1789498ae 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -255,8 +255,6 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async)
>  		ret = dio->page_errors;
>  	if (ret == 0)
>  		ret = dio->io_error;
> -	if (ret == 0)
> -		ret = transferred;
>  
>  	if (dio->end_io) {
>  		int err;
> @@ -284,7 +282,7 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async)
>  	}
>  
>  	kmem_cache_free(dio_cache, dio);
> -	return ret;
> +	return transferred ? transferred : ret;
>  }
>  
>  static void dio_aio_complete_work(struct work_struct *work)
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 269b24a01f32..019c4d993300 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -714,28 +714,29 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
>  {
>  	struct kiocb *iocb = dio->iocb;
>  	ssize_t ret;
> +	ssize_t transferred = 0;

How about initializing this to dio->size here and then using it
further down everywere you currently use dio->size?

Otherwise looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>