On Tue, Feb 25, 2025 at 11:11:45AM +0000, John Garry wrote:
> On 24/02/2025 20:20, Darrick J. Wong wrote:
> > On Thu, Feb 13, 2025 at 01:56:17PM +0000, John Garry wrote:
> > > When completing a CoW-based write, each extent range mapping update is
> > > covered by a separate transaction.
> > >
> > > For a CoW-based atomic write, all mappings must be changed at once, so
> > > change to use a single transaction.
> > >
> > > Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
> > > ---
> > > fs/xfs/xfs_file.c | 5 ++++-
> > > fs/xfs/xfs_reflink.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > > fs/xfs/xfs_reflink.h | 3 +++
> > > 3 files changed, 52 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > index 9762fa503a41..243640fe4874 100644
> > > --- a/fs/xfs/xfs_file.c
> > > +++ b/fs/xfs/xfs_file.c
> > > @@ -527,7 +527,10 @@ xfs_dio_write_end_io(
> > > nofs_flag = memalloc_nofs_save();
> > > if (flags & IOMAP_DIO_COW) {
> > > - error = xfs_reflink_end_cow(ip, offset, size);
> > > + if (iocb->ki_flags & IOCB_ATOMIC)
> > > + error = xfs_reflink_end_atomic_cow(ip, offset, size);
> > > + else
> > > + error = xfs_reflink_end_cow(ip, offset, size);
> > > if (error)
> > > goto out;
> > > }
> > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > > index 3dab3ba900a3..d097d33dc000 100644
> > > --- a/fs/xfs/xfs_reflink.c
> > > +++ b/fs/xfs/xfs_reflink.c
> > > @@ -986,6 +986,51 @@ xfs_reflink_end_cow(
> > > trace_xfs_reflink_end_cow_error(ip, error, _RET_IP_);
> > > return error;
> > > }
> > > +int
> > > +xfs_reflink_end_atomic_cow(
> > > + struct xfs_inode *ip,
> > > + xfs_off_t offset,
> > > + xfs_off_t count)
> > > +{
> > > + xfs_fileoff_t offset_fsb;
> > > + xfs_fileoff_t end_fsb;
> > > + int error = 0;
> > > + struct xfs_mount *mp = ip->i_mount;
> > > + struct xfs_trans *tp;
> > > + unsigned int resblks;
> > > +
> > > + trace_xfs_reflink_end_cow(ip, offset, count);
> > > +
> > > + offset_fsb = XFS_B_TO_FSBT(ip->i_mount, offset);
> > > + end_fsb = XFS_B_TO_FSB(ip->i_mount, offset + count);
> >
> > Use @mp here instead of walking the pointer.
>
> Yes
>
> >
> > > +
> > > + resblks = (end_fsb - offset_fsb) *
> > > + XFS_NEXTENTADD_SPACE_RES(mp, 1, XFS_DATA_FORK);
> >
> > How did you arrive at this computation?
>
> hmmm... you suggested this, but maybe I picked it up incorrectly :)
>
> > The "b" parameter to
> > XFS_NEXTENTADD_SPACE_RES is usually the worst case number of mappings
> > that you're going to change on this file. I think that quantity is
> > (end_fsb - offset_fsb)?
>
> Can you please check this versus what you suggested in
> https://lore.kernel.org/linux-xfs/20250206215014.GX21808@frogsfrogsfrogs/#t
Ah, yeah, that ^^ is correct. This needs a better comment then:
/*
* Each remapping operation could cause a btree split, so in
* the worst case that's one for each block.
*/
resblks = (end_fsb - offset_fsb) *
XFS_NEXTENTADD_SPACE_RES(mp, 1, XFS_DATA_FORK);
--D
> >
> > > +
> > > + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, resblks, 0,
> > > + XFS_TRANS_RESERVE, &tp);
> > > + if (error)
> > > + return error;
> > > +
> > > + xfs_ilock(ip, XFS_ILOCK_EXCL);
> > > + xfs_trans_ijoin(tp, ip, 0);
> > > +
> > > + while (end_fsb > offset_fsb && !error)
> > > + error = xfs_reflink_end_cow_extent_locked(tp, ip, &offset_fsb,
> > > + end_fsb);
> >
> > Overly long line, and the continuation line only needs to be indented
> > two more tabs.
>
> ok
>
> >
> > > +
> > > + if (error) {
> > > + trace_xfs_reflink_end_cow_error(ip, error, _RET_IP_);
> > > + goto out_cancel;
> > > + }
> > > + error = xfs_trans_commit(tp);
> > > + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > > + return 0;
> >
> > Why is it ok to drop @error here? Shouldn't a transaction commit error
> > should be reported to the writer thread?
> >
>
> I can fix that, as I should not ignore errors from xfs_trans_commit()
>
> Thanks,
> John
>
