On Tue, Feb 25, 2025 at 10:58:56AM +0000, John Garry wrote:
> On 24/02/2025 20:32, Darrick J. Wong wrote:
> > On Thu, Feb 13, 2025 at 01:56:14PM +0000, John Garry wrote:
> > > For CoW-based atomic write support, always allocate a cow hole in
> > > xfs_reflink_allocate_cow() to write the new data.
> > >
> > > The semantics is that if @atomic is set, we will be passed a CoW fork
> > > extent mapping for no error returned.
> > >
> > > Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
> > > ---
> > > fs/xfs/xfs_iomap.c | 2 +-
> > > fs/xfs/xfs_reflink.c | 12 +++++++-----
> > > fs/xfs/xfs_reflink.h | 2 +-
> > > 3 files changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > > index d61460309a78..ab79f0080288 100644
> > > --- a/fs/xfs/xfs_iomap.c
> > > +++ b/fs/xfs/xfs_iomap.c
> > > @@ -865,7 +865,7 @@ xfs_direct_write_iomap_begin(
> > > /* may drop and re-acquire the ilock */
> > > error = xfs_reflink_allocate_cow(ip, &imap, &cmap, &shared,
> > > &lockmode,
> > > - (flags & IOMAP_DIRECT) || IS_DAX(inode));
> > > + (flags & IOMAP_DIRECT) || IS_DAX(inode), false);
> >
> > Now I'm /really/ think it's time for some reflink allocation flags,
> > because the function signature now involves two booleans...
>
> ok, but the @convert_now arg is passed to other functions from
> xfs_reflink_allocate_cow() - so would you prefer to create a bool
> @convert_now inside xfs_reflink_allocate_cow() and pass that bool as before?
> Or pass the flags all the way down to end users of @convert_now?
>
> >
> > > if (error)
> > > goto out_unlock;
> > > if (shared)
> > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > > index 8428f7b26ee6..3dab3ba900a3 100644
> > > --- a/fs/xfs/xfs_reflink.c
> > > +++ b/fs/xfs/xfs_reflink.c
> > > @@ -435,7 +435,8 @@ xfs_reflink_fill_cow_hole(
> > > struct xfs_bmbt_irec *cmap,
> > > bool *shared,
> > > uint *lockmode,
> > > - bool convert_now)
> > > + bool convert_now,
> > > + bool atomic)
> >
> > ...but this can come later.
>
> Do you mean that this would just be a new flag to set?
Sorry, I meant that the double booleans -> flags conversion could be a
cleanup patch at the end of the series. But first we'd have to figure
out where we want the flags boundaries to be -- do we just pass the
IOMAP_{DIRECT,DAX,ATOMIC_*} flags directly to the reflink code and let
it figure out what to do? Or do we make the xfs_iomap.c code translate
that into XFS_REFLINK_ALLOC_* flags?
Either way, that is not something that needs to be done in this patch.
> > Also, is atomic==true only for the> ATOMIC_SW operation?
>
> Right, so I think that the variable (or new flag) can be renamed for that.
>
> > I think so, but that's the unfortunate thing about
> > booleans.
> >
> > > {
> > > struct xfs_mount *mp = ip->i_mount;
> > > struct xfs_trans *tp;
> > > @@ -466,7 +467,7 @@ xfs_reflink_fill_cow_hole(
> > > *lockmode = XFS_ILOCK_EXCL;
> > > error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found);
> > > - if (error || !*shared)
> > > + if (error || (!*shared && !atomic))
> > > goto out_trans_cancel;
> > > if (found) {
> > > @@ -566,7 +567,8 @@ xfs_reflink_allocate_cow(
> > > struct xfs_bmbt_irec *cmap,
> > > bool *shared,
> > > uint *lockmode,
> > > - bool convert_now)
> > > + bool convert_now,
> > > + bool atomic)
> >
> > Nit: ^ space before tab.
>
> ok
>
> >
> > If the answer to the question above is 'yes' then with that nit fixed,
> > Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
>
> Thanks, but I will wait for your feedback to decide whether to pick that up.
>
> John
>
