iomap: return partial I/O count on error in direct I/O

Commit Message

Goldwyn Rodrigues Feb. 13, 2020, 7:25 p.m. UTC

From: Goldwyn Rodrigues <[e-mail]>

In case of a block device error, iomap code returns 0 as opposed to
the amount of submitted I/O, which may have completed before the
error occurred. Return the count of submitted I/O for correct
accounting.

Signed-off-by: Goldwyn Rodrigues <[e-mail]>
---
 fs/iomap/direct-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christoph Hellwig Feb. 17, 2020, 1:17 p.m. UTC | #1

On Thu, Feb 13, 2020 at 01:25:03PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <[e-mail]>
> 
> In case of a block device error, iomap code returns 0 as opposed to
> the amount of submitted I/O, which may have completed before the
> error occurred. Return the count of submitted I/O for correct
> accounting.

Haven't we traditionally failed direct I/O syscalls that don't fully
complete and never supported short writes (or reads)?

Goldwyn Rodrigues Feb. 17, 2020, 1:44 p.m. UTC | #2

On  5:17 17/02, Christoph Hellwig wrote:
> On Thu, Feb 13, 2020 at 01:25:03PM -0600, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <[e-mail]>
> > 
> > In case of a block device error, iomap code returns 0 as opposed to
> > the amount of submitted I/O, which may have completed before the
> > error occurred. Return the count of submitted I/O for correct
> > accounting.
> 
> Haven't we traditionally failed direct I/O syscalls that don't fully
> complete and never supported short writes (or reads)?

Yes, but I think that decision should be with the filesystem what to do
with it and not the iomap layer.

The reason we need this patch for btrfs is that we need to account for
updating the allocations. iomap_end() returns written as zero while
iomap_dio_rw loop has submitted part of the I/O. So btrfs has no idea
as to how much has been submitted before the failure and how much of
the allocation to update.

This was exhibited by generic/250 in some of the runs where it fails the
underlying storage.

Christoph Hellwig Feb. 17, 2020, 2:02 p.m. UTC | #3

On Mon, Feb 17, 2020 at 07:44:17AM -0600, Goldwyn Rodrigues wrote:
> > Haven't we traditionally failed direct I/O syscalls that don't fully
> > complete and never supported short writes (or reads)?
> 
> Yes, but I think that decision should be with the filesystem what to do
> with it and not the iomap layer.

But then you also need to fix up the existing callers to do the
conversion.

Goldwyn Rodrigues Feb. 19, 2020, 8:31 p.m. UTC | #4

On  6:02 17/02, Christoph Hellwig wrote:
> On Mon, Feb 17, 2020 at 07:44:17AM -0600, Goldwyn Rodrigues wrote:
> > > Haven't we traditionally failed direct I/O syscalls that don't fully
> > > complete and never supported short writes (or reads)?
> > 
> > Yes, but I think that decision should be with the filesystem what to do
> > with it and not the iomap layer.
> 
> But then you also need to fix up the existing callers to do the
> conversion.

The error returned is set in iomap_dio_complete() which happens after.
I checked all instances and the the only place which uses
written in direct I/O is ext4. I will put in the change.

Thanks!

Patch

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 23837926c0c5..a980b7b7660f 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -260,7 +260,7 @@  iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 		size_t n;
 		if (dio->error) {
 			iov_iter_revert(dio->submit.iter, copied);
-			copied = ret = 0;
+			ret = 0;
 			goto out;
 		}