All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
@ 2021-12-08  9:12 Christoph Hellwig
  2021-12-09  0:48 ` Darrick J. Wong
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-12-08  9:12 UTC (permalink / raw)
  To: dan.j.williams; +Cc: linux-fsdevel, linux-xfs, nvdimm, Dan Carpenter

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
  2021-12-08  9:12 [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t Christoph Hellwig
@ 2021-12-09  0:48 ` Darrick J. Wong
  2021-12-09  0:55   ` Darrick J. Wong
  2021-12-11 17:50 ` Matthew Wilcox
  2021-12-20 22:10 ` Matthew Wilcox
  2 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2021-12-09  0:48 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: dan.j.williams, linux-fsdevel, linux-xfs, nvdimm, Dan Carpenter

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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
  2021-12-09  0:48 ` Darrick J. Wong
@ 2021-12-09  0:55   ` Darrick J. Wong
  2021-12-09  1:58     ` Dan Williams
  2021-12-09  6:11     ` Christoph Hellwig
  0 siblings, 2 replies; 10+ messages in thread
From: Darrick J. Wong @ 2021-12-09  0:55 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: dan.j.williams, linux-fsdevel, linux-xfs, nvdimm, Dan Carpenter

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
> > 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
  2021-12-09  0:55   ` Darrick J. Wong
@ 2021-12-09  1:58     ` Dan Williams
  2021-12-09  6:11     ` Christoph Hellwig
  1 sibling, 0 replies; 10+ messages in thread
From: Dan Williams @ 2021-12-09  1:58 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Christoph Hellwig, linux-fsdevel, linux-xfs, Linux NVDIMM, Dan Carpenter

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
  2021-12-09  0:55   ` Darrick J. Wong
  2021-12-09  1:58     ` Dan Williams
@ 2021-12-09  6:11     ` Christoph Hellwig
  2021-12-09 18:19       ` Darrick J. Wong
  1 sibling, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2021-12-09  6:11 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Christoph Hellwig, dan.j.williams, linux-fsdevel, linux-xfs,
	nvdimm, Dan Carpenter

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
  2021-12-09  6:11     ` Christoph Hellwig
@ 2021-12-09 18:19       ` Darrick J. Wong
  0 siblings, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2021-12-09 18:19 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: dan.j.williams, linux-fsdevel, linux-xfs, nvdimm, Dan Carpenter

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
  2021-12-08  9:12 [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t Christoph Hellwig
  2021-12-09  0:48 ` Darrick J. Wong
@ 2021-12-11 17:50 ` Matthew Wilcox
  2021-12-13  7:38   ` Christoph Hellwig
  2021-12-20 22:10 ` Matthew Wilcox
  2 siblings, 1 reply; 10+ messages in thread
From: Matthew Wilcox @ 2021-12-11 17:50 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: dan.j.williams, linux-fsdevel, linux-xfs, nvdimm, Dan Carpenter

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)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
  2021-12-11 17:50 ` Matthew Wilcox
@ 2021-12-13  7:38   ` Christoph Hellwig
  0 siblings, 0 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-12-13  7:38 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Christoph Hellwig, dan.j.williams, linux-fsdevel, linux-xfs,
	nvdimm, Dan Carpenter

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
  2021-12-08  9:12 [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t Christoph Hellwig
  2021-12-09  0:48 ` Darrick J. Wong
  2021-12-11 17:50 ` Matthew Wilcox
@ 2021-12-20 22:10 ` Matthew Wilcox
  2021-12-21  4:12   ` Dan Williams
  2 siblings, 1 reply; 10+ messages in thread
From: Matthew Wilcox @ 2021-12-20 22:10 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: dan.j.williams, linux-fsdevel, linux-xfs, nvdimm, Dan Carpenter

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
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t
  2021-12-20 22:10 ` Matthew Wilcox
@ 2021-12-21  4:12   ` Dan Williams
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Williams @ 2021-12-21  4:12 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Christoph Hellwig, linux-fsdevel, linux-xfs, Linux NVDIMM, Dan Carpenter

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-12-21  4:13 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-08  9:12 [PATCH] iomap: turn the byte variable in iomap_zero_iter into a ssize_t Christoph Hellwig
2021-12-09  0:48 ` Darrick J. Wong
2021-12-09  0:55   ` Darrick J. Wong
2021-12-09  1:58     ` Dan Williams
2021-12-09  6:11     ` Christoph Hellwig
2021-12-09 18:19       ` Darrick J. Wong
2021-12-11 17:50 ` Matthew Wilcox
2021-12-13  7:38   ` Christoph Hellwig
2021-12-20 22:10 ` Matthew Wilcox
2021-12-21  4:12   ` Dan Williams

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.