Re: [PATCH v5 1/2] Return bytes transferred for partial direct I/O

[Goldwyn Rodrigues <rgoldwyn@suse.de>]

On 01/22/2018 01:13 PM, Jens Axboe wrote:
> On 1/22/18 12:10 PM, Andreas Dilger wrote:
>>
>>> On Jan 20, 2018, at 8:07 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 1/20/18 7:23 PM, Goldwyn Rodrigues wrote:
>>>>
>>>>
>>>> On 01/20/2018 08:11 PM, Andi Kleen wrote:
>>>>>
>>>>> It's likely there's a lot of code in user space that does
>>>>>
>>>>>     if (write(..., N) < 0) handle error
>>>>>
>>>>> With your change it would need to be
>>>>>
>>>>>     if (write(..., N) != N) handle error
>>>>>
>>>>> How much code is actually doing that?
>>>>>
>>>>> I can understand it fixes your artifical test suite, but it seems to me your
>>>>> change has a high potential to break a lot of existing user code
>>>>> in subtle ways. So it seems to be a bad idea.
>>>>
>>>>
>>>> Quoting 'man 2 write':
>>>>
>>>> RETURN VALUE
>>>> On  success,  the  number  of bytes written is returned (zero indicates
>>>> nothing was written).  It is not an error if  this  number  is  smaller
>>>> than the number of bytes requested; this may happen for example because
>>>> the disk device was filled.  See also NOTES.
>>>
>>> You can quote as much man page as you want - Andi is well aware of how
>>> read/write system call works, as I'm sure all of us are, that is not the
>>> issue. The issue is that there are potentially LOTS of applications out
>>> there that do not check for short writes, they do exactly what Andi
>>> speculated above. If you break it with this change, it doesn't matter
>>> what's in the man page. What matters is previous behavior, and that
>>> you are breaking user space. At that point nobody cares what's in the
>>> man page.
>>
>> Applications that don't handle partial writes are already broken with
>> buffered I/O, this change doesn't really make them more broken.
> 
> Not disagreeing that they are broken, of course they are. But if you've
> been doing direct IO and not seeing short writes, then this could break
> your application. And if that's the case, it doesn't really help to say

I started exploring short writes and how direct I/O behaves on errors
after your suggestion to incorporate short writes in a previous
conversation [1]. I started looking into how midway errors with direct
I/O's are handled now and I stumble upon this issue.

> that their application was "already broken". I'd hate for a kernel
> upgrade to break them.
> 
> I do wish we could make the change, and maybe we can. But it probably
> needs some safe guard proc entry to toggle the behavior, something we
> can drop in a few years when we're confident it won't break real
> applications.

Assuming we call it /proc/sys/fs/dio_short_writes(better names/paths?),
should it be enabled or disabled by default?

[1] https://www.spinics.net/lists/linux-block/msg15910.html



-- 
Goldwyn