Message-ID: <a20b3c07-605e-44c2-b562-e98269d37558@oracle.com>
Date: Wed, 7 Feb 2024 14:13:23 +0000
From: John Garry <john.g.garry@...cle.com>
To: Dave Chinner <david@...morbit.com>
Cc: "Darrick J. Wong" <djwong@...nel.org>, hch@....de, viro@...iv.linux.org.uk,
        brauner@...nel.org, dchinner@...hat.com, jack@...e.cz,
        chandan.babu@...cle.com, martin.petersen@...cle.com,
        linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, tytso@....edu, jbongio@...gle.com,
        ojaswin@...ux.ibm.com
Subject: Re: [PATCH RFC 5/6] fs: xfs: iomap atomic write support

On 07/02/2024 00:06, Dave Chinner wrote:
>>> 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?
> My inability to do math. The actual number is 128GB.
> 
> Max extent size = XFS_MAX_BMBT_EXTLEN * fs block size.
> 	        = 2^21  * fs block size.
> 
> So for a 4kB block size filesystem, that's 8GB max extent length,
> and that's the most we will allocate in a single transaction (i.e.
> one new BMBT record).
> 
> For 64kB block size, we can get 128GB of space allocated in a single
> transaction.

atomic write unit max theoretical upper limit is 
rounddown_power_of_2(2^32 - 1) = 2GB

So this would be what is expected to be the largest extent size 
requested for atomic writes. I am not saying that 2GB is small, but 
certainly much smaller than 128GB.

> 
>>>>> 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
> Please don't write one-off custom test programs to issue IO - please
> use and enhance xfs_io so the test cases can then be put straight
> into fstests without adding yet another "do some minor IO variant"
> test program. This also means you don't need a random assortment of
> other tools.
> 
> i.e.
> 
> # xfs_io -dc "pwrite -VA 0 4096" /root/mnt/file
> 
> Should do an RWF_ATOMIC IO, and
> 
> # xfs_io -dc "pwrite -VAD 0 4096" /root/mnt/file
> 
> should do an RWF_ATOMIC|RWF_DSYNC IO...
> 
> 
>> # filefrag -v mnt/file
> xfs_io -c "fiemap" mnt/file

Fine, but I like using something generic for accessing block devices and 
also other FSes. I didn't think that xfs_io can do that.

Anyway, we can look to add atomic write support to xfs_io and any other 
xfs-progs

> 
>> 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
>> #
> Whole test as one repeatable command:
> 
> # xfs_io -d -c "truncate 0" -c "chattr +r" \
> 	-c "pwrite -VAD 0 4096" \
> 	-c "fiemap" \
> 	-c "pwrite -VAD 0 16384" \
> 	/mnt/root/file
>> 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.
> Yes, but I think that's a feature.  You've optimised the filesystem
> layout for IO that is 64kB sized and aligned IO, but your test case
> is mixing 4kB and 16KB IO. The filesystem should be telling you that
> you're doing something that is sub-optimal for it's configuration,
> and refusing to do weird overlapping sub-rtextsize atomic IO is a
> pretty good sign that you've got something wrong.

Then we really end up with a strange behavior for the user. I mean, the 
user may ask - "why did this 16KB atomic write pass and this one fail? 
I'm following all the rules", and then "No one said not to mix write 
sizes or not mix atomic and non-atomic writes, so should be ok. Indeed, 
that earlier 4K write for the same region passed".

Playing devil's advocate here, at least this behavior should be documented.

> 
> The whole reason for rtextsize existing is to optimise the rtextent
> allocation to the typical minimum IO size done to that volume. If
> all your IO is sub-rtextsize size and alignment, then all that has
> been done is forcing the entire rt device IO into a corner it was
> never really intended nor optimised for.

Sure, but just because we are optimized for a certain IO write size 
should not mean that other writes are disallowed or quite problematic.

> 
> Why should we jump through crazy hoops to try to make filesystems
> optimised for large IOs with mismatched, overlapping small atomic
> writes?

As mentioned, typically the atomic writes will be the same size, but we 
may have other writes of smaller size.

> 
>> 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.
> Sure, I know how it works. My point is that it's a terrible way to
> go about allowing that second atomic write to succeed.
I think 'terrible' is a bit too strong a word here. Indeed, you suggest 
to manually zero the file to solve this problem, below, while this code 
change does the same thing automatically.

BTW, there was a request for behavior like in this patch. Please see 
this discussion on the ext4 atomic writes port:

https://lore.kernel.org/linux-ext4/ZXhb0tKFvAge%2FGWf@infradead.org/

So we should have some solution where the kernel automatically takes 
care of this unwritten extent issue.

> 
>>>> 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.
> Better the application zeros the file when it is being initialised
> and doesn't have performance constraints rather than forcing the
> filesystem to do it in the IO fast path when IO performance and
> latency actually matters to the application.

Can't we do both? I mean, the well-informed user can still pre-zero the 
file just to ensure we aren't doing this zero'ing with the extent 
allocation.

> 
> There are production databases that already do this manual zero
> initialisation to avoid unwritten extent conversion overhead during
> runtime operation. That's because they want FUA writes to work, and
> that gives 25% better IO performance over the same O_DSYNC writes
> doing allocation and/or unwritten extent conversion after
> fallocate() which requires journal flushes with O_DSYNC writes.
> 
> Using atomic writes is no different.

Sure, and as I said, they can do both. The user is already wise enough 
to zero the file for other performance reasons (like FUA writes).

> 
>> 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.

JFYI, these 4K writes were in some compressed mode in the DB setup 
phase, hence the smaller size.

> And therein lies the problem.
> 
> If you are doing sub-rtextent IO at all, then you are forcing the
> filesystem down the path of explicitly using unwritten extents and
> requiring O_DSYNC direct IO to do journal flushes in IO completion
> context and then performance just goes down hill from them.
> 
> The requirement for unwritten extents to track sub-rtextsize written
> regions is what you're trying to work around with XFS_BMAPI_ZERO so
> that atomic writes will always see "atomic write aligned" allocated
> regions.
> 
> Do you see the problem here? You've explicitly told the filesystem
> that allocation is aligned to 64kB chunks, then because the
> filesystem block size is 4kB, it's allowed to track unwritten
> regions at 4kB boundaries. Then you do 4kB aligned file IO, which
> then changes unwritten extents at 4kB boundaries. Then you do a
> overlapping 16kB IO that*requires*  16kB allocation alignment, and
> things go BOOM.
> 
> Yes, they should go BOOM.
> 
> This is a horrible configuration - it is incomaptible with 16kB
> aligned and sized atomic IO. 

Just because the DB may do 16KB atomic writes most of the time should 
not disallow it from any other form of writes.

Indeed at 
https://lore.kernel.org/linux-nvme/ZSR4jeSKlppLWjQy@dread.disaster.area/ 
you wrote "Not every IO to every file needs to be atomic." (sorry for 
quoting you)

So the user can do other regular writes, but you say that they should be 
always writing full extents. This just may not suit some DBs.

> Allocation is aligned to 64kB, written
> region tracking is aligned to 4kB, and there's nothing to tell the
> filesystem that it should be maintaining 16kB "written alignment" so
> that 16kB atomic writes can always be issued atomically.
> 
> i.e. if we are going to do 16kB aligned atomic IO, then all the
> allocation and unwritten tracking needs to be done in 16kB aligned
> chunks, not 4kB. That means a 4KB write into an unwritten region or
> a hole actually needs to zero the rest of the 16KB range it sits
> within.
> 
> The direct IO code can do this, but it needs extension of the
> unaligned IO serialisation in XFS (the alignment checks in
> xfs_file_dio_write()) and the the sub-block zeroing in
> iomap_dio_bio_iter() (the need_zeroing padding has to span the fs
> allocation size, not the fsblock size) to do this safely.
> 
> Regardless of how we do it, all IO concurrency on this file is shot
> if we have sub-rtextent sized IOs being done. That is true even with
> this patch set - XFS_BMAPI_ZERO is done whilst holding the
> XFS_ILOCK_EXCL, and so no other DIO can map extents whilst the
> zeroing is being done.
> 
> IOWs, anything to do with sub-rtextent IO really has to be treated
> like sub-fsblock DIO - i.e. exclusive inode access until the
> sub-rtextent zeroing has been completed.

I do understand that this is not perfect that we may have mixed block 
sizes being written, but I don't think that we should disallow it and 
throw an error.

I would actually think that the worst thing is that the user does not 
know this restriction.

> 
>>>>> 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.
> So why can't the application do that manually like others already do
> to enable FUA optimisations for O_DSYNC writes?

Is this even officially documented as advice or a suggestion for users?

> 
> FWIW, we probably should look to extend fallocate() to allow
> userspace to say "write real zeroes, not fake ones" so the
> filesystem can call blkdev_issue_zeroout() after preallocation to
> offload the zeroing to the hardware and then clear the unwritten
> bits on the preallocated range...

ack

> 
>>>>>>     	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.
> Which means, initially, iomap only supposed writes that are a
> multiple of filesystem block size. regardless, this should be
> checked in the submission path, not in the extent mapping callback.
> 
> FWIW, we've already established above that iomap needs to handle
> rtextsize chunks rather than fs block size for correct zeroing
> behaviour for atomic writes, so this probably just needs to go away.

Fine, I think that all this can be moved to iomap core / removed.

> 
>> The idea is that we can first ensure size is a multiple of FS blocksize, and
>> then can use br_blockcount directly, below.
> Yes, and we can do all these checks on the iomap that we return to
> the iomap layer. All this is doing is running the checks on the XFS
> imap before it is formatted into the iomap iomap and returned to the
> iomap layer. These checks can be done on the returned iomap in the
> iomap layer if IOMAP_ATOMIC is set....
> 
>> However, the core iomap code does not know FS atomic write min and max per
>> inode, so we need some checks here.
> So maybe we should pass them down to iomap in the iocb when
> IOCB_ATOMIC is set, or reject the IO at the filesytem layer when
> checking atomic IO alignment before we pass the IO to the iomap
> layer...

Yes, I think that something like this is possible.

As for using kiocb, it's quite a small structure, so I can't imagine 
that it would be welcome to add all this atomic write stuff.

So we could add extra awu min and max args to iomao_dio_rw(), and these 
could be filled in by the calling FS. But that function already has 
enough args.

Alternatively we could add an iomap_ops method to look up awu min and 
max for the inode.

Thanks,
John

Powered by blists - more mailing lists