Am Fr., 23. Dez. 2022 um 16:06 Uhr schrieb Christoph Hellwig
<hch@xxxxxxxxxxxxx>:
> 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?
A good reason for a different name is because
folio_may_straddle_isize() requires a locked folio, while
pagecache_isize_extended() will fail if the folio is still locked. So
this doesn't follow the usual "replace 'page' with 'folio'" pattern.
> > 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.
Yes, that's what the horse says.
> > --- 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?
I'm not really sure about that. The boundary conditions in the two
functions are not identical. I think the logic in
folio_may_straddle_isize() is sufficient for the
extending-write-under-folio-lock case, but I'd still need confirmation
for that. If the same logic would also be enough in
pagecache_isize_extended() is more unclear to me.
> > +EXPORT_SYMBOL(folio_may_straddle_isize);
>
> Please make this an EXPORT_SYMBOL_GPL just like folio_mkclean.
Thanks,
Andreas
