| Message ID | 20211208091203.2927754-1-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | de291b5902860d18d6e02000808aeb833ec1feb6 |
| Headers | show |
| Series | iomap: turn the byte variable in iomap_zero_iter into a ssize_t | expand |
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 >
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 > >
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.
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
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
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)
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.
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
>
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.
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;
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(-)