Re: [PATCH v3 RESEND] iomap: set REQ_NOWAIT according to IOCB_NOWAIT in Direct IO

[JeffleXu <jefflexu@linux.alibaba.com>]

Thanks for your explanation, again.

On 12/14/20 10:56 AM, Dave Chinner wrote:
> On Fri, Dec 11, 2020 at 10:50:44AM +0800, JeffleXu wrote:
>> Excellent explanation! Thanks a lot.
>>
>> Still some questions below.
>>
>> On 12/10/20 1:18 PM, Dave Chinner wrote:
>>> On Thu, Dec 10, 2020 at 09:55:32AM +0800, JeffleXu wrote:
>>>> Sorry I'm still a little confused.
>>>>
>>>>
>>>> On 12/10/20 5:23 AM, Dave Chinner wrote:
>>>>> On Tue, Dec 08, 2020 at 01:46:47PM +0800, JeffleXu wrote:
>>>>>>
>>>>>>
>>>>>> On 12/7/20 10:21 AM, Dave Chinner wrote:
>>>>>>> On Fri, Dec 04, 2020 at 05:44:56PM +0800, Hao Xu wrote:
>>>>>>>> Currently, IOCB_NOWAIT is ignored in Direct IO, REQ_NOWAIT is only set
>>>>>>>> when IOCB_HIPRI is set. But REQ_NOWAIT should be set as well when
>>>>>>>> IOCB_NOWAIT is set.
>>>>>>>>
>>>>>>>> Suggested-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>>>>>>>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>>>>>>>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Hi all,
>>>>>>>> I tested fio io_uring direct read for a file on ext4 filesystem on a
>>>>>>>> nvme ssd. I found that IOCB_NOWAIT is ignored in iomap layer, which
>>>>>>>> means REQ_NOWAIT is not set in bio->bi_opf.
>>>>>>>
>>>>>>> What iomap is doing is correct behaviour. IOCB_NOWAIT applies to the
>>>>>>> filesystem behaviour, not the block device.
>>>>>>>
>>>>>>> REQ_NOWAIT can result in partial IO failures because the error is
>>>>>>> only reported to the iomap layer via IO completions. Hence we can
>>>>>>> split a DIO into multiple bios and have random bios in that IO fail
>>>>>>> with EAGAIN because REQ_NOWAIT is set. This error will
>>>>>>> get reported to the submitter via completion, and it will override
>>>>>>> any of the partial IOs that actually completed.
>>>>>>>
>>>>>>> Hence, like the recently reported multi-mapping IOCB_NOWAIT bug
>>>>>>> reported by Jens and fixed in commit 883a790a8440 ("xfs: don't allow
>>>>>>> NOWAIT DIO across extent boundaries") we'll get silent partial
>>>>>>> writes occurring because the second submitted bio in an IO can
>>>>>>> trigger EAGAIN errors with partial IO completion having already
>>>>>>> occurred.
>>>>>>>
>>>>
>>>>>>> Further, we don't allow partial IO completion for DIO on XFS at all.
>>>>>>> DIO must be completely submitted and completed or return an error
>>>>>>> without having issued any IO at all.  Hence using REQ_NOWAIT for
>>>>>>> DIO bios is incorrect and not desirable.
>>>>
>>>>
>>>> The current block layer implementation causes that, as long as one split
>>>> bio fails, then the whole DIO fails, in which case several split bios
>>>> maybe have succeeded and the content has been written to the disk. This
>>>> is obviously what you called "partial IO completion".
>>>>
>>>> I'm just concerned on how do you achieve that "DIO must return an error
>>>> without having issued any IO at all". Do you have some method of
>>>> reverting the content has already been written into the disk when a
>>>> partial error happened?
>>>
>>> I think you've misunderstood what I was saying. I did not say
>>> "DIO must return an error without having issued any IO at all".
>>> There are two parts to my statement, and you just smashed part of
>>> the first statement into part of the second statement and came up
>>> something I didn't actually say.
>>>
>>> The first statement is:
>>>
>>> 	1. "DIO must be fully submitted and completed ...."
>>>
>>> That is, if we need to break an IO up into multiple parts, the
>>> entire IO must be submitted and completed as a whole. We do not
>>> allow partial submission or completion of the IO at all because we
>>> cannot accurately report what parts of a multi-bio DIO that failed
>>> through the completion interface. IOWs, if any of the IOs after the
>>> first one fail submission, then we must complete all the IOs that
>>> have already been submitted before we can report the failure that
>>> occurred during the IO.
>>>
>>
>> 1. Actually I'm quite not clear on what you called "partial submission
>> or completion". Even when REQ_NOWAIT is set for all split bios of one
>> DIO, then all these split bios are **submitted** to block layer through
>> submit_bio(). Even when one split bio after the first one failed
>> **inside** submit_bio() because of REQ_NOWAIT, submit_bio() only returns
>> BLK_QC_T_NONE, and the DIO layer (such as __blkdev_direct_IO()) will
>> still call submit_bio() for the remaining split bios. And then only when
>> all split bios complete, will the whole kiocb complete.
> 
> Yes, so what you have there is complete submission and completion.
> i.e. what I described as #1.
> 
>> So if you define "submission" as submitting to hardware disk (such as
>> nvme device driver), then it is indeed **partial submission** when
>> REQ_NOWAIT set. But if the definition of "submission" is actually
>> "submitting to block layer by calling submit_bio()", then all split bios
>> of one DIO are indeed submitted to block layer, even when one split bio
>> gets BLK_STS_AGAIN because of REQ_NOWIAT.
> 
> The latter is the definition we using in iomap, because at this
> layer we are the ones submitting the bios and defining the bounds of
> the IO. What happens in the lower layers does not concern us here.
> 
> Partial submission can occur at the iomap layer if IOCB_NOWAIT is
> set - that was what 883a790a8440 fixed. i.e. we do
> 
> 	loop for all mapped extents {
> 		loop for mapped range {
> 			submit_bio(subrange)
> 		}
> 	}
> 
> So if we have an extent range like so:
> 
> 	start					end
> 	+-----------+---------+------+-----------+
> 	  ext 1        ext 2    ext 3    ext 4
> 
> We actually have to loop above 4 times to map the 4 extents that
> span the user IO range. That means we need to submit at least 4
> independent bios to run this IO.
> 
> So if we submit all the bios in a first mapped range (ext 1), then
> go back to the filesystem to get the map for the next range to
> submit and it returns -EAGAIN, we break out of the outer loop
> without having completely submitted bios for the range spanning ext
> 2, 3 or 4. That's partial IO submission as the *iomap layer* defines
> it.

Now I can understand the context of commit 883a790a8440 and the root
issue here.

> 
> And the "silent" part of the "partial write" it came from the fact
> this propagated -EAGAIN to the user despite the fact that some IO
> was actually successfully committed and completed. IOws, we may have
> corrupted data on disk by telling the user nothing was done with
> -EAGAIN rather than failing the partial IO with -EIO.>
> This is the problem commit 883a790a8440 fixed.
> 
>> 2. One DIO could be split into multiple bios in DIO layer. Similarly one
>> big bio could be split into multiple bios in block layer. In the
>> situation when all split bios have already been submitted to block
>> layer, since the IO completion interface could return only one error
>> code, the whole DIO could fail as long as one split bio fails, while
>> several other split bios could have already succeeded and the
>> corresponding disk sectors have already been overwritten.
> 
> Yup. That can happen. That's a data corruption for which prevention
> is the responsibility of the layer doing the splitting and
> recombining. 

Unfortunately the block layer doesn't handle this well currently.


> The iomap layer knows nothing of this - this situation
> needs to result in the entire bio that was split being marked with
> an EIO error because we do not know what parts of the bio made it to
> disk or not. IOWs, the result of the IO is undefined, and so we
> need to return EIO to the user.

What's the difference of EIO and EAGAIN here, besides the literal
semantics, when the data corruption has already happened? The upper user
has to resubmit the bio when EAGAIN is received, and the data corruption
should disappear once the resubmission completes successfully. But the
bio may not be resubmitted when EIO is received, and the data corruption
is just left there.



> 
> This "data is inconsistent until all sub-devices complete their IO
> successfully" situation is also known as the "RAID 5 write hole".
> Crash while these IOs are in flight, and some might hit the disk and
> some might not. When the system comes back up, you've got no idea
> which part of that IO was completed or not, so the data in the
> entire stripe is corrupt.
> 
>> I'm not sure
>> if this is what you called "silent partial writes", and if this is the
>> core reason for commit 883a790a8440 ("xfs: don't allow NOWAIT DIO across
>> extent boundaries") you mentioned in previous mail.
> 
> No, it's not the same thing. This "split IO failed" should never
> result in a silent partial write - it should always result in EIO.

I'm doubted about this.

The block layer could split one big bio into multiple split bios. Also
one big DIO from one extent could also be split into multiple split
bios, since one single bio could maintain at most BIO_MAX_PAGES i.e. 256
segments. (see the loop in iomap_dio_bio_actor()) If the input iov_iter
contains more than 256 segments, then it will be split into multiple
split bios, though this iov_iter may be mapped to one single extent.

And once one split bio (among all these split bios) failed with EAGAIN,
the completion status of the whole IO is EAGAIN, rather than EIO, at
least for the current implementation of block layer. So this should also
be called "silent partial write", since partial bios may have been
written to the disk successfully, while still **EAGAIN** returned in
this situation?


> 
>> If this is the case, then it seems that the IO completion interface
>> should be blamed for this issue. Besides REQ_NOWIAT, there may be more
>> situations that will cause "silent partial writes".
> 
> Yes, there are many potential causes of this behaviour. They are all
> considered to be a data corruption, and so in all cases they should
> be returning EIO as the completion status to the submitter of the
> bio.
> >> As long as one split bios could fail (besides EAGAIN, maybe EOPNOTSUPP,
>> if possible), while other split bios may still succeeds, then the error
>> of one split bio will still override the completion status of the whole
>> DIO.
> 
> If you get EOPNOTSUPP from on device in bio split across many
> devices, then you have a bug in the layer that is splitting the IO.
> All the sub-devices need to support the operation, or the top level
> device cannot safely support that operation.
> 
> And if one device didn't do the opreation, then we have inconsistent
> data on disk now and so EIO is the result that should be returned to
> the user.

EOPNOTSUPP is just an example here. I admit that NOWAIT may be
problematic here. But what I mean here is that, once one split bio
fails, the whole IO fails with the same error code e.g.
EAGIAN/EOPNOTSUPP rather than EIO, while partial split bios may have
already succeeded. IOWs, the block layer may also trigger "silent
partial write" even without REQ_NOWAIT, though the possibility of
"silent partial write" may increase a lot when REQ_NOWAIT applied.


> 
>> In this case "silent partial writes" is still possible? In my
>> understanding, passing REQ_NOWAIT flags from IOCB_NOWAIT in DIO layer
>> only makes the problem more likely to happen, but it's not the only
>> source of this problem?
> 
> Passing REQ_NOWAIT from the iomap dio layer means that we will see
> data corruptions in stable storage simply due to high IO load, rather
> than the much, much rarer situations of software bugs or actual
> hardware errors.  IOWs, REQ_NOWAIT essentially guarantees data
> corruption in stable storage of a perfectly working storage stack
> will occur and there is nothing the application can do to prevent
> that occurring.
> 
> The worst part of this is that REQ_NOWAIT also requires the
> application to detect and correct the corruption caused by the
> kernel using REQ_NOWAIT inappropriately. And if the application
> crashes before it can correct the data corruption, it's suddenly a
> permanent, uncorrectable data corruption.
> 
> REQ_NOWAIT is dangerous and can easily cause harm to user data by
> aborting parts of submitted IOs. IOWs, it's more like a random IO
> error injection mechanism than a useful method of doing safe
> non-blocking IO.
> 
> Cheers,
> 
> Dave.
> 

-- 
Thanks,
Jeffle