Re: [RFC v3 1/7] fs: Add folio_may_straddle_isize helper

From: Christoph Hellwig <hch@infradead.org>
To: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	"Darrick J . Wong" <djwong@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Matthew Wilcox <willy@infradead.org>,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-ext4@vger.kernel.org, cluster-devel@redhat.com
Subject: Re: [RFC v3 1/7] fs: Add folio_may_straddle_isize helper
Date: Fri, 23 Dec 2022 06:56:11 -0800	[thread overview]
Message-ID: <Y6XBi/YJ4QV3NK5q@infradead.org> (raw)
In-Reply-To: <20221216150626.670312-2-agruenba@redhat.com>

On Fri, Dec 16, 2022 at 04:06:20PM +0100, Andreas Gruenbacher wrote:
> Add a folio_may_straddle_isize() helper as a replacement for
> pagecache_isize_extended() when we have a locked folio.

I find the naming very confusing.  Any good reason to not follow
the naming of pagecache_isize_extended an call it
folio_isize_extended?

> Use the new helper in generic_write_end(), iomap_write_end(),
> ext4_write_end(), and ext4_journalled_write_end().

Please split this into a patch per caller in addition to the one
adding the helper, and write commit logs explaining the rationale
for the helper.  The obious ones I'm trying to guess are that
the new helper avoid a page cache radix tree lookup and a lock
page/folio cycle, but I'd rather hear that from the horses mouth
in the commit log.

> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2164,16 +2164,15 @@ int generic_write_end(struct file *file, struct address_space *mapping,
>  	 * But it's important to update i_size while still holding page lock:
>  	 * page writeout could otherwise come in and zero beyond i_size.
>  	 */
> -	if (pos + copied > inode->i_size) {
> +	if (pos + copied > old_size) {

This is and unrelated and undocument (but useful) change.  Please split
it out as well.

> + * This function must be called while we still hold i_rwsem - this not only
> + * makes sure i_size is stable but also that userspace cannot observe the new
> + * i_size value before we are prepared to handle mmap writes there.

Please add a lockdep_assert_held_write to enforce that.

> +void folio_may_straddle_isize(struct inode *inode, struct folio *folio,
> +			      loff_t old_size, loff_t start)
> +{
> +	unsigned int blocksize = i_blocksize(inode);
> +
> +	if (round_up(old_size, blocksize) >= round_down(start, blocksize))
> +		return;
> +
> +	/*
> +	 * See clear_page_dirty_for_io() for details why folio_set_dirty()
> +	 * is needed.
> +	 */
> +	if (folio_mkclean(folio))
> +		folio_set_dirty(folio);

Should pagecache_isize_extended be rewritten to use this helper,
i.e. turn this into a factoring out of a helper?

> +EXPORT_SYMBOL(folio_may_straddle_isize);

Please make this an EXPORT_SYMBOL_GPL just like folio_mkclean.