On Tue, May 28, 2024 at 05:48:28PM +0100, Matthew Wilcox (Oracle) wrote:
> Use an ERR_PTR to return any error that may have occurred, otherwise
> return the folio directly instead of returning it by reference. This
> mirrors changes which are going into the filemap ->write_begin callbacks.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> ---
> fs/iomap/buffered-io.c | 35 ++++++++++++++++-------------------
> 1 file changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index c5802a459334..f0c40ac425ce 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -764,27 +764,27 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
> return iomap_read_inline_data(iter, folio);
> }
>
> -static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
> - size_t len, struct folio **foliop)
> +static struct folio *iomap_write_begin(struct iomap_iter *iter, loff_t pos,
> + size_t len)
> {
> const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
> const struct iomap *srcmap = iomap_iter_srcmap(iter);
> struct folio *folio;
> - int status = 0;
> + int status;
Uninitialised return value.
>
> BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
> if (srcmap != &iter->iomap)
> BUG_ON(pos + len > srcmap->offset + srcmap->length);
>
> if (fatal_signal_pending(current))
> - return -EINTR;
> + return ERR_PTR(-EINTR);
>
> if (!mapping_large_folio_support(iter->inode->i_mapping))
> len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
>
> folio = __iomap_get_folio(iter, pos, len);
> if (IS_ERR(folio))
> - return PTR_ERR(folio);
> + return folio;
>
> /*
> * Now we have a locked folio, before we do anything with it we need to
> @@ -801,7 +801,6 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
> &iter->iomap);
> if (!iomap_valid) {
> iter->iomap.flags |= IOMAP_F_STALE;
> - status = 0;
> goto out_unlock;
> }
> }
That looks wrong - status is now uninitialised when we jump to
the error handling. This case needs to return "no error, no folio"
so that the caller can detect the IOMAP_F_STALE flag and do the
right thing....
> @@ -819,13 +818,12 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
> if (unlikely(status))
> goto out_unlock;
>
> - *foliop = folio;
> - return 0;
> + return folio;
>
> out_unlock:
> __iomap_put_folio(iter, pos, 0, folio);
>
> - return status;
> + return ERR_PTR(status);
This returns the uninitialised status value....
> }
>
> static bool __iomap_write_end(struct inode *inode, loff_t pos, size_t len,
> @@ -940,9 +938,10 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
> break;
> }
>
> - status = iomap_write_begin(iter, pos, bytes, &folio);
> - if (unlikely(status)) {
> + folio = iomap_write_begin(iter, pos, bytes);
> + if (IS_ERR(folio)) {
> iomap_write_failed(iter->inode, pos, bytes);
> + status = PTR_ERR(folio);
> break;
> }
> if (iter->iomap.flags & IOMAP_F_STALE)
So this will now fail the write rather than iterating again at the
same offset with a new iomap.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
