[PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Christoph Hellwig]

bytes also hold the return value from iomap_write_end, which can contain
a negative error value.  As bytes is always less than the page size even
the signed type can hold the entire possible range.

Fixes: c6f40468657d ("fsdax: decouple zeroing from the iomap buffered I/O code")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/buffered-io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index b1511255b4df8..ac040d607f4fe 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -883,7 +883,7 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 
 	do {
 		unsigned offset = offset_in_page(pos);
-		size_t bytes = min_t(u64, PAGE_SIZE - offset, length);
+		ssize_t bytes = min_t(u64, PAGE_SIZE - offset, length);
 		struct page *page;
 		int status;
 
-- 
2.30.2

Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Darrick J. Wong]

On Wed, Dec 08, 2021 at 10:12:03AM +0100, Christoph Hellwig wrote:
> bytes also hold the return value from iomap_write_end, which can contain
> a negative error value.  As bytes is always less than the page size even
> the signed type can hold the entire possible range.
> 
> Fixes: c6f40468657d ("fsdax: decouple zeroing from the iomap buffered I/O code")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/iomap/buffered-io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index b1511255b4df8..ac040d607f4fe 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -883,7 +883,7 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  
>  	do {
>  		unsigned offset = offset_in_page(pos);
> -		size_t bytes = min_t(u64, PAGE_SIZE - offset, length);
> +		ssize_t bytes = min_t(u64, PAGE_SIZE - offset, length);
>  		struct page *page;
>  		int status;
>  
> -- 
> 2.30.2
> 

Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Darrick J. Wong]

On Wed, Dec 08, 2021 at 04:48:46PM -0800, Darrick J. Wong wrote:
> On Wed, Dec 08, 2021 at 10:12:03AM +0100, Christoph Hellwig wrote:
> > bytes also hold the return value from iomap_write_end, which can contain
> > a negative error value.  As bytes is always less than the page size even
> > the signed type can hold the entire possible range.
> > 
> > Fixes: c6f40468657d ("fsdax: decouple zeroing from the iomap buffered I/O code")

...waitaminute, ^^^^^^ in what tree is this commit?  Did Linus merge
the dax decoupling series into upstream without telling me?

/me checks... no?

Though I searched for it on gitweb and came up with this bizarre plot
twist:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c6f40468657d16e4010ef84bf32a761feb3469ea

(Is this the same as that github thing a few months ago where
everybody's commits get deduplicated into the same realm and hence
anyone can make trick the frontend into sort of making it look like
their rando commits end up in Linus' tree?  Or did it get merged and
push -f reverted?)

Ok, so ... I don't know what I'm supposed to apply this to?  Is this
something that should go in Christoph's development branch?

<confused, going to run away now>

On the plus side, that means I /can/ go test-merge willy's iomap folios
for 5.17 stuff tonight.

--D

> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Looks good,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
> > ---
> >  fs/iomap/buffered-io.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index b1511255b4df8..ac040d607f4fe 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -883,7 +883,7 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> >  
> >  	do {
> >  		unsigned offset = offset_in_page(pos);
> > -		size_t bytes = min_t(u64, PAGE_SIZE - offset, length);
> > +		ssize_t bytes = min_t(u64, PAGE_SIZE - offset, length);
> >  		struct page *page;
> >  		int status;
> >  
> > -- 
> > 2.30.2
> > 

Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Dan Williams]

On Wed, Dec 8, 2021 at 4:56 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Wed, Dec 08, 2021 at 04:48:46PM -0800, Darrick J. Wong wrote:
> > On Wed, Dec 08, 2021 at 10:12:03AM +0100, Christoph Hellwig wrote:
> > > bytes also hold the return value from iomap_write_end, which can contain
> > > a negative error value.  As bytes is always less than the page size even
> > > the signed type can hold the entire possible range.
> > >
> > > Fixes: c6f40468657d ("fsdax: decouple zeroing from the iomap buffered I/O code")
>
> ...waitaminute, ^^^^^^ in what tree is this commit?  Did Linus merge
> the dax decoupling series into upstream without telling me?
>
> /me checks... no?
>
> Though I searched for it on gitweb and came up with this bizarre plot
> twist:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c6f40468657d16e4010ef84bf32a761feb3469ea
>
> (Is this the same as that github thing a few months ago where
> everybody's commits get deduplicated into the same realm and hence
> anyone can make trick the frontend into sort of making it look like
> their rando commits end up in Linus' tree?  Or did it get merged and
> push -f reverted?)
>
> Ok, so ... I don't know what I'm supposed to apply this to?  Is this
> something that should go in Christoph's development branch?
>
> <confused, going to run away now>
>
> On the plus side, that means I /can/ go test-merge willy's iomap folios
> for 5.17 stuff tonight.

This commit is in the nvdimm.git tree and is merged in linux-next.

Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Christoph Hellwig]

On Wed, Dec 08, 2021 at 04:55:59PM -0800, Darrick J. Wong wrote:
> Ok, so ... I don't know what I'm supposed to apply this to?  Is this
> something that should go in Christoph's development branch?

This is against the decouple DAX from block devices series, which also
decouples DAX zeroing from iomap buffered I/O zeroing.  It is in nvdimm.git
and has been reviewed by you as well:

https://git.kernel.org/pub/scm/linux/kernel/git/djbw/nvdimm.git/log/?h=libnvdimm-pending

Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Darrick J. Wong]

On Thu, Dec 09, 2021 at 07:11:19AM +0100, Christoph Hellwig wrote:
> On Wed, Dec 08, 2021 at 04:55:59PM -0800, Darrick J. Wong wrote:
> > Ok, so ... I don't know what I'm supposed to apply this to?  Is this
> > something that should go in Christoph's development branch?
> 
> This is against the decouple DAX from block devices series, which also
> decouples DAX zeroing from iomap buffered I/O zeroing.  It is in nvdimm.git
> and has been reviewed by you as well:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djbw/nvdimm.git/log/?h=libnvdimm-pending

Ah, ok.  IOWs, this is a 5.17 thing, not a critical fix for 5.16-rcX.

--D

Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Matthew Wilcox]

On Wed, Dec 08, 2021 at 10:12:03AM +0100, Christoph Hellwig wrote:
> bytes also hold the return value from iomap_write_end, which can contain
> a negative error value.  As bytes is always less than the page size even
> the signed type can hold the entire possible range.

iomap_write_end() can't return an errno.  I went through and checked as
part of the folio conversion.  It actually has two return values -- 0
on error and 'len' on success.  And it can't have an error because
that only occurs if 'copied' is less than 'length'.

So I think this should actually be:

-               bytes = iomap_write_end(iter, pos, bytes, bytes, folio);
-               if (bytes < 0)
-                       return bytes;
+               status = iomap_write_end(iter, pos, bytes, bytes, folio);
+               if (WARN_ON_ONCE(status == 0))
+                       return -EIO;

just like its counterpart loop in iomap_unshare_iter()

(ok this won't apply to Dan's tree, but YKWIM)

Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Christoph Hellwig]

On Sat, Dec 11, 2021 at 05:50:51PM +0000, Matthew Wilcox wrote:
> On Wed, Dec 08, 2021 at 10:12:03AM +0100, Christoph Hellwig wrote:
> > bytes also hold the return value from iomap_write_end, which can contain
> > a negative error value.  As bytes is always less than the page size even
> > the signed type can hold the entire possible range.
> 
> iomap_write_end() can't return an errno.  I went through and checked as
> part of the folio conversion.  It actually has two return values -- 0
> on error and 'len' on success.  And it can't have an error because
> that only occurs if 'copied' is less than 'length'.
> 
> So I think this should actually be:
> 
> -               bytes = iomap_write_end(iter, pos, bytes, bytes, folio);
> -               if (bytes < 0)
> -                       return bytes;
> +               status = iomap_write_end(iter, pos, bytes, bytes, folio);
> +               if (WARN_ON_ONCE(status == 0))
> +                       return -EIO;
> 
> just like its counterpart loop in iomap_unshare_iter()
> 
> (ok this won't apply to Dan's tree, but YKWIM)

Indeed.  It might make sense to eventually switch to actually return
an errno or a bool as the current calling convention is rather confusing.

Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Matthew Wilcox]

Dan, why is this erroneous commit still in your tree?
iomap_write_end() cannot return an errno; if an error occurs, it
returns zero.  The code in iomap_zero_iter() should be:

                bytes = iomap_write_end(iter, pos, bytes, bytes, page);
                if (WARN_ON_ONCE(bytes == 0))
                        return -EIO;

On Wed, Dec 08, 2021 at 10:12:03AM +0100, Christoph Hellwig wrote:
> bytes also hold the return value from iomap_write_end, which can contain
> a negative error value.  As bytes is always less than the page size even
> the signed type can hold the entire possible range.
> 
> Fixes: c6f40468657d ("fsdax: decouple zeroing from the iomap buffered I/O code")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/iomap/buffered-io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index b1511255b4df8..ac040d607f4fe 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -883,7 +883,7 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  
>  	do {
>  		unsigned offset = offset_in_page(pos);
> -		size_t bytes = min_t(u64, PAGE_SIZE - offset, length);
> +		ssize_t bytes = min_t(u64, PAGE_SIZE - offset, length);
>  		struct page *page;
>  		int status;
>  
> -- 
> 2.30.2
> 

Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t

[Dan Williams]

On Mon, Dec 20, 2021 at 2:10 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> Dan, why is this erroneous commit still in your tree?
> iomap_write_end() cannot return an errno; if an error occurs, it
> returns zero.  The code in iomap_zero_iter() should be:
>
>                 bytes = iomap_write_end(iter, pos, bytes, bytes, page);
>                 if (WARN_ON_ONCE(bytes == 0))
>                         return -EIO;

Care to send a fixup? I'm away from my key at present, but can get it
pushed out later this week.