[PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Goldwyn Rodrigues]

In case of a block device error, written parameter in iomap_end()
is zero as opposed to the amount of submitted I/O.
Filesystems such as btrfs need to account for the I/O in ordered
extents, even if it resulted in an error. Having (incomplete)
submitted bytes in written gives the filesystem the amount of data
which has been submitted before the error occurred, and the
filesystem code can choose how to use it.

The final returned error for iomap_dio_rw() is set by
iomap_dio_complete().

Partial writes in direct I/O are considered an error. So,
->iomap_end() using written == 0 as error must be changed
to written < length. In this case, ext4 is the only user.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ext4/inode.c      | 2 +-
 fs/iomap/direct-io.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e60aca791d3f..e50e7414351a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3475,7 +3475,7 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
 	 * the I/O. Any blocks that may have been allocated in preparation for
 	 * the direct I/O will be reused during buffered I/O.
 	 */
-	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written == 0)
+	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written < length)
 		return -ENOTBLK;
 
 	return 0;
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 41c1e7c20a1f..01865db1bd09 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -264,7 +264,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;
 		}
 
-- 
2.25.0

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Matthew Wilcox]

On Thu, Feb 20, 2020 at 09:23:55AM -0600, Goldwyn Rodrigues wrote:
> In case of a block device error, written parameter in iomap_end()
> is zero as opposed to the amount of submitted I/O.
> Filesystems such as btrfs need to account for the I/O in ordered
> extents, even if it resulted in an error. Having (incomplete)
> submitted bytes in written gives the filesystem the amount of data
> which has been submitted before the error occurred, and the
> filesystem code can choose how to use it.
> 
> The final returned error for iomap_dio_rw() is set by
> iomap_dio_complete().
> 
> Partial writes in direct I/O are considered an error. So,
> ->iomap_end() using written == 0 as error must be changed
> to written < length. In this case, ext4 is the only user.

I really had a hard time understanding this.  I think what you meant
was:

Currently, I/Os that complete with an error indicate this by passing
written == 0 to the iomap_end function.  However, btrfs needs to know how
many bytes were written for its own accounting.  Change the convention
to pass the number of bytes which were actually written, and change the
only user to check for a short write instead of a zero length write.

> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/ext4/inode.c      | 2 +-
>  fs/iomap/direct-io.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index e60aca791d3f..e50e7414351a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3475,7 +3475,7 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
>  	 * the I/O. Any blocks that may have been allocated in preparation for
>  	 * the direct I/O will be reused during buffered I/O.
>  	 */
> -	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written == 0)
> +	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written < length)
>  		return -ENOTBLK;
>  
>  	return 0;
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 41c1e7c20a1f..01865db1bd09 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -264,7 +264,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;
>  		}
>  
> -- 
> 2.25.0
> 

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Goldwyn Rodrigues]

On  9:42 20/02, Matthew Wilcox wrote:
> On Thu, Feb 20, 2020 at 09:23:55AM -0600, Goldwyn Rodrigues wrote:
> > In case of a block device error, written parameter in iomap_end()
> > is zero as opposed to the amount of submitted I/O.
> > Filesystems such as btrfs need to account for the I/O in ordered
> > extents, even if it resulted in an error. Having (incomplete)
> > submitted bytes in written gives the filesystem the amount of data
> > which has been submitted before the error occurred, and the
> > filesystem code can choose how to use it.
> > 
> > The final returned error for iomap_dio_rw() is set by
> > iomap_dio_complete().
> > 
> > Partial writes in direct I/O are considered an error. So,
> > ->iomap_end() using written == 0 as error must be changed
> > to written < length. In this case, ext4 is the only user.
> 
> I really had a hard time understanding this.  I think what you meant
> was:
> 
> Currently, I/Os that complete with an error indicate this by passing
> written == 0 to the iomap_end function.  However, btrfs needs to know how
> many bytes were written for its own accounting.  Change the convention
> to pass the number of bytes which were actually written, and change the
> only user to check for a short write instead of a zero length write.

Yes, thats right.  I was trying to cover base from the previous patch
and made a mess of it all. Thanks..


> 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> >  fs/ext4/inode.c      | 2 +-
> >  fs/iomap/direct-io.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index e60aca791d3f..e50e7414351a 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -3475,7 +3475,7 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
> >  	 * the I/O. Any blocks that may have been allocated in preparation for
> >  	 * the direct I/O will be reused during buffered I/O.
> >  	 */
> > -	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written == 0)
> > +	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written < length)
> >  		return -ENOTBLK;
> >  
> >  	return 0;
> > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > index 41c1e7c20a1f..01865db1bd09 100644
> > --- a/fs/iomap/direct-io.c
> > +++ b/fs/iomap/direct-io.c
> > @@ -264,7 +264,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;
> >  		}
> >  
> > -- 
> > 2.25.0
> > 

-- 
Goldwyn

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Ritesh Harjani]

On 2/20/20 8:53 PM, Goldwyn Rodrigues wrote:
> In case of a block device error, written parameter in iomap_end()
> is zero as opposed to the amount of submitted I/O.
> Filesystems such as btrfs need to account for the I/O in ordered
> extents, even if it resulted in an error. Having (incomplete)
> submitted bytes in written gives the filesystem the amount of data
> which has been submitted before the error occurred, and the
> filesystem code can choose how to use it.
> 
> The final returned error for iomap_dio_rw() is set by
> iomap_dio_complete().
> 
> Partial writes in direct I/O are considered an error. So,
> ->iomap_end() using written == 0 as error must be changed
> to written < length. In this case, ext4 is the only user.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>   fs/ext4/inode.c      | 2 +-
>   fs/iomap/direct-io.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index e60aca791d3f..e50e7414351a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3475,7 +3475,7 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
>   	 * the I/O. Any blocks that may have been allocated in preparation for
>   	 * the direct I/O will be reused during buffered I/O.
>   	 */
> -	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written == 0)
> +	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written < length)
>   		return -ENOTBLK;
>   
>   	return 0;
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 41c1e7c20a1f..01865db1bd09 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -264,7 +264,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;
>   		}

But if I am seeing this correctly, even after there was a dio->error
if you return copied > 0, then the loop in iomap_dio_rw will continue
for next iteration as well. Until the second time it won't copy
anything since dio->error is set and from there I guess it may return
0 which will break the loop.

Is this the correct flow? Shouldn't the while loop doing
iomap_apply in iomap_dio_rw should also break in case of
dio->error? Or did I miss anything?


-ritesh

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Goldwyn Rodrigues]

On 10:21 21/02, Ritesh Harjani wrote:
> 
> 
> On 2/20/20 8:53 PM, Goldwyn Rodrigues wrote:
> > In case of a block device error, written parameter in iomap_end()
> > is zero as opposed to the amount of submitted I/O.
> > Filesystems such as btrfs need to account for the I/O in ordered
> > extents, even if it resulted in an error. Having (incomplete)
> > submitted bytes in written gives the filesystem the amount of data
> > which has been submitted before the error occurred, and the
> > filesystem code can choose how to use it.
> > 
> > The final returned error for iomap_dio_rw() is set by
> > iomap_dio_complete().
> > 
> > Partial writes in direct I/O are considered an error. So,
> > ->iomap_end() using written == 0 as error must be changed
> > to written < length. In this case, ext4 is the only user.
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> >   fs/ext4/inode.c      | 2 +-
> >   fs/iomap/direct-io.c | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index e60aca791d3f..e50e7414351a 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -3475,7 +3475,7 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
> >   	 * the I/O. Any blocks that may have been allocated in preparation for
> >   	 * the direct I/O will be reused during buffered I/O.
> >   	 */
> > -	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written == 0)
> > +	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written < length)
> >   		return -ENOTBLK;
> >   	return 0;
> > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > index 41c1e7c20a1f..01865db1bd09 100644
> > --- a/fs/iomap/direct-io.c
> > +++ b/fs/iomap/direct-io.c
> > @@ -264,7 +264,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;
> >   		}
> 
> But if I am seeing this correctly, even after there was a dio->error
> if you return copied > 0, then the loop in iomap_dio_rw will continue
> for next iteration as well. Until the second time it won't copy
> anything since dio->error is set and from there I guess it may return
> 0 which will break the loop.
> 
> Is this the correct flow? Shouldn't the while loop doing
> iomap_apply in iomap_dio_rw should also break in case of
> dio->error? Or did I miss anything?
> 

Yes, We can save an extra iteration by checking for dio->error in the
while loop of iomap_dio_rw(). 

-- 
Goldwyn

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[David Sterba]

On Thu, Feb 20, 2020 at 09:23:55AM -0600, Goldwyn Rodrigues wrote:
> In case of a block device error, written parameter in iomap_end()
> is zero as opposed to the amount of submitted I/O.
> Filesystems such as btrfs need to account for the I/O in ordered
> extents, even if it resulted in an error. Having (incomplete)
> submitted bytes in written gives the filesystem the amount of data
> which has been submitted before the error occurred, and the
> filesystem code can choose how to use it.
> 
> The final returned error for iomap_dio_rw() is set by
> iomap_dio_complete().
> 
> Partial writes in direct I/O are considered an error. So,
> ->iomap_end() using written == 0 as error must be changed
> to written < length. In this case, ext4 is the only user.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 41c1e7c20a1f..01865db1bd09 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -264,7 +264,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;
>  		}

This part fixes problems I saw with the dio-iomap btrfs conversion
patchset, thanks.

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Christoph Hellwig]

On Fri, Feb 21, 2020 at 10:21:04AM +0530, Ritesh Harjani wrote:
> >   		if (dio->error) {
> >   			iov_iter_revert(dio->submit.iter, copied);
> > -			copied = ret = 0;
> > +			ret = 0;
> >   			goto out;
> >   		}
> 
> But if I am seeing this correctly, even after there was a dio->error
> if you return copied > 0, then the loop in iomap_dio_rw will continue
> for next iteration as well. Until the second time it won't copy
> anything since dio->error is set and from there I guess it may return
> 0 which will break the loop.

In addition to that copied is also iov_iter_reexpand call.  We don't
really need the re-expand in case of errors, and in fact we also
have the iov_iter_revert call before jumping out, so this will
need a little bit more of an audit and properly documented in the
commit log.

> 
> Is this the correct flow? Shouldn't the while loop doing
> iomap_apply in iomap_dio_rw should also break in case of
> dio->error? Or did I miss anything?

We'd need something there iff we care about a good number of written
in case of the error.  Goldwyn, can you explain what you need this
number for in btrfs?  Maybe with a pointer to the current code base?

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Damien Le Moal]

On 2020/02/26 5:53, Christoph Hellwig wrote:
> On Fri, Feb 21, 2020 at 10:21:04AM +0530, Ritesh Harjani wrote:
>>>   		if (dio->error) {
>>>   			iov_iter_revert(dio->submit.iter, copied);
>>> -			copied = ret = 0;
>>> +			ret = 0;
>>>   			goto out;
>>>   		}
>>
>> But if I am seeing this correctly, even after there was a dio->error
>> if you return copied > 0, then the loop in iomap_dio_rw will continue
>> for next iteration as well. Until the second time it won't copy
>> anything since dio->error is set and from there I guess it may return
>> 0 which will break the loop.
> 
> In addition to that copied is also iov_iter_reexpand call.  We don't
> really need the re-expand in case of errors, and in fact we also
> have the iov_iter_revert call before jumping out, so this will
> need a little bit more of an audit and properly documented in the
> commit log.
> 
>>
>> Is this the correct flow? Shouldn't the while loop doing
>> iomap_apply in iomap_dio_rw should also break in case of
>> dio->error? Or did I miss anything?
> 
> We'd need something there iff we care about a good number of written
> in case of the error.  Goldwyn, can you explain what you need this
> number for in btrfs?  Maybe with a pointer to the current code base?

Not sure about btrfs, but for zonefs, getting the partial I/O count done for a
failed large dio would also be useful to avoid having to do the error recovery
dance with report zones for getting the current zone write pointer.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Goldwyn Rodrigues]

On 12:53 25/02, Christoph Hellwig wrote:
> On Fri, Feb 21, 2020 at 10:21:04AM +0530, Ritesh Harjani wrote:
> > >   		if (dio->error) {
> > >   			iov_iter_revert(dio->submit.iter, copied);
> > > -			copied = ret = 0;
> > > +			ret = 0;
> > >   			goto out;
> > >   		}
> > 
> > But if I am seeing this correctly, even after there was a dio->error
> > if you return copied > 0, then the loop in iomap_dio_rw will continue
> > for next iteration as well. Until the second time it won't copy
> > anything since dio->error is set and from there I guess it may return
> > 0 which will break the loop.
> 
> In addition to that copied is also iov_iter_reexpand call.  We don't
> really need the re-expand in case of errors, and in fact we also
> have the iov_iter_revert call before jumping out, so this will
> need a little bit more of an audit and properly documented in the
> commit log.
> 
> > 
> > Is this the correct flow? Shouldn't the while loop doing
> > iomap_apply in iomap_dio_rw should also break in case of
> > dio->error? Or did I miss anything?
> 
> We'd need something there iff we care about a good number of written
> in case of the error.  Goldwyn, can you explain what you need this
> number for in btrfs?  Maybe with a pointer to the current code base?

btrfs needs to account for the bytes "processed", failed or
uptodate. This is currently performed in
fs/btrfs/inode.c:__end_write_update_ordered().

For the current development version, how I am using it is in my git
branch btrfs-iomap-dio [1]. The related commit besides this patch
is:

9aeb2b31d10b ("btrfs: Use ->iomap_end() instead of btrfs_dio_data")

[1] https://github.com/goldwynr/linux/tree/btrfs-iomap-dio

-- 
Goldwyn

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Goldwyn Rodrigues]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On 12:53 25/02, Christoph Hellwig wrote:
> On Fri, Feb 21, 2020 at 10:21:04AM +0530, Ritesh Harjani wrote:
> > >   		if (dio->error) {
> > >   			iov_iter_revert(dio->submit.iter, copied);
> > > -			copied = ret = 0;
> > > +			ret = 0;
> > >   			goto out;
> > >   		}
> > 
> > But if I am seeing this correctly, even after there was a dio->error
> > if you return copied > 0, then the loop in iomap_dio_rw will continue
> > for next iteration as well. Until the second time it won't copy
> > anything since dio->error is set and from there I guess it may return
> > 0 which will break the loop.
> 


Reading the code again, there are a few clarifications.

If iomap_end() handles (written < length) as an error, iomap_apply()
will return an error immediately. It will not execute the 
loop a second time.

On the other hand, if there is no ->iomap_end() defined by the
filesystem such as in the case of XFS, we will need to check for
dio->error in the do {} while loop of iomap_dio_rw().

> In addition to that copied is also iov_iter_reexpand call.  We don't
> really need the re-expand in case of errors, and in fact we also
> have the iov_iter_revert call before jumping out, so this will
> need a little bit more of an audit and properly documented in the
> commit log.

We are still handling this as an error, so why are we concerned about
expanding? There is no success/written returned in iomap_dio_rw() call
in case of an error.

Attached is an updated patch.


-- 
Goldwyn

[-- Attachment #2: 0004-iomap-return-partial-I-O-count-on-error-in-iomap_dio.patch --]
[-- Type: text/x-patch, Size: 2132 bytes --]

From af694f4fc662daf5c62a78391ced5f8e2d4beed2 Mon Sep 17 00:00:00 2001
From: Goldwyn Rodrigues <rgoldwyn@suse.com>
Date: Thu, 13 Feb 2020 13:28:55 -0600
Subject: [PATCH] iomap: return partial I/O count on error in
 iomap_dio_bio_actor

Currently, I/Os that complete with an error indicate this by passing
written == 0 to the iomap_end function.  However, btrfs needs to know how
many bytes were written for its own accounting.  Change the convention
to pass the number of bytes which were actually written, and change the
only user (ext4) to check for a short write instead of a zero length write.

In case a filesystem does not define an ->iomap_end(), check for
dio->error after the iomap_apply() call to diagnose the error.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ext4/inode.c      | 2 +-
 fs/iomap/direct-io.c | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index fa0ff78dc033..d52c70f851e6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3475,7 +3475,7 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
 	 * the I/O. Any blocks that may have been allocated in preparation for
 	 * the direct I/O will be reused during buffered I/O.
 	 */
-	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written == 0)
+	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written < length)
 		return -ENOTBLK;
 
 	return 0;
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 41c1e7c20a1f..a0002311cc20 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -264,7 +264,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;
 		}
 
@@ -499,6 +499,10 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	do {
 		ret = iomap_apply(inode, pos, count, flags, ops, dio,
 				iomap_dio_actor);
+
+		if (ret >= 0 && dio->error)
+			ret = dio->error;
+
 		if (ret <= 0) {
 			/* magic error code to fall back to buffered I/O */
 			if (ret == -ENOTBLK) {
-- 
2.25.0

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Matthew Wilcox]

On Fri, Feb 28, 2020 at 01:44:01PM -0600, Goldwyn Rodrigues wrote:
> +++ b/fs/iomap/direct-io.c
> @@ -264,7 +264,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;

There's another change here ... look at the out label

out:
        /* Undo iter limitation to current extent */
        iov_iter_reexpand(dio->submit.iter, orig_count - copied);
        if (copied)
                return copied;
        return ret;

so you're also changing by how much the iter is reexpanded.  I
don't know if it's the appropriate amount; I still don't quite get the
iov_iter complexities.

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Goldwyn Rodrigues]

On 11:59 28/02, Matthew Wilcox wrote:
> On Fri, Feb 28, 2020 at 01:44:01PM -0600, Goldwyn Rodrigues wrote:
> > +++ b/fs/iomap/direct-io.c
> > @@ -264,7 +264,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;
> 
> There's another change here ... look at the out label
> 
> out:
>         /* Undo iter limitation to current extent */
>         iov_iter_reexpand(dio->submit.iter, orig_count - copied);
>         if (copied)
>                 return copied;
>         return ret;
> 
> so you're also changing by how much the iter is reexpanded.  I
> don't know if it's the appropriate amount; I still don't quite get the
> iov_iter complexities.
> 

Ah, okay. Now I understand what Christoph was saying.

I suppose it is safe to remove iov_iter_reexpand(). I don't see any
other goto to this label which will have a non-zero copied value.
And we have already performed the iov_iter_revert().

-- 
Goldwyn

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Christoph Hellwig]

On Fri, Feb 28, 2020 at 02:35:38PM -0600, Goldwyn Rodrigues wrote:
> 
> Ah, okay. Now I understand what Christoph was saying.
> 
> I suppose it is safe to remove iov_iter_reexpand(). I don't see any
> other goto to this label which will have a non-zero copied value.
> And we have already performed the iov_iter_revert().

I don't really understand the iov_iter complexities either, at least
not without spending sifnificant time with the implementation.  But
the important thing is that you document the changes in behavior and
your findings on why you think it is safe.

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Goldwyn Rodrigues]

On  7:35 20/03, Christoph Hellwig wrote:
> On Fri, Mar 20, 2020 at 10:23:43AM -0400, Josef Bacik wrote:
> > I'm not sure what you're looking at specifically wrt error handling, but I
> > can explain __endio_write_update_ordered.
> > 
> > Btrfs has ordered extents to keep track of an extent that currently has IO
> > being done on it.  Generally that IO takes multiple bio's, so we keep track
> > of the outstanding size of the IO being done, and each bio completes and
> > thus removes its size from the pending size.  If any one of those bios has
> > an error we need to make sure we discard the whole ordered extent, as part
> > of it won't be valid. Just a cursory look at the current code I assume
> > that's what's confusing you, we call this when we have an error in the
> > O_DIRECT code.  This is just so we get the proper cleanup for the ordered
> > extent.  People will wait on the ordered extent to be completed, so if we've
> > started an ordered extent and aren't able to complete the range we need to
> > do __endio_write_update_ordered() so that the ordered extent is finished and
> > we wakeup any waiters.
> > 
> > Does this help?  If I need to I can context switch into whatever you're
> > looking at, but I'm going to avoid looking and hope I can just shout useful
> > information in your direction ;).  Thanks,
> 
> Yes, this helps a lot.  This is about the patches from Goldwyn to
> convert btrfs to use the iomap direct I/O code.  And in that series
> he currently calls __endio_write_update_ordered from the ->iomap_end
> method, which for direct I/O is called after all bios are submitted
> to complete ordered extents for a range after an I/O error, that
> is one that no I/O has been submitted to, and the accounting for that
> is a little complicated..

I think you meant "some" instead of "no".

Yes, keeping the information in iomap->private and setting in
btrfs_submit_direct() would be better. I will modify the code and
re-test. Thanks!

-- 
Goldwyn

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Christoph Hellwig]

On Fri, Mar 20, 2020 at 10:23:43AM -0400, Josef Bacik wrote:
> I'm not sure what you're looking at specifically wrt error handling, but I
> can explain __endio_write_update_ordered.
> 
> Btrfs has ordered extents to keep track of an extent that currently has IO
> being done on it.  Generally that IO takes multiple bio's, so we keep track
> of the outstanding size of the IO being done, and each bio completes and
> thus removes its size from the pending size.  If any one of those bios has
> an error we need to make sure we discard the whole ordered extent, as part
> of it won't be valid. Just a cursory look at the current code I assume
> that's what's confusing you, we call this when we have an error in the
> O_DIRECT code.  This is just so we get the proper cleanup for the ordered
> extent.  People will wait on the ordered extent to be completed, so if we've
> started an ordered extent and aren't able to complete the range we need to
> do __endio_write_update_ordered() so that the ordered extent is finished and
> we wakeup any waiters.
> 
> Does this help?  If I need to I can context switch into whatever you're
> looking at, but I'm going to avoid looking and hope I can just shout useful
> information in your direction ;).  Thanks,

Yes, this helps a lot.  This is about the patches from Goldwyn to
convert btrfs to use the iomap direct I/O code.  And in that series
he currently calls __endio_write_update_ordered from the ->iomap_end
method, which for direct I/O is called after all bios are submitted
to complete ordered extents for a range after an I/O error, that
is one that no I/O has been submitted to, and the accounting for that
is a little complicated..

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Josef Bacik]

On 3/20/20 10:05 AM, Christoph Hellwig wrote:
> I spent a fair amount of time looking over this change, and I am
> starting to feel very bad about it.  iomap_apply() has pretty clear
> semantics of either return an error, or return the bytes processed,
> and in general these semantics work just fine.
> 
> The thing that breaks this concept is the btrfs submit_bio hook,
> which allows the file system to keep state for each bio actually
> submitted.  But I think you can simply keep the length internally
> in btrfs - use the space in iomap->private as a counter of how
> much was allocated, pass the iomap to the submit_io hook, and
> update it there, and then deal with the rest in ->iomap_end.
> 
> That assumes ->iomap_end actually is the right place - can someone
> explain what the expected call site for __endio_write_update_ordered
> is?  It kinda sorta looks to me like something that would want to
> be called after I/O completion, not after I/O submission, but maybe
> I misunderstand the code.
> 

I'm not sure what you're looking at specifically wrt error handling, but I can 
explain __endio_write_update_ordered.

Btrfs has ordered extents to keep track of an extent that currently has IO being 
done on it.  Generally that IO takes multiple bio's, so we keep track of the 
outstanding size of the IO being done, and each bio completes and thus removes 
its size from the pending size.  If any one of those bios has an error we need 
to make sure we discard the whole ordered extent, as part of it won't be valid. 
Just a cursory look at the current code I assume that's what's confusing you, we 
call this when we have an error in the O_DIRECT code.  This is just so we get 
the proper cleanup for the ordered extent.  People will wait on the ordered 
extent to be completed, so if we've started an ordered extent and aren't able to 
complete the range we need to do __endio_write_update_ordered() so that the 
ordered extent is finished and we wakeup any waiters.

Does this help?  If I need to I can context switch into whatever you're looking 
at, but I'm going to avoid looking and hope I can just shout useful information 
in your direction ;).  Thanks,

Josef

Re: [PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Christoph Hellwig]

I spent a fair amount of time looking over this change, and I am
starting to feel very bad about it.  iomap_apply() has pretty clear
semantics of either return an error, or return the bytes processed,
and in general these semantics work just fine.

The thing that breaks this concept is the btrfs submit_bio hook,
which allows the file system to keep state for each bio actually
submitted.  But I think you can simply keep the length internally
in btrfs - use the space in iomap->private as a counter of how
much was allocated, pass the iomap to the submit_io hook, and
update it there, and then deal with the rest in ->iomap_end.

That assumes ->iomap_end actually is the right place - can someone
explain what the expected call site for __endio_write_update_ordered
is?  It kinda sorta looks to me like something that would want to
be called after I/O completion, not after I/O submission, but maybe
I misunderstand the code.

[PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Goldwyn Rodrigues]

Currently, I/Os that complete with an error indicate this by passing
written == 0 to the iomap_end function.  However, btrfs needs to know how
many bytes were written for its own accounting.  Change the convention
to pass the number of bytes which were actually written, and change the
only user (ext4) to check for a short write instead of a zero length
write.

For filesystems that do not define ->iomap_end(), check for
dio->error again after the iomap_apply() call to diagnose the error.

Changes since v1:
 - Considerate of iov_iter rollback functions
 - Double check errors for filesystems not implementing iomap_end()

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index fa0ff78..d52c70f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3475,7 +3475,7 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
 	 * the I/O. Any blocks that may have been allocated in preparation for
 	 * the direct I/O will be reused during buffered I/O.
 	 */
-	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written == 0)
+	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written < length)
 		return -ENOTBLK;
 
 	return 0;
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 41c1e7c..b5f4d4a 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -264,7 +264,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 = dio->error;
 			goto out;
 		}
 
@@ -325,8 +325,17 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
 			iomap_dio_zero(dio, iomap, pos, fs_block_size - pad);
 	}
 out:
-	/* Undo iter limitation to current extent */
-	iov_iter_reexpand(dio->submit.iter, orig_count - copied);
+	/*
+	 * Undo iter limitation to current extent
+	 * If there is an error, undo the entire extent. However, return the
+	 * bytes copied so far for filesystems such as btrfs to account for
+	 * submitted I/O.
+	 */
+	if (ret < 0)
+		iov_iter_reexpand(dio->submit.iter, orig_count);
+	else
+		iov_iter_reexpand(dio->submit.iter, orig_count - copied);
+
 	if (copied)
 		return copied;
 	return ret;
@@ -499,6 +508,10 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	do {
 		ret = iomap_apply(inode, pos, count, flags, ops, dio,
 				iomap_dio_actor);
+
+		if (ret >= 0 && dio->error)
+			ret = dio->error;
+
 		if (ret <= 0) {
 			/* magic error code to fall back to buffered I/O */
 			if (ret == -ENOTBLK) {

-- 
Goldwyn

[PATCH v2] iomap: return partial I/O count on error in iomap_dio_bio_actor

[Goldwyn Rodrigues]

In case of a block device error, written parameter in iomap_end()
is zero as opposed to the amount of submitted I/O.
Filesystems such as btrfs need to account for the I/O in ordered
extents, even if it resulted in an error. Having (incomplete)
submitted bytes in written gives the filesystem the amount of data
which has been submitted before the error occurred, and the
filesystem code can choose how to use it.

The final returned error for iomap_dio_rw() is set by
iomap_dio_complete().

Partial writes in direct I/O are considered an error. So,
->iomap_end() using written == 0 as error must be changed
to written < length. In this case, ext4 is the only user.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/ext4/inode.c      | 2 +-
 fs/iomap/direct-io.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e60aca791d3f..e50e7414351a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3475,7 +3475,7 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
 	 * the I/O. Any blocks that may have been allocated in preparation for
 	 * the direct I/O will be reused during buffered I/O.
 	 */
-	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written == 0)
+	if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written < length)
 		return -ENOTBLK;
 
 	return 0;
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 41c1e7c20a1f..01865db1bd09 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -264,7 +264,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;
 		}
 
-- 
2.25.0


-- 
Goldwyn