Received-SPF: pass (google.com: domain of linux-ext4+bounces-2680-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 29 May 2024 09:44:56 +1000
From: Dave Chinner <david@fromorbit.com>
To: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Christoph Hellwig <hch@lst.de>, linux-fsdevel@vger.kernel.org,
	linux-ext4@vger.kernel.org
Subject: Re: [PATCH 7/7] iomap: Return the folio from iomap_write_begin()
Message-ID: <ZlZseEfwzD0m7bOC@dread.disaster.area>
References: <20240528164829.2105447-1-willy@infradead.org>
 <20240528164829.2105447-8-willy@infradead.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240528164829.2105447-8-willy@infradead.org>

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@infradead.org>
> ---
>  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@fromorbit.com