Hi Dave,
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 18c8f168b153..758dc1c90a42 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -289,6 +289,9 @@ xfs_iomap_write_direct(
}
}
+ if (xfs_inode_atomicwrites(ip))
+ bmapi_flags = XFS_BMAPI_ZERO;
We really, really don't want to be doing this during allocation
unless we can avoid it. If the filesystem block size is 64kB, we
could be allocating up to 96GB per extent, and that becomes an
uninterruptable write stream inside a transaction context that holds
inode metadata locked.
Where does that 96GB figure come from?
IOWs, if the inode is already dirty, this data zeroing effectively
pins the tail of the journal until the data writes complete, and
hence can potentially stall the entire filesystem for that length of
time.
Historical note: XFS_BMAPI_ZERO was introduced for DAX where
unwritten extents are not used for initial allocation because the
direct zeroing overhead is typically much lower than unwritten
extent conversion overhead. It was not intended as a general
purpose "zero data at allocation time" solution primarily because of
how easy it would be to DOS the storage with a single, unkillable
fallocate() call on slow storage.
Understood
Why do we want to write zeroes to the disk if we're allocating space
even if we're not sending an atomic write?
(This might want an explanation for why we're doing this at all -- it's
to avoid unwritten extent conversion, which defeats hardware untorn
writes.)
It's to handle the scenario where we have a partially written extent, and
then try to issue an atomic write which covers the complete extent.
When/how would that ever happen with the forcealign bits being set
preventing unaligned allocation and writes?
Consider this scenario:
# mkfs.xfs -r rtdev=/dev/sdb,extsize=64K -d rtinherit=1 /dev/sda
# mount /dev/sda mnt -o rtdev=/dev/sdb
# touch mnt/file
# /test-pwritev2 -a -d -l 4096 -p 0 /root/mnt/file # direct IO, atomic
write, 4096B at pos 0
# filefrag -v mnt/file
Filesystem type is: 58465342
File size of mnt/file is 4096 (1 block of 4096 bytes)
ext: logical_offset: physical_offset: length: expected:
flags:
0: 0.. 0: 24.. 24: 1:
last,eof
mnt/file: 1 extent found
# /test-pwritev2 -a -d -l 16384 -p 0 /root/mnt/file
wrote -1 bytes at pos 0 write_size=16384
#
For the 2nd write, which would cover a 16KB extent, the iomap code will
iter twice and produce 2x BIOs, which we don't want - that's why it
errors there.
With the change in this patch, instead we have something like this after
the first write:
# /test-pwritev2 -a -d -l 4096 -p 0 /root/mnt/file
wrote 4096 bytes at pos 0 write_size=4096
# filefrag -v mnt/file
Filesystem type is: 58465342
File size of mnt/file is 4096 (1 block of 4096 bytes)
ext: logical_offset: physical_offset: length: expected:
flags:
0: 0.. 3: 24.. 27: 4:
last,eof
mnt/file: 1 extent found
#
So the 16KB extent is in written state and the 2nd 16KB write would iter
once, producing a single BIO.
In this
scenario, the iomap code will issue 2x IOs, which is unacceptable. So we
ensure that the extent is completely written whenever we allocate it. At
least that is my idea.
So return an unaligned extent, and then the IOMAP_ATOMIC checks you
add below say "no" and then the application has to do things the
slow, safe way....
We have been porting atomic write support to some database apps and they
(database developers) have had to do something like manually zero the
complete file to get around this issue, but that's not a good user
experience.
Note that in their case the first 4KB write is non-atomic, but that does
not change things. They do these 4KB writes in some DB setup phase.
I think we should support IOCB_ATOMIC when the mapping is unwritten --
the data will land on disk in an untorn fashion, the unwritten extent
conversion on IO completion is itself atomic, and callers still have to
set O_DSYNC to persist anything.
But does this work for the scenario above?
Probably not, but if we want the mapping to return a single
contiguous extent mapping that spans both unwritten and written
states, then we should directly code that behaviour for atomic
IO and not try to hack around it via XFS_BMAPI_ZERO.
Unwritten extent conversion will already do the right thing in that
it will only convert unwritten regions to written in the larger
range that is passed to it, but if there are multiple regions that
need conversion then the conversion won't be atomic.
We would need something atomic.
Then we can avoid the cost of
BMAPI_ZERO, because double-writes aren't free.
About double-writes not being free, I thought that this was acceptable to
just have this write zero when initially allocating the extent as it should
not add too much overhead in practice, i.e. it's one off.
The whole point about atomic writes is they are a performance
optimisation. If the cost of enabling atomic writes is that we
double the amount of IO we are doing, then we've lost more
performance than we gained by using atomic writes. That doesn't
seem desirable....
But the zero'ing is a one off per extent allocation, right? I would
expect just an initial overhead when the database is being created/extended.
Anyway, I did mark this as an RFC for this same reason.
+
error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_write, dblocks,
rblocks, force, &tp);
if (error)
@@ -812,6 +815,44 @@ xfs_direct_write_iomap_begin(
if (error)
goto out_unlock;
+ if (flags & IOMAP_ATOMIC) {
+ xfs_filblks_t unit_min_fsb, unit_max_fsb;
+ unsigned int unit_min, unit_max;
+
+ xfs_get_atomic_write_attr(ip, &unit_min, &unit_max);
+ unit_min_fsb = XFS_B_TO_FSBT(mp, unit_min);
+ unit_max_fsb = XFS_B_TO_FSBT(mp, unit_max);
+
+ if (!imap_spans_range(&imap, offset_fsb, end_fsb)) {
+ error = -EINVAL;
+ goto out_unlock;
+ }
+
+ if ((offset & mp->m_blockmask) ||
+ (length & mp->m_blockmask)) {
+ error = -EINVAL;
+ goto out_unlock;
+ }
That belongs in the iomap DIO setup code, not here. It's also only
checking the data offset/length is filesystem block aligned, not
atomic IO aligned, too.
hmmm... I'm not sure about that. Initially XFS will only support writes
whose size is a multiple of FS block size, and this is what we are
checking here, even if it is not obvious.
The idea is that we can first ensure size is a multiple of FS blocksize,
and then can use br_blockcount directly, below.
+
+ if (imap.br_blockcount == unit_min_fsb ||
+ imap.br_blockcount == unit_max_fsb) {
+ /* ok if exactly min or max */
Why? Exact sizing doesn't imply alignment is correct.
We're not checking alignment specifically, but just checking that the
size is ok.
+ } else if (imap.br_blockcount < unit_min_fsb ||
+ imap.br_blockcount > unit_max_fsb) {
+ error = -EINVAL;
+ goto out_unlock;
Why do this after an exact check?
And this is a continuation of the size check.
+ } else if (!is_power_of_2(imap.br_blockcount)) {
+ error = -EINVAL;
+ goto out_unlock;
Why does this matter? If the extent mapping spans a range larger
than was asked for, who cares what size it is as the infrastructure
is only going to do IO for the sub-range in the mapping the user
asked for....
ok, so where would be a better check for power-of-2 write length? In
iomap DIO code?
I was thinking of doing that, but not so happy with sparse checks.
+ }
+
+ if (imap.br_startoff &&
+ imap.br_startoff & (imap.br_blockcount - 1)) {
Not sure why we care about the file position, it's br_startblock that
gets passed into the bio, not br_startoff.
We just want to ensure that the length of the write is valid w.r.t. to the
offset within the extent, and br_startoff would be the offset within the
aligned extent.
I'm not sure why the filesystem extent mapping code needs to care
about IOMAP_ATOMIC like this - the extent allocation behaviour is
determined by the inode forcealign flag, not IOMAP_ATOMIC.
Everything else we have to do is just mapping the offset/len that
was passed to it from the iomap DIO layer. As long as we allocate
with correct alignment and return a mapping that spans the start
offset of the requested range, we've done our job here.
Actually determining if the mapping returned for IO is suitable for
the type of IO we are doing (i.e. IOMAP_ATOMIC) is the
responsibility of the iomap infrastructure. The same checks will
have to be done for every filesystem that implements atomic writes,
so these checks belong in the generic code, not the filesystem
mapping callouts.
We can move some of these checks to the core iomap code.
However, the core iomap code does not know FS atomic write min and max
per inode, so we need some checks here.
Thanks,
John
