Re: [PATCH v6 10/13] xfs: iomap COW-based atomic write support

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

John Garry <john.g.garry@oracle.com> writes:

>>> +		}
>>>   		end_fsb = imap.br_startoff + imap.br_blockcount;
>>>   		length = XFS_FSB_TO_B(mp, end_fsb) - offset;
>>>   	}
>>>   
>>> -	if (imap_needs_alloc(inode, flags, &imap, nimaps))
>>> +	needs_alloc = imap_needs_alloc(inode, flags, &imap, nimaps);
>>> +
>>> +	if (flags & IOMAP_ATOMIC) {
>>> +		error = -EAGAIN;
>>> +		/*
>>> +		 * If we allocate less than what is required for the write
>>> +		 * then we may end up with multiple mappings, which means that
>>> +		 * REQ_ATOMIC-based cannot be used, so avoid this possibility.
>>> +		 */
>>> +		if (needs_alloc && orig_end_fsb - offset_fsb > 1)
>>> +			goto out_unlock;
>> 
>> I have a quick question here. Based on above check it looks like
>> allocation requests on a hole or the 1st time allocation (append writes)
>> for a given logical range will always be done using CoW fallback
>> mechanism, isn't it? 
>
> Right, but...
>
>
>> So that means HW based multi-fsblock atomic write
>> request will only happen for over writes (non-discontigous extent),
>> correct?
>
> For an unwritten pre-allocated extent, we can use the REQ_ATOMIC method.
>
> fallocate (without ZERO RANGE) would give a pre-allocated unwritten 
> extent, and a write there would not technically be an overwrite.
>
>> 
>> Now, it's not always necessary that if we try to allocate an extent for
>> the given range, it results into discontiguous extents. e.g. say, if the
>> entire range being written to is a hole or append writes, then it might
>> just allocate a single unwritten extent which is valid for doing an
>> atomic write using HW/BIOs right?
>
> Right
>
>> And it is valid to write using unwritten extent as long as we don't have
>> mixed mappings i.e. the entire range should either be unwritten or
>> written for the atomic write to be untorned, correct?
>> 
>
> We can't write to discontiguous extents, and a mixed mapping would mean 
> discontiguous extents.
>
> And, as mentioned earlier, it is ok to use REQ_ATOMIC method on an 
> unwritten extent.
>
>> I am guessing this is kept intentional?
>> 
> Yes

Thanks, John for addressing the queries. It would be helpful to include
this information in the commit message as well then right? Otherwise
IMO, the original commit message looks incomplete.

Maybe we can add this too?
=========================
This patch adds CoW based atomic write support which will be used as a
SW fallback in following scenarios:

- All append write scenarios.
- Any new writes on the region containing holes.
- Writes to any misaligned regions
- Writes to discontiguous extents.


<original commit msg snip>
=========================
In cases of an atomic write covering misaligned or discontiguous disk
blocks, we will use a CoW-based method to issue the atomic write.

-ritesh