| Message ID | 20250213135619.1148432-7-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | large atomic writes for xfs with CoW | expand |
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@oracle.com> > --- > 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... > 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. Also, is atomic==true only for the ATOMIC_SW operation? 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. If the answer to the question above is 'yes' then with that nit fixed, Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> --D > { > int error; > bool found; > @@ -578,7 +580,7 @@ xfs_reflink_allocate_cow( > } > > error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found); > - if (error || !*shared) > + if (error || (!*shared && !atomic)) > return error; > > /* CoW fork has a real extent */ > @@ -592,7 +594,7 @@ xfs_reflink_allocate_cow( > */ > if (cmap->br_startoff > imap->br_startoff) > return xfs_reflink_fill_cow_hole(ip, imap, cmap, shared, > - lockmode, convert_now); > + lockmode, convert_now, atomic); > > /* > * CoW fork has a delalloc reservation. Replace it with a real extent. > diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h > index cc4e92278279..754d2bb692d3 100644 > --- a/fs/xfs/xfs_reflink.h > +++ b/fs/xfs/xfs_reflink.h > @@ -32,7 +32,7 @@ int xfs_bmap_trim_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap, > > int xfs_reflink_allocate_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap, > struct xfs_bmbt_irec *cmap, bool *shared, uint *lockmode, > - bool convert_now); > + bool convert_now, bool atomic); > extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset, > xfs_off_t count); > > -- > 2.31.1 > >
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@oracle.com> >> --- >> 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? > 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@kernel.org> Thanks, but I will wait for your feedback to decide whether to pick that up. John
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@oracle.com> > > > --- > > > 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@kernel.org> > > Thanks, but I will wait for your feedback to decide whether to pick that up. > > John >
On 25/02/2025 17:37, Darrick J. Wong wrote: > 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@oracle.com> >>>> --- >>>> 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? We have the odd case of @convert_now being set from IS_DAX(inode) in xfs_direct_write_iomap_begin() -> xfs_reflink_allocate_cow(), so that thwarts the idea of passing the IOMAP flags directly. BTW, it may be possible to clear up that IS_DAX() usage - I'm not sure, so I'll check again. > Or do we make the xfs_iomap.c code translate > that into XFS_REFLINK_ALLOC_* flags? That is what I was thinking of doing. But, as mentioned, it needs to be decided if we pass XFS_REFLINK_ALLOC_* to callees of xfs_reflink_allocate_cow(). I'm thinking 'no', as it will only create churn. > > Either way, that is not something that needs to be done in this patch. Sure Thanks, John
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); 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) { 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) { int error; bool found; @@ -578,7 +580,7 @@ xfs_reflink_allocate_cow( } error = xfs_find_trim_cow_extent(ip, imap, cmap, shared, &found); - if (error || !*shared) + if (error || (!*shared && !atomic)) return error; /* CoW fork has a real extent */ @@ -592,7 +594,7 @@ xfs_reflink_allocate_cow( */ if (cmap->br_startoff > imap->br_startoff) return xfs_reflink_fill_cow_hole(ip, imap, cmap, shared, - lockmode, convert_now); + lockmode, convert_now, atomic); /* * CoW fork has a delalloc reservation. Replace it with a real extent. diff --git a/fs/xfs/xfs_reflink.h b/fs/xfs/xfs_reflink.h index cc4e92278279..754d2bb692d3 100644 --- a/fs/xfs/xfs_reflink.h +++ b/fs/xfs/xfs_reflink.h @@ -32,7 +32,7 @@ int xfs_bmap_trim_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap, int xfs_reflink_allocate_cow(struct xfs_inode *ip, struct xfs_bmbt_irec *imap, struct xfs_bmbt_irec *cmap, bool *shared, uint *lockmode, - bool convert_now); + bool convert_now, bool atomic); extern int xfs_reflink_convert_cow(struct xfs_inode *ip, xfs_off_t offset, xfs_off_t count);
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@oracle.com> --- 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(-)