From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:15
Currently atomic write support for xfs is limited to writing a single
block as we have no way to guarantee alignment and that the write covers
a single extent.
This series introduces a method to issue atomic writes via a software
emulated method.
The software emulated method is used as a fallback for when attempting to
issue an atomic write over misaligned or multiple extents.
The basic idea of this CoW method is to alloc a range in the CoW fork,
write the data, and atomically update the mapping.
Initial mysql performance testing has shown this method to perform ok.
However, there we are only using 16K atomic writes (and 4K block size),
so typically - and thankfully - this software fallback method won't be
used often.
For other FSes which want large atomics writes and don't support CoW, I
think that they can follow the example in [0].
Based on 35010cc72acc (xfs/next-rc) xfs: flush inodegc before swapon
[0] https://lore.kernel.org/linux-xfs/20250102140411.14617-1-john.g.garry@oracle.com/
Differences to RFC:
- Rework CoW alloc method
- Rename IOMAP_ATOMIC -> IOMAP_ATOMIC_HW
- Rework transaction commit func args
- Chaneg resblks size for transaction commit
- Rename BMAPI extszhint align flag
John Garry (10):
iomap: Rename IOMAP_ATOMIC -> IOMAP_ATOMIC_HW
xfs: Switch atomic write size check in xfs_file_write_iter()
xfs: Refactor xfs_reflink_end_cow_extent()
iomap: Support CoW-based atomic writes
xfs: Reflink CoW-based atomic write support
xfs: iomap CoW-based atomic write support
xfs: Add xfs_file_dio_write_atomic()
xfs: Commit CoW-based atomic writes atomically
xfs: Update atomic write max size
xfs: Allow block allocator to take an alignment hint
Ritesh Harjani (IBM) (1):
iomap: Lift blocksize restriction on atomic writes
.../filesystems/iomap/operations.rst | 19 ++-
fs/ext4/inode.c | 2 +-
fs/iomap/direct-io.c | 20 +--
fs/iomap/trace.h | 2 +-
fs/xfs/libxfs/xfs_bmap.c | 7 +-
fs/xfs/libxfs/xfs_bmap.h | 6 +-
fs/xfs/xfs_file.c | 59 +++++++-
fs/xfs/xfs_iomap.c | 74 +++++++++-
fs/xfs/xfs_iops.c | 31 +++-
fs/xfs/xfs_iops.h | 2 +
fs/xfs/xfs_mount.c | 28 ++++
fs/xfs/xfs_mount.h | 1 +
fs/xfs/xfs_reflink.c | 138 +++++++++++++-----
fs/xfs/xfs_reflink.h | 5 +-
include/linux/iomap.h | 8 +-
15 files changed, 331 insertions(+), 71 deletions(-)
--
2.31.1
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:17
Currently the size of atomic write allowed is fixed at the blocksize.
To start to lift this restriction, refactor xfs_get_atomic_write_attr()
to into a helper - xfs_report_atomic_write() - and use that helper to
find the per-inode atomic write limits and check according to that.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_file.c | 12 +++++-------
fs/xfs/xfs_iops.c | 20 +++++++++++++++++---
fs/xfs/xfs_iops.h | 3 +++
3 files changed, 25 insertions(+), 10 deletions(-)
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:18
Refactor xfs_reflink_end_cow_extent() into separate parts which process
the CoW range and commit the transaction.
This refactoring will be used in future for when it is required to commit
a range of extents as a single transaction, similar to how it was done
pre-commit d6f215f359637.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_reflink.c | 73 ++++++++++++++++++++++++++------------------
1 file changed, 43 insertions(+), 30 deletions(-)
@@ -846,14 +830,14 @@ xfs_reflink_end_cow_extent(error=xfs_iext_count_extend(tp,ip,XFS_DATA_FORK,XFS_IEXT_REFLINK_END_COW_CNT);if(error)-gotoout_cancel;+returnerror;/* Grab the corresponding mapping in the data fork. */nmaps=1;error=xfs_bmapi_read(ip,del.br_startoff,del.br_blockcount,&data,&nmaps,0);if(error)-gotoout_cancel;+returnerror;/* We can only remap the smaller of the two extent sizes. */data.br_blockcount=min(data.br_blockcount,del.br_blockcount);
@@ -899,17 +883,46 @@ xfs_reflink_end_cow_extent(/* Remove the mapping from the CoW fork. */xfs_bmap_del_extent_cow(ip,&icur,&got,&del);-error=xfs_trans_commit(tp);-xfs_iunlock(ip,XFS_ILOCK_EXCL);-if(error)-returnerror;-/* Update the caller about how much progress we made. */*offset_fsb=del.br_startoff+del.br_blockcount;return0;+}-out_cancel:-xfs_trans_cancel(tp);++/*+*RemappartoftheCoWforkintothedatafork.+*+*Weaimtoremaptherangestartingat@offset_fsbandendingat@end_fsb+*intothedatafork;thisfunctionwillremapwhatitcan(attheendofthe+*range)andupdate@end_fsbappropriately.Eachremapgetsitsown+*transactionbecausewecanendupmergingandsplittingbmbtblocksfor+*everyremapoperationandwe'dliketokeeptheblockreservation+*requirementsaslowaspossible.+*/+STATICint+xfs_reflink_end_cow_extent(+structxfs_inode*ip,+xfs_fileoff_t*offset_fsb,+xfs_fileoff_tend_fsb)+{+structxfs_mount*mp=ip->i_mount;+structxfs_trans*tp;+unsignedintresblks;+interror;++resblks=XFS_EXTENTADD_SPACE_RES(mp,XFS_DATA_FORK);+error=xfs_trans_alloc(mp,&M_RES(mp)->tr_write,resblks,0,+XFS_TRANS_RESERVE,&tp);+if(error)+returnerror;+xfs_ilock(ip,XFS_ILOCK_EXCL);+xfs_trans_ijoin(tp,ip,0);++error=xfs_reflink_end_cow_extent_locked(tp,ip,offset_fsb,end_fsb);+if(error)+xfs_trans_cancel(tp);+else+error=xfs_trans_commit(tp);xfs_iunlock(ip,XFS_ILOCK_EXCL);returnerror;}
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:18
Currently atomic write support requires dedicated HW support. This imposes
a restriction on the filesystem that disk blocks need to be aligned and
contiguously mapped to FS blocks to issue atomic writes.
XFS has no method to guarantee FS block alignment for regular non-RT files.
As such, atomic writes are currently limited to 1x FS block there.
To allow deal with the scenario that we are issuing an atomic write over
misaligned or discontiguous data blocks larger atomic writes - and raise
the atomic write limit - support a CoW-based software emulated atomic
write mode.
For this special mode, the FS will reserve blocks for that data to be
written and then atomically map that data in once the data has been
committed to disk.
It is the responsibility of the FS to detect discontiguous atomic writes
and switch to IOMAP_DIO_ATOMIC_COW mode and retry the write.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
Documentation/filesystems/iomap/operations.rst | 15 +++++++++++++--
fs/iomap/direct-io.c | 4 +++-
include/linux/iomap.h | 6 ++++++
3 files changed, 22 insertions(+), 3 deletions(-)
@@ -525,8 +525,19 @@ IOMAP_WRITE`` with any combination of the following enhancements: conversion or copy on write), all updates for the entire file range must be committed atomically as well. Only one space mapping is allowed per untorn write.- Untorn writes must be aligned to, and must not be longer than, a- single file block.+ Untorn writes may be longer than a single file block. In all cases,+ the mapping start disk block must have at least the same alignment as+ the write offset.++*``IOMAP_ATOMIC_COW``: This write is being issued with torn-write+ protection based on CoW support.+ All the length, alignment, and single bio restrictions which apply+ to IOMAP_ATOMIC_HW do not apply here.+ CoW-based atomic writes are intended as a fallback for when+ HW-based atomic writes may not be issued, e.g. the range covered in+ the atomic write covers multiple extents.+ All filesystem metadata updates for the entire file range must be+ committed atomically as well. Callers commonly hold ``i_rwsem`` in shared or exclusive mode before calling this function.
@@ -644,7 +644,9 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,iomi.flags|=IOMAP_OVERWRITE_ONLY;}-if(iocb->ki_flags&IOCB_ATOMIC)+if(dio_flags&IOMAP_DIO_ATOMIC_COW)+iomi.flags|=IOMAP_ATOMIC_COW;+elseif(iocb->ki_flags&IOCB_ATOMIC)iomi.flags|=IOMAP_ATOMIC_HW;/* for data sync or sync, we need sync completion processing */
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:18
In future xfs will support a CoW-based atomic write, so rename
IOMAP_ATOMIC -> IOMAP_ATOMIC_HW to be clear which mode is being used.
Also relocate setting of IOMAP_ATOMIC_HW to the write path in
__iomap_dio_rw(), to be clear that this flag is only relevant to writes
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
Documentation/filesystems/iomap/operations.rst | 4 ++--
fs/ext4/inode.c | 2 +-
fs/iomap/direct-io.c | 18 +++++++++---------
fs/iomap/trace.h | 2 +-
include/linux/iomap.h | 2 +-
5 files changed, 14 insertions(+), 14 deletions(-)
@@ -513,8 +513,8 @@ IOMAP_WRITE`` with any combination of the following enhancements: if the mapping is unwritten and the filesystem cannot handle zeroing the unaligned regions without exposing stale contents.-*``IOMAP_ATOMIC``: This write is being issued with torn-write- protection.+*``IOMAP_ATOMIC_HW``: This write is being issued with torn-write+ protection based on HW-offload support. Only a single bio can be created for the write, and the write must not be split into multiple I/O requests, i.e. flag REQ_ATOMIC must be set.
@@ -3467,7 +3467,7 @@ static inline bool ext4_want_directio_fallback(unsigned flags, ssize_t written)returnfalse;/* atomic writes are all-or-nothing */-if(flags&IOMAP_ATOMIC)+if(flags&IOMAP_ATOMIC_HW)returnfalse;/* can only try again if we wrote nothing */
@@ -647,6 +644,9 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,iomi.flags|=IOMAP_OVERWRITE_ONLY;}+if(iocb->ki_flags&IOCB_ATOMIC)+iomi.flags|=IOMAP_ATOMIC_HW;+/* for data sync or sync, we need sync completion processing */if(iocb_is_dsync(iocb)){dio->flags|=IOMAP_DIO_NEED_SYNC;
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:22
In cases of an atomic write occurs for misaligned or discontiguous disk
blocks, we will use a CoW-based method to issue the atomic write.
So, for that case, return -EAGAIN to request that the write be issued in
CoW atomic write mode. The dio write path should detect this, similar to
how misaligned regalar DIO writes are handled.
For normal HW-based mode, when the range which we are atomic writing to
covers a shared data extent, try to allocate a new CoW fork. However, if
we find that what we allocated does not meet atomic write requirements
in terms of length and alignment, then fallback on the CoW-based mode
for the atomic write.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_iomap.c | 72 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 69 insertions(+), 3 deletions(-)
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:22
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(-)
@@ -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)gotoout_unlock;if(shared)
@@ -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))returnerror;/* CoW fork has a real extent */
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:27
Add xfs_file_dio_write_atomic() for dedicated handling of atomic writes.
In case of -EAGAIN being returned from iomap_dio_rw(), reissue the write
in CoW-based atomic write mode.
For CoW-based mode, ensure that we have no outstanding IOs which we
may trample on.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_file.c | 42 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:27
From: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
Filesystems like ext4 can submit writes in multiples of blocksizes.
But we still can't allow the writes to be split. Hence let's check if
the iomap_length() is same as iter->len or not.
It is the role of the FS to ensure that a single mapping may be created
for an atomic write. The FS will also continue to check size and alignment
legality.
Signed-off-by: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
jpg: Tweak commit message
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/iomap/direct-io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:30
When issuing an atomic write by the CoW method, give the block allocator a
hint to naturally align the data blocks.
This means that we have a better chance to issuing the atomic write via
HW offload next time.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 7 ++++++-
fs/xfs/libxfs/xfs_bmap.h | 6 +++++-
fs/xfs/xfs_reflink.c | 8 ++++++--
3 files changed, 17 insertions(+), 4 deletions(-)
@@ -87,6 +87,9 @@ struct xfs_bmalloca {/* Do not update the rmap btree. Used for reconstructing bmbt from rmapbt. */#define XFS_BMAPI_NORMAP (1u << 10)+/* Try to naturally align allocations to extsz hint */+#define XFS_BMAPI_EXTSZALIGN (1u << 11)+#define XFS_BMAPI_FLAGS \{XFS_BMAPI_ENTIRE,"ENTIRE"},\{XFS_BMAPI_METADATA,"METADATA"},\
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:57:33
Now that CoW-based atomic writes are supported, update the max size of an
atomic write.
For simplicity, limit at the max of what the mounted bdev can support in
terms of atomic write limits. Maybe in future we will have a better way
to advertise this optimised limit.
In addition, the max atomic write size needs to be aligned to the agsize.
Limit the size of atomic writes to the greatest power-of-two factor of the
agsize so that allocations for an atomic write will always be aligned
compatibly with the alignment requirements of the storage.
For RT inode, just limit to 1x block, even though larger can be supported
in future.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_iops.c | 13 ++++++++++++-
fs/xfs/xfs_iops.h | 1 -
fs/xfs/xfs_mount.c | 28 ++++++++++++++++++++++++++++
fs/xfs/xfs_mount.h | 1 +
4 files changed, 41 insertions(+), 2 deletions(-)
@@ -606,12 +606,23 @@ xfs_get_atomic_write_attr(unsignedint*unit_min,unsignedint*unit_max){+structxfs_buftarg*target=xfs_inode_buftarg(ip);+structxfs_mount*mp=ip->i_mount;+if(!xfs_inode_can_atomicwrite(ip)){*unit_min=*unit_max=0;return;}-*unit_min=*unit_max=ip->i_mount->m_sb.sb_blocksize;+*unit_min=ip->i_mount->m_sb.sb_blocksize;++if(XFS_IS_REALTIME_INODE(ip)){+/* For now, set limit at 1x block */+*unit_max=ip->i_mount->m_sb.sb_blocksize;+}else{+*unit_max=min_t(unsignedint,XFS_FSB_TO_B(mp,mp->awu_max),+target->bt_bdev_awu_max);+}}staticvoid
@@ -651,6 +651,32 @@ xfs_agbtree_compute_maxlevels(levels=max(levels,mp->m_rmap_maxlevels);mp->m_agbtree_maxlevels=max(levels,mp->m_refc_maxlevels);}+staticinlinevoid+xfs_compute_awu_max(+structxfs_mount*mp)+{+xfs_agblock_tagsize=mp->m_sb.sb_agblocks;+xfs_agblock_tawu_max;++if(!xfs_has_reflink(mp)){+mp->awu_max=1;+return;+}++/*+*Findhighestpower-of-2evenlydivisibleintoagsizeandwhich+*alsofitsintoanunsignedintfield.+*/+awu_max=1;+while(1){+if(agsize%(awu_max*2))+break;+if(XFS_FSB_TO_B(mp,awu_max*2)>UINT_MAX)+break;+awu_max*=2;+}+mp->awu_max=awu_max;+}/* Compute maximum possible height for realtime btree types for this fs. */staticinlinevoid
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-13 13:59:29
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@oracle.com>
---
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(-)
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-20 07:48:48
On 13/02/2025 13:56, John Garry wrote:
Currently atomic write support for xfs is limited to writing a single
block as we have no way to guarantee alignment and that the write covers
a single extent.
This series introduces a method to issue atomic writes via a software
emulated method.
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 19:59:13
On Thu, Feb 13, 2025 at 01:56:12PM +0000, John Garry wrote:
quoted hunk
Currently atomic write support requires dedicated HW support. This imposes
a restriction on the filesystem that disk blocks need to be aligned and
contiguously mapped to FS blocks to issue atomic writes.
XFS has no method to guarantee FS block alignment for regular non-RT files.
As such, atomic writes are currently limited to 1x FS block there.
To allow deal with the scenario that we are issuing an atomic write over
misaligned or discontiguous data blocks larger atomic writes - and raise
the atomic write limit - support a CoW-based software emulated atomic
write mode.
For this special mode, the FS will reserve blocks for that data to be
written and then atomically map that data in once the data has been
committed to disk.
It is the responsibility of the FS to detect discontiguous atomic writes
and switch to IOMAP_DIO_ATOMIC_COW mode and retry the write.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
Documentation/filesystems/iomap/operations.rst | 15 +++++++++++++--
fs/iomap/direct-io.c | 4 +++-
include/linux/iomap.h | 6 ++++++
3 files changed, 22 insertions(+), 3 deletions(-)
@@ -525,8 +525,19 @@ IOMAP_WRITE`` with any combination of the following enhancements: conversion or copy on write), all updates for the entire file range must be committed atomically as well. Only one space mapping is allowed per untorn write.- Untorn writes must be aligned to, and must not be longer than, a- single file block.+ Untorn writes may be longer than a single file block. In all cases,+ the mapping start disk block must have at least the same alignment as+ the write offset.++*``IOMAP_ATOMIC_COW``: This write is being issued with torn-write+ protection based on CoW support.
I think using "COW" here results in a misnamed flag. Consider:
"IOMAP_ATOMIC_SW: This write is being issued with torn-write protection
via a software fallback provided by the filesystem."
iomap itself doesn't care *how* the filesystem guarantees that the
direct write isn't torn, right? The fs' io completion handler has to
ensure that the mapping update(s) are either applied fully or discarded
fully.
In theory if you had a bunch of physical space mapped to the same
file but with different unwritten states, you could gang together all
the unwritten extent conversions in a single transaction, which would
provide the necessary tearing prevention without the out of place write.
Nobody does that right now, but I think that's the only option for ext4.
--D
quoted hunk
+ All the length, alignment, and single bio restrictions which apply+ to IOMAP_ATOMIC_HW do not apply here.+ CoW-based atomic writes are intended as a fallback for when+ HW-based atomic writes may not be issued, e.g. the range covered in+ the atomic write covers multiple extents.+ All filesystem metadata updates for the entire file range must be+ committed atomically as well. Callers commonly hold ``i_rwsem`` in shared or exclusive mode before calling this function.
@@ -644,7 +644,9 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,iomi.flags|=IOMAP_OVERWRITE_ONLY;}-if(iocb->ki_flags&IOCB_ATOMIC)+if(dio_flags&IOMAP_DIO_ATOMIC_COW)+iomi.flags|=IOMAP_ATOMIC_COW;+elseif(iocb->ki_flags&IOCB_ATOMIC)iomi.flags|=IOMAP_ATOMIC_HW;/* for data sync or sync, we need sync completion processing */
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 20:13:33
On Thu, Feb 13, 2025 at 01:56:15PM +0000, John Garry wrote:
quoted hunk
In cases of an atomic write occurs for misaligned or discontiguous disk
blocks, we will use a CoW-based method to issue the atomic write.
So, for that case, return -EAGAIN to request that the write be issued in
CoW atomic write mode. The dio write path should detect this, similar to
how misaligned regalar DIO writes are handled.
For normal HW-based mode, when the range which we are atomic writing to
covers a shared data extent, try to allocate a new CoW fork. However, if
we find that what we allocated does not meet atomic write requirements
in terms of length and alignment, then fallback on the CoW-based mode
for the atomic write.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_iomap.c | 72 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 69 insertions(+), 3 deletions(-)
+ ASSERT(flags & (IOMAP_WRITE | IOMAP_ZERO)); if (xfs_is_shutdown(mp))
@@ -832,7 +857,7 @@ xfs_direct_write_iomap_begin( * COW writes may allocate delalloc space or convert unwritten COW * extents, so we need to make sure to take the lock exclusively here. */- if (xfs_is_cow_inode(ip))+ if (xfs_is_cow_inode(ip) || atomic_cow) lockmode = XFS_ILOCK_EXCL; else lockmode = XFS_ILOCK_SHARED;
@@ -857,6 +882,22 @@ xfs_direct_write_iomap_begin( if (error) goto out_unlock;+ if (flags & IOMAP_ATOMIC_COW) {
if (atomic_cow) ?
Or really, atomic_sw?
quoted hunk
+ error = xfs_reflink_allocate_cow(ip, &imap, &cmap, &shared,+ &lockmode,+ (flags & IOMAP_DIRECT) || IS_DAX(inode), true);+ /*+ * Don't check @shared. For atomic writes, we should error when+ * we don't get a CoW fork.
"Get a CoW fork"? What does that mean? The cow fork should be
automatically allocated when needed, right? Or should this really read
"...when we don't get a COW mapping"?
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 20:20:35
On Thu, Feb 13, 2025 at 01:56:17PM +0000, John Garry wrote:
quoted hunk
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@oracle.com>
---
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(-)
How did you arrive at this computation? 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)?
Why is it ok to drop @error here? Shouldn't a transaction commit error
should be reported to the writer thread?
--D
quoted hunk
+out_cancel:+ xfs_trans_cancel(tp);+ xfs_iunlock(ip, XFS_ILOCK_EXCL);+ return error;+} /* * Free all CoW staging blocks that are still referenced by the ondisk refcount
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 20:23:31
On Thu, Feb 13, 2025 at 01:56:09PM +0000, John Garry wrote:
In future xfs will support a CoW-based atomic write, so rename
IOMAP_ATOMIC -> IOMAP_ATOMIC_HW to be clear which mode is being used.
Also relocate setting of IOMAP_ATOMIC_HW to the write path in
__iomap_dio_rw(), to be clear that this flag is only relevant to writes
Signed-off-by: John Garry <john.g.garry@oracle.com>
Looks fine,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
@@ -513,8 +513,8 @@ IOMAP_WRITE`` with any combination of the following enhancements: if the mapping is unwritten and the filesystem cannot handle zeroing the unaligned regions without exposing stale contents.-*``IOMAP_ATOMIC``: This write is being issued with torn-write- protection.+*``IOMAP_ATOMIC_HW``: This write is being issued with torn-write+ protection based on HW-offload support. Only a single bio can be created for the write, and the write must not be split into multiple I/O requests, i.e. flag REQ_ATOMIC must be set.
@@ -3467,7 +3467,7 @@ static inline bool ext4_want_directio_fallback(unsigned flags, ssize_t written)returnfalse;/* atomic writes are all-or-nothing */-if(flags&IOMAP_ATOMIC)+if(flags&IOMAP_ATOMIC_HW)returnfalse;/* can only try again if we wrote nothing */
@@ -647,6 +644,9 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,iomi.flags|=IOMAP_OVERWRITE_ONLY;}+if(iocb->ki_flags&IOCB_ATOMIC)+iomi.flags|=IOMAP_ATOMIC_HW;+/* for data sync or sync, we need sync completion processing */if(iocb_is_dsync(iocb)){dio->flags|=IOMAP_DIO_NEED_SYNC;
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 20:24:57
On Thu, Feb 13, 2025 at 01:56:10PM +0000, John Garry wrote:
Currently the size of atomic write allowed is fixed at the blocksize.
To start to lift this restriction, refactor xfs_get_atomic_write_attr()
to into a helper - xfs_report_atomic_write() - and use that helper to
find the per-inode atomic write limits and check according to that.
Signed-off-by: John Garry <john.g.garry@oracle.com>
Looks fine,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 20:26:10
On Thu, Feb 13, 2025 at 01:56:11PM +0000, John Garry wrote:
quoted hunk
Refactor xfs_reflink_end_cow_extent() into separate parts which process
the CoW range and commit the transaction.
This refactoring will be used in future for when it is required to commit
a range of extents as a single transaction, similar to how it was done
pre-commit d6f215f359637.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_reflink.c | 73 ++++++++++++++++++++++++++------------------
1 file changed, 43 insertions(+), 30 deletions(-)
@@ -846,14 +830,14 @@ xfs_reflink_end_cow_extent(error=xfs_iext_count_extend(tp,ip,XFS_DATA_FORK,XFS_IEXT_REFLINK_END_COW_CNT);if(error)-gotoout_cancel;+returnerror;/* Grab the corresponding mapping in the data fork. */nmaps=1;error=xfs_bmapi_read(ip,del.br_startoff,del.br_blockcount,&data,&nmaps,0);if(error)-gotoout_cancel;+returnerror;/* We can only remap the smaller of the two extent sizes. */data.br_blockcount=min(data.br_blockcount,del.br_blockcount);
@@ -899,17 +883,46 @@ xfs_reflink_end_cow_extent(/* Remove the mapping from the CoW fork. */xfs_bmap_del_extent_cow(ip,&icur,&got,&del);-error=xfs_trans_commit(tp);-xfs_iunlock(ip,XFS_ILOCK_EXCL);-if(error)-returnerror;-/* Update the caller about how much progress we made. */*offset_fsb=del.br_startoff+del.br_blockcount;return0;+}-out_cancel:-xfs_trans_cancel(tp);++/*+*RemappartoftheCoWforkintothedatafork.+*+*Weaimtoremaptherangestartingat@offset_fsbandendingat@end_fsb+*intothedatafork;thisfunctionwillremapwhatitcan(attheendofthe+*range)andupdate@end_fsbappropriately.Eachremapgetsitsown+*transactionbecausewecanendupmergingandsplittingbmbtblocksfor+*everyremapoperationandwe'dliketokeeptheblockreservation+*requirementsaslowaspossible.+*/+STATICint+xfs_reflink_end_cow_extent(+structxfs_inode*ip,+xfs_fileoff_t*offset_fsb,+xfs_fileoff_tend_fsb)+{+structxfs_mount*mp=ip->i_mount;+structxfs_trans*tp;+unsignedintresblks;+interror;++resblks=XFS_EXTENTADD_SPACE_RES(mp,XFS_DATA_FORK);+error=xfs_trans_alloc(mp,&M_RES(mp)->tr_write,resblks,0,+XFS_TRANS_RESERVE,&tp);+if(error)+returnerror;+xfs_ilock(ip,XFS_ILOCK_EXCL);+xfs_trans_ijoin(tp,ip,0);++error=xfs_reflink_end_cow_extent_locked(tp,ip,offset_fsb,end_fsb);
Overly long line, but otherwise looks fine. With that fixed,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 20:32:28
On Thu, Feb 13, 2025 at 01:56:14PM +0000, John Garry wrote:
quoted hunk
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(-)
@@ -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...
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
quoted hunk
{
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.
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 20:32:44
On Thu, Feb 13, 2025 at 01:56:16PM +0000, John Garry wrote:
Add xfs_file_dio_write_atomic() for dedicated handling of atomic writes.
In case of -EAGAIN being returned from iomap_dio_rw(), reissue the write
in CoW-based atomic write mode.
For CoW-based mode, ensure that we have no outstanding IOs which we
may trample on.
Signed-off-by: John Garry <john.g.garry@oracle.com>
Looks fine,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 20:34:39
On Thu, Feb 13, 2025 at 01:56:18PM +0000, John Garry wrote:
quoted hunk
Now that CoW-based atomic writes are supported, update the max size of an
atomic write.
For simplicity, limit at the max of what the mounted bdev can support in
terms of atomic write limits. Maybe in future we will have a better way
to advertise this optimised limit.
In addition, the max atomic write size needs to be aligned to the agsize.
Limit the size of atomic writes to the greatest power-of-two factor of the
agsize so that allocations for an atomic write will always be aligned
compatibly with the alignment requirements of the storage.
For RT inode, just limit to 1x block, even though larger can be supported
in future.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_iops.c | 13 ++++++++++++-
fs/xfs/xfs_iops.h | 1 -
fs/xfs/xfs_mount.c | 28 ++++++++++++++++++++++++++++
fs/xfs/xfs_mount.h | 1 +
4 files changed, 41 insertions(+), 2 deletions(-)
@@ -606,12 +606,23 @@ xfs_get_atomic_write_attr(unsignedint*unit_min,unsignedint*unit_max){+structxfs_buftarg*target=xfs_inode_buftarg(ip);+structxfs_mount*mp=ip->i_mount;+if(!xfs_inode_can_atomicwrite(ip)){*unit_min=*unit_max=0;return;}-*unit_min=*unit_max=ip->i_mount->m_sb.sb_blocksize;+*unit_min=ip->i_mount->m_sb.sb_blocksize;++if(XFS_IS_REALTIME_INODE(ip)){+/* For now, set limit at 1x block */+*unit_max=ip->i_mount->m_sb.sb_blocksize;+}else{+*unit_max=min_t(unsignedint,XFS_FSB_TO_B(mp,mp->awu_max),+target->bt_bdev_awu_max);+}}staticvoid
@@ -651,6 +651,32 @@ xfs_agbtree_compute_maxlevels(levels=max(levels,mp->m_rmap_maxlevels);mp->m_agbtree_maxlevels=max(levels,mp->m_refc_maxlevels);}+staticinlinevoid+xfs_compute_awu_max(+structxfs_mount*mp)+{+xfs_agblock_tagsize=mp->m_sb.sb_agblocks;+xfs_agblock_tawu_max;++if(!xfs_has_reflink(mp)){+mp->awu_max=1;+return;+}++/*+*Findhighestpower-of-2evenlydivisibleintoagsizeandwhich+*alsofitsintoanunsignedintfield.+*/+awu_max=1;+while(1){+if(agsize%(awu_max*2))+break;+if(XFS_FSB_TO_B(mp,awu_max*2)>UINT_MAX)+break;+awu_max*=2;+}+mp->awu_max=awu_max;+}/* Compute maximum possible height for realtime btree types for this fs. */staticinlinevoid
@@ -198,6 +198,7 @@ typedef struct xfs_mount {boolm_fail_unmount;boolm_finobt_nores;/* no per-AG finobt resv. */boolm_update_sb;/* sb needs update in mount */+xfs_extlen_tawu_max;/* max atomic write */
Might want to clarify that this is for the *data* device.
/* max atomic write to datadev */
With those two things fixed,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
/*
* Bitsets of per-fs metadata that have been checked and/or are sick.
--
2.31.1
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-24 20:37:38
On Thu, Feb 13, 2025 at 01:56:19PM +0000, John Garry wrote:
quoted hunk
When issuing an atomic write by the CoW method, give the block allocator a
hint to naturally align the data blocks.
This means that we have a better chance to issuing the atomic write via
HW offload next time.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 7 ++++++-
fs/xfs/libxfs/xfs_bmap.h | 6 +++++-
fs/xfs/xfs_reflink.c | 8 ++++++--
3 files changed, 17 insertions(+), 4 deletions(-)
@@ -87,6 +87,9 @@ struct xfs_bmalloca {/* Do not update the rmap btree. Used for reconstructing bmbt from rmapbt. */#define XFS_BMAPI_NORMAP (1u << 10)+/* Try to naturally align allocations to extsz hint */+#define XFS_BMAPI_EXTSZALIGN (1u << 11)
IMO "naturally" makes things confusing here -- are we aligning to the
extent size hint, or are we aligning to the length requested? Or
whatever it is that "naturally" means.
(FWIW you and I have bumped over this repeatedly, so maybe this is
simple one of those cognitive friction things where block storage always
deals with powers of two and "naturally" means a lot, vs. filesystems
where we don't usually enforce alignment anywhere.)
I suggest "Try to align allocations to the extent size hint" for the
comment, and with that:
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-25 10:20:13
On 24/02/2025 19:59, Darrick J. Wong wrote:
quoted
+ * ``IOMAP_ATOMIC_COW``: This write is being issued with torn-write
+ protection based on CoW support.
I think using "COW" here results in a misnamed flag. Consider:
"IOMAP_ATOMIC_SW:
ok, fine
This write is being issued with torn-write protection
via a software fallback provided by the filesystem."
I'm not sure that we really need to even mention software fallback. Indeed, xfs could just use IOMAP_ATOMIC_SW always when the bdev does not support HW offload. Maybe I can mention that typically it can be used as a software fallback when HW offload is not possible.
iomap itself doesn't care*how* the filesystem guarantees that the
direct write isn't torn, right?
Correct. iomap just ensures that for IOMAP_ATOMIC_HW we produce a single bio - that's the only check really.
The fs' io completion handler has to
ensure that the mapping update(s) are either applied fully or discarded
fully.
right
In theory if you had a bunch of physical space mapped to the same
file but with different unwritten states, you could gang together all
the unwritten extent conversions in a single transaction, which would
provide the necessary tearing prevention without the out of place write.
Nobody does that right now, but I think that's the only option for ext4.
ok, maybe. But ext4 still does have bigalloc or opportunity to support forcealign (to always use IOMAP_ATOMIC_HW for large untorn writes).
Thanks,
John
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-25 10:59:53
On 24/02/2025 20:32, Darrick J. Wong wrote:
On Thu, Feb 13, 2025 at 01:56:14PM +0000, John Garry wrote:
quoted
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(-)
@@ -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?
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-25 11:07:09
On 24/02/2025 20:13, Darrick J. Wong wrote:
On Thu, Feb 13, 2025 at 01:56:15PM +0000, John Garry wrote:
quoted
In cases of an atomic write occurs for misaligned or discontiguous disk
blocks, we will use a CoW-based method to issue the atomic write.
So, for that case, return -EAGAIN to request that the write be issued in
CoW atomic write mode. The dio write path should detect this, similar to
how misaligned regalar DIO writes are handled.
For normal HW-based mode, when the range which we are atomic writing to
covers a shared data extent, try to allocate a new CoW fork. However, if
we find that what we allocated does not meet atomic write requirements
in terms of length and alignment, then fallback on the CoW-based mode
for the atomic write.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_iomap.c | 72 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 69 insertions(+), 3 deletions(-)
+ ASSERT(flags & (IOMAP_WRITE | IOMAP_ZERO)); if (xfs_is_shutdown(mp))
@@ -832,7 +857,7 @@ xfs_direct_write_iomap_begin( * COW writes may allocate delalloc space or convert unwritten COW * extents, so we need to make sure to take the lock exclusively here. */- if (xfs_is_cow_inode(ip))+ if (xfs_is_cow_inode(ip) || atomic_cow) lockmode = XFS_ILOCK_EXCL; else lockmode = XFS_ILOCK_SHARED;
@@ -857,6 +882,22 @@ xfs_direct_write_iomap_begin( if (error) goto out_unlock;+ if (flags & IOMAP_ATOMIC_COW) {
if (atomic_cow) ?
Or really, atomic_sw?
Yes, will change.
quoted
+ error = xfs_reflink_allocate_cow(ip, &imap, &cmap, &shared,+ &lockmode,+ (flags & IOMAP_DIRECT) || IS_DAX(inode), true);+ /*+ * Don't check @shared. For atomic writes, we should error when+ * we don't get a CoW fork.
"Get a CoW fork"? What does that mean? The cow fork should be
automatically allocated when needed, right? Or should this really read
"...when we don't get a COW mapping"?
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-25 11:12:01
On 24/02/2025 20:20, Darrick J. Wong wrote:
On Thu, Feb 13, 2025 at 01:56:17PM +0000, John Garry wrote:
quoted
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@oracle.com>
---
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(-)
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)?
@@ -651,6 +651,32 @@ xfs_agbtree_compute_maxlevels(levels=max(levels,mp->m_rmap_maxlevels);mp->m_agbtree_maxlevels=max(levels,mp->m_refc_maxlevels);}+staticinlinevoid+xfs_compute_awu_max(+structxfs_mount*mp)+{+xfs_agblock_tagsize=mp->m_sb.sb_agblocks;+xfs_agblock_tawu_max;++if(!xfs_has_reflink(mp)){+mp->awu_max=1;+return;+}++/*+*Findhighestpower-of-2evenlydivisibleintoagsizeandwhich+*alsofitsintoanunsignedintfield.+*/+awu_max=1;+while(1){+if(agsize%(awu_max*2))+break;+if(XFS_FSB_TO_B(mp,awu_max*2)>UINT_MAX)+break;+awu_max*=2;+}+mp->awu_max=awu_max;+}/* Compute maximum possible height for realtime btree types for this fs. */staticinlinevoid
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-25 11:17:18
On 24/02/2025 20:37, Darrick J. Wong wrote:
On Thu, Feb 13, 2025 at 01:56:19PM +0000, John Garry wrote:
quoted
When issuing an atomic write by the CoW method, give the block allocator a
hint to naturally align the data blocks.
This means that we have a better chance to issuing the atomic write via
HW offload next time.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/libxfs/xfs_bmap.c | 7 ++++++-
fs/xfs/libxfs/xfs_bmap.h | 6 +++++-
fs/xfs/xfs_reflink.c | 8 ++++++--
3 files changed, 17 insertions(+), 4 deletions(-)
@@ -87,6 +87,9 @@ struct xfs_bmalloca {/* Do not update the rmap btree. Used for reconstructing bmbt from rmapbt. */#define XFS_BMAPI_NORMAP (1u << 10)+/* Try to naturally align allocations to extsz hint */+#define XFS_BMAPI_EXTSZALIGN (1u << 11)
IMO "naturally" makes things confusing here -- are we aligning to the
extent size hint, or are we aligning to the length requested? Or
whatever it is that "naturally" means.
We align to extsz hint, not length.
As for use of word "naturally", I'll try to avoid using that word.
(FWIW you and I have bumped over this repeatedly, so maybe this is
simple one of those cognitive friction things where block storage always
deals with powers of two and "naturally" means a lot, vs. filesystems
where we don't usually enforce alignment anywhere.)
I suggest "Try to align allocations to the extent size hint" for the
comment, and with that:
that's fine
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-25 17:33:56
On Tue, Feb 25, 2025 at 10:19:49AM +0000, John Garry wrote:
On 24/02/2025 19:59, Darrick J. Wong wrote:
quoted
quoted
+ * ``IOMAP_ATOMIC_COW``: This write is being issued with torn-write
+ protection based on CoW support.
I think using "COW" here results in a misnamed flag. Consider:
"IOMAP_ATOMIC_SW:
ok, fine
quoted
This write is being issued with torn-write protection
via a software fallback provided by the filesystem."
I'm not sure that we really need to even mention software fallback. Indeed,
xfs could just use IOMAP_ATOMIC_SW always when the bdev does not support HW
offload. Maybe I can mention that typically it can be used as a software
fallback when HW offload is not possible.
Ok, a software mechanism then.
quoted
iomap itself doesn't care*how* the filesystem guarantees that the
direct write isn't torn, right?
Correct. iomap just ensures that for IOMAP_ATOMIC_HW we produce a single bio
- that's the only check really.
quoted
The fs' io completion handler has to
ensure that the mapping update(s) are either applied fully or discarded
fully.
right
quoted
In theory if you had a bunch of physical space mapped to the same
file but with different unwritten states, you could gang together all
the unwritten extent conversions in a single transaction, which would
provide the necessary tearing prevention without the out of place write.
Nobody does that right now, but I think that's the only option for ext4.
ok, maybe. But ext4 still does have bigalloc or opportunity to support
forcealign (to always use IOMAP_ATOMIC_HW for large untorn writes).
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-25 17:37:51
On Tue, Feb 25, 2025 at 10:58:56AM +0000, John Garry wrote:
On 24/02/2025 20:32, Darrick J. Wong wrote:
quoted
On Thu, Feb 13, 2025 at 01:56:14PM +0000, John Garry wrote:
quoted
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(-)
@@ -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?
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.
quoted
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.
quoted
I think so, but that's the unfortunate thing about
booleans.
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-25 17:47:28
On Tue, Feb 25, 2025 at 11:06:50AM +0000, John Garry wrote:
On 24/02/2025 20:13, Darrick J. Wong wrote:
quoted
On Thu, Feb 13, 2025 at 01:56:15PM +0000, John Garry wrote:
quoted
In cases of an atomic write occurs for misaligned or discontiguous disk
blocks, we will use a CoW-based method to issue the atomic write.
So, for that case, return -EAGAIN to request that the write be issued in
CoW atomic write mode. The dio write path should detect this, similar to
how misaligned regalar DIO writes are handled.
For normal HW-based mode, when the range which we are atomic writing to
covers a shared data extent, try to allocate a new CoW fork. However, if
we find that what we allocated does not meet atomic write requirements
in terms of length and alignment, then fallback on the CoW-based mode
for the atomic write.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
fs/xfs/xfs_iomap.c | 72 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 69 insertions(+), 3 deletions(-)
+ ASSERT(flags & (IOMAP_WRITE | IOMAP_ZERO)); if (xfs_is_shutdown(mp))
@@ -832,7 +857,7 @@ xfs_direct_write_iomap_begin( * COW writes may allocate delalloc space or convert unwritten COW * extents, so we need to make sure to take the lock exclusively here. */- if (xfs_is_cow_inode(ip))+ if (xfs_is_cow_inode(ip) || atomic_cow) lockmode = XFS_ILOCK_EXCL; else lockmode = XFS_ILOCK_SHARED;
@@ -857,6 +882,22 @@ xfs_direct_write_iomap_begin( if (error) goto out_unlock;+ if (flags & IOMAP_ATOMIC_COW) {
if (atomic_cow) ?
Or really, atomic_sw?
Yes, will change.
quoted
quoted
+ error = xfs_reflink_allocate_cow(ip, &imap, &cmap, &shared,+ &lockmode,+ (flags & IOMAP_DIRECT) || IS_DAX(inode), true);+ /*+ * Don't check @shared. For atomic writes, we should error when+ * we don't get a CoW fork.
"Get a CoW fork"? What does that mean? The cow fork should be
automatically allocated when needed, right? Or should this really read
"...when we don't get a COW mapping"?
From: "Darrick J. Wong" <djwong@kernel.org> Date: 2025-02-25 17:50:15
On Tue, Feb 25, 2025 at 11:11:45AM +0000, John Garry wrote:
On 24/02/2025 20:20, Darrick J. Wong wrote:
quoted
On Thu, Feb 13, 2025 at 01:56:17PM +0000, John Garry wrote:
quoted
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@oracle.com>
---
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(-)
hmmm... you suggested this, but maybe I picked it up incorrectly :)
quoted
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)?
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
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-25 18:02:34
On 25/02/2025 17:37, Darrick J. Wong wrote:
On Tue, Feb 25, 2025 at 10:58:56AM +0000, John Garry wrote:
quoted
On 24/02/2025 20:32, Darrick J. Wong wrote:
quoted
On Thu, Feb 13, 2025 at 01:56:14PM +0000, John Garry wrote:
quoted
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(-)
@@ -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?
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.
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-25 18:07:27
On 25/02/2025 17:47, Darrick J. Wong wrote:
quoted
I can try, and would then need to try to factor out what would be much
duplicated code.
<nod> I think it's pretty straightforward:
Yeah, I already had done sometime like this since.
xfs_direct_cow_write_iomap_begin()
{
ASSERT(flags & IOMAP_WRITE);
ASSERT(flags & IOMAP_DIRECT);
ASSERT(flags & IOMAP_ATOMIC_SW);
if (xfs_is_shutdown(mp))
return -EIO;
/*
* Writes that span EOF might trigger an IO size update on
* completion, so consider them to be dirty for the purposes of
* O_DSYNC even if there is no other metadata changes pending or
* have been made here.
*/
if (offset + length > i_size_read(inode))
iomap_flags |= IOMAP_F_DIRTY;
lockmode = XFS_ILOCK_EXCL;
error = xfs_ilock_for_iomap(ip, flags, &lockmode);
if (error)
return error;
error = xfs_bmapi_read(ip, offset_fsb, end_fsb - offset_fsb,
&imap, &nimaps, 0);
if (error)
goto out_unlock;
error = xfs_reflink_allocate_cow(ip, &imap, &cmap, &shared,
&lockmode, true, true);
if (error)
goto out_unlock;
endoff = XFS_FSB_TO_B(mp, cmap.br_startoff + cmap.br_blockcount);
trace_xfs_iomap_found(ip, offset, endoff - offset, XFS_COW_FORK,
&cmap);
if (imap.br_startblock != HOLESTARTBLOCK) {
note: As you know, all this is common to xfs_direct_write_iomap_begin(), but unfortunately can't neatly be factored out due to the xfs_iunlock() calls.
From: John Garry <john.g.garry@oracle.com> Date: 2025-02-25 18:07:58
On 25/02/2025 17:50, Darrick J. Wong wrote:
quoted
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);