Re: [PATCH] dd: support iflag=direct with arbitrary sized files

[Pádraig Brady]

On 17/10/17 01:10, Pádraig Brady wrote:
> On 16/10/17 00:15, Pádraig Brady wrote:
>> * src/dd.c (iread): Handle read error with a non-aligned
>> file offset in the O_DIRECT case.
>> * tests/dd/direct.sh: Test the iflag=direct case also.
>> * NEWS: Mention the improvement.
>> ---
>>  NEWS               |  4 ++++
>>  src/dd.c           | 15 +++++++++++----
>>  tests/dd/direct.sh |  4 ++--
>>  3 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index c61e4e8..00cf6df 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -16,6 +16,10 @@ GNU coreutils NEWS                                    -*- 
>> outline -*-
>>    to attempt to hide the original length of the file name.
>>    [bug introduced in coreutils-8.28]
>>  
>> +** Improvements
>> +
>> +  dd now supports iflag=direct with arbitrary sized files.
>> +
>>  ** Build-related
>>  
>>    Default man pages are now distributed which are used if perl is
>> diff --git a/src/dd.c b/src/dd.c
>> index 636fac6..613dfe0 100644
>> --- a/src/dd.c
>> +++ b/src/dd.c
>> @@ -1108,11 +1108,21 @@ static ssize_t
>>  iread (int fd, char *buf, size_t size)
>>  {
>>    ssize_t nread;
>> +  static ssize_t prev_nread;
>>  
>>    do
>>      {
>>        process_signals ();
>>        nread = read (fd, buf, size);
>> +      /* Ignore final read error with iflag=direct as that
>> +         returns EINVAL due to the non aligned file offset.  */
>> +      if (nread == -1 && errno == EINVAL
>> +          && 0 < prev_nread && prev_nread < size
>> +          && (input_flags & O_DIRECT))
>> +        {
>> +          errno = 0;
>> +          nread = 0;
>> +        }
>>      }
>>    while (nread < 0 && errno == EINTR);
>>  
>> @@ -1122,8 +1132,6 @@ iread (int fd, char *buf, size_t size)
>>  
>>    if (0 < nread && warn_partial_read)
>>      {
>> -      static ssize_t prev_nread;
>> -
>>        if (0 < prev_nread && prev_nread < size)
>>          {
>>            uintmax_t prev = prev_nread;
>> @@ -1136,10 +1144,9 @@ iread (int fd, char *buf, size_t size)
>>                     prev);
>>            warn_partial_read = false;
>>          }
>> -
>> -      prev_nread = nread;
>>      }
>>  
>> +  prev_nread = nread;
>>    return nread;
>>  }
>>  
>> diff --git a/tests/dd/direct.sh b/tests/dd/direct.sh
>> index e440ba3..f6bc9ca 100755
>> --- a/tests/dd/direct.sh
>> +++ b/tests/dd/direct.sh
>> @@ -1,5 +1,5 @@
>>  #!/bin/sh
>> -# ensure that dd's oflag=direct works
>> +# ensure that dd's iflag=direct and oflag=direct work
>>  
>>  # Copyright (C) 2009-2017 Free Software Foundation, Inc.
>>  
>> @@ -29,7 +29,7 @@ truncate -s 8193 p1 || framework_failure_
>>  
>>  for i in short m1 p1; do
>>    rm -f out
>> -  dd if=$i oflag=direct of=out || fail=1
>> +  dd if=$i iflag=direct oflag=direct of=out || fail=1
>>  done
>>  
>>  Exit $fail
>>
> 
> This will need adjusting.
> It works on ext4,
> and on newer 4.10.6-200.fc25.x86_64) xfs, where it's not actually needed as 
> EINVAL is not returned
> but on older (centos7) xfs, even the initial read() will return EINVAL,
> so we'd have to disable O_DIRECT and retry to support that, which I'm not 
> sure about.
> I'll do a bit more testing...

Actually the older XFS I was testing was on a device with 4K sectors,
which confused me as I was testing with a file less than 4K.
All versions of XFS seem to be able to read arbitrarily sized files
as long as the read size is a multiple of the sector size of the underlying 
device.
Newer XFS may relax that to 512, but that's beside the point here.

So the proposed patch should avoid the issue with reading after the last
short read on ext4, so I'll apply this.

cheers,
Pádraig.