Re: [PATCH 02/10] iomap: remove iomap_file_buffered_write_punch_delalloc

[Dave Chinner <david@fromorbit.com>]

On Mon, Sep 23, 2024 at 09:18:25AM -0700, Darrick J. Wong wrote:
> On Mon, Sep 23, 2024 at 05:28:16PM +0200, Christoph Hellwig wrote:
> > Currently iomap_file_buffered_write_punch_delalloc can be called from
> > XFS either with the invalidate lock held or not.  To fix this while
> > keeping the locking in the file system and not the iomap library
> > code we'll need to life the locking up into the file system.
> > 
> > To prepare for that, open code iomap_file_buffered_write_punch_delalloc
> > in the only caller, and instead export iomap_write_delalloc_release.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  .../filesystems/iomap/operations.rst          |  2 +-
> >  fs/iomap/buffered-io.c                        | 85 ++++++-------------
> >  fs/xfs/xfs_iomap.c                            | 16 +++-
> >  include/linux/iomap.h                         |  6 +-
> >  4 files changed, 46 insertions(+), 63 deletions(-)
> > 
> > diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst
> > index 8e6c721d233010..b93115ab8748ae 100644
> > --- a/Documentation/filesystems/iomap/operations.rst
> > +++ b/Documentation/filesystems/iomap/operations.rst
> > @@ -208,7 +208,7 @@ The filesystem must arrange to `cancel
> >  such `reservations
> >  <https://lore.kernel.org/linux-xfs/20220817093627.GZ3600936@dread.disaster.area/>`_
> >  because writeback will not consume the reservation.
> > -The ``iomap_file_buffered_write_punch_delalloc`` can be called from a
> > +The ``iomap_write_delalloc_release`` can be called from a
> >  ``->iomap_end`` function to find all the clean areas of the folios
> >  caching a fresh (``IOMAP_F_NEW``) delalloc mapping.
> >  It takes the ``invalidate_lock``.
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index 884891ac7a226c..237aeb883166df 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -1149,6 +1149,32 @@ static void iomap_write_delalloc_scan(struct inode *inode,
> >   * have dirty data still pending in the page cache - those are going to be
> >   * written and so must still retain the delalloc backing for writeback.
> >   *
> > + * When a short write occurs, the filesystem may need to remove reserved space
> > + * that was allocated in ->iomap_begin from it's ->iomap_end method. For
> 
> "When a short write occurs, the filesystem may need to remove space
> reservations created in ->iomap_begin.
> 
> > + * filesystems that use delayed allocation, we need to punch out delalloc
> > + * extents from the range that are not dirty in the page cache. As the write can
> > + * race with page faults, there can be dirty pages over the delalloc extent
> > + * outside the range of a short write but still within the delalloc extent
> > + * allocated for this iomap.
> > + *
> > + * The punch() callback *must* only punch delalloc extents in the range passed
> > + * to it. It must skip over all other types of extents in the range and leave
> > + * them completely unchanged. It must do this punch atomically with respect to
> > + * other extent modifications.
> 
> Can a failing buffered write race with a write fault to the same file
> range?

Yes.

> write() thread:			page_mkwrite thread:
> ---------------			--------------------
> take i_rwsem
> ->iomap_begin
> create da reservation
> lock folio
> fail to write
> unlock folio
> 				take invalidation lock
> 				lock folio
> 				->iomap_begin
> 				sees da reservation
> 				mark folio dirty
> 				unlock folio
> 				drop invalidation lock
> ->iomap_end
> take invalidation lock
> iomap_write_delalloc_release
> drop invalidation lock
> 
> Can we end up in this situation, where the write fault thinks it has a
> dirty page backed by a delalloc reservation, yet the delalloc
> reservation gets removed by the delalloc punch logic? 

No.

> I think the
> answer to my question is that this sequence is impossible because the
> write fault dirties the folio so the iomap_write_delalloc_release does
> nothing, correct?

Yes.

The above situation is the race condition that the delalloc punching
code is taking into account when it checks for dirty data over the
range being punched. As the comment above
iomap_write_delalloc_release() says:

/*
 * Punch out all the delalloc blocks in the range given except for those that
 * have dirty data still pending in the page cache - those are going to be
 * written and so must still retain the delalloc backing for writeback.
 *
....

-Dave.
-- 
Dave Chinner
david@fromorbit.com