diff mbox

[v2] sysdeps/posix/posix_fallocate*: Make emulated posix_fallocate() work properly

Message ID 20200115013151.905-1-yangx.jy@cn.fujitsu.com
State FSF Assignment Missing
Headers show

Commit Message

Xiao Yang Jan. 15, 2020, 1:31 a.m. UTC
Emulated posix_fallocate() only writes data in one block if block
size is 4096, offset is 4095 and len is 2.  The emulated code should
write data in two blocks in the case because it actually crosses two
blocks.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 sysdeps/posix/posix_fallocate.c   | 17 +++++++++++++++++
 sysdeps/posix/posix_fallocate64.c | 17 +++++++++++++++++
 2 files changed, 34 insertions(+)

Comments

Siddhesh Poyarekar Dec. 21, 2020, 4:13 a.m. UTC | #1
On 1/15/20 7:01 AM, Xiao Yang wrote:
> Emulated posix_fallocate() only writes data in one block if block
> size is 4096, offset is 4095 and len is 2.  The emulated code should
> write data in two blocks in the case because it actually crosses two
> blocks.
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>

Could you please clarify your copyright assignment status?  I suppose 
you ought to be covered by any agreement Fujitsu may have with the FSF 
but I'm not a steward and hence do not have a way to check.  I'll review 
anyway given that there are patches from @cn.fujitsu.com in the repo.

> ---
>   sysdeps/posix/posix_fallocate.c   | 17 +++++++++++++++++
>   sysdeps/posix/posix_fallocate64.c | 17 +++++++++++++++++
>   2 files changed, 34 insertions(+)

Please file a bug report describing the issue and also add a test case 
to protect from regressions.

> diff --git a/sysdeps/posix/posix_fallocate.c b/sysdeps/posix/posix_fallocate.c
> index e7fccfc1c8..22e5fea091 100644
> --- a/sysdeps/posix/posix_fallocate.c
> +++ b/sysdeps/posix/posix_fallocate.c
> @@ -93,6 +93,23 @@ posix_fallocate (int fd, __off_t offset, __off_t len)
>         increment = 4096;
>     }
>   
> +  if (offset % increment + len % increment > increment)
> +    {
> +      if (offset < st.st_size)
> +        {
> +          unsigned char b;
> +          ssize_t rdsize = __pread (fd, &b, 1, offset);
> +          if (rdsize < 0)
> +            return errno;
> +          if (rdsize == 1 && b != 0)
> +            goto next;
> +        }
> +
> +      if (__pwrite (fd, "", 1, offset) != 1)
> +        return errno;
> +    }

A better fix may be to fix the loop conditions to fold this corner case in.

Thanks,
Siddhesh
Carlos O'Donell Dec. 21, 2020, 4:23 a.m. UTC | #2
On 12/20/20 11:13 PM, Siddhesh Poyarekar wrote:
> On 1/15/20 7:01 AM, Xiao Yang wrote:
>> Emulated posix_fallocate() only writes data in one block if block 
>> size is 4096, offset is 4095 and len is 2.  The emulated code
>> should write data in two blocks in the case because it actually
>> crosses two blocks.
>> 
>> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> 
> Could you please clarify your copyright assignment status?  I suppose
> you ought to be covered by any agreement Fujitsu may have with the
> FSF but I'm not a steward and hence do not have a way to check.  I'll
> review anyway given that there are patches from @cn.fujitsu.com in
> the repo.

There is no assignment on file with the FSF for Fujitsu regarding glibc.
Siddhesh Poyarekar Dec. 21, 2020, 4:24 a.m. UTC | #3
On 12/21/20 9:53 AM, Carlos O'Donell wrote:
> On 12/20/20 11:13 PM, Siddhesh Poyarekar wrote:
>> On 1/15/20 7:01 AM, Xiao Yang wrote:
>>> Emulated posix_fallocate() only writes data in one block if block
>>> size is 4096, offset is 4095 and len is 2.  The emulated code
>>> should write data in two blocks in the case because it actually
>>> crosses two blocks.
>>>
>>> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
>>
>> Could you please clarify your copyright assignment status?  I suppose
>> you ought to be covered by any agreement Fujitsu may have with the
>> FSF but I'm not a steward and hence do not have a way to check.  I'll
>> review anyway given that there are patches from @cn.fujitsu.com in
>> the repo.
> 
> There is no assignment on file with the FSF for Fujitsu regarding glibc.
> 

Thanks for confirming Carlos, I've marked the patch as such in patchwork.

Siddhesh
diff mbox

Patch

diff --git a/sysdeps/posix/posix_fallocate.c b/sysdeps/posix/posix_fallocate.c
index e7fccfc1c8..22e5fea091 100644
--- a/sysdeps/posix/posix_fallocate.c
+++ b/sysdeps/posix/posix_fallocate.c
@@ -93,6 +93,23 @@  posix_fallocate (int fd, __off_t offset, __off_t len)
       increment = 4096;
   }
 
+  if (offset % increment + len % increment > increment)
+    {
+      if (offset < st.st_size)
+        {
+          unsigned char b;
+          ssize_t rdsize = __pread (fd, &b, 1, offset);
+          if (rdsize < 0)
+            return errno;
+          if (rdsize == 1 && b != 0)
+            goto next;
+        }
+
+      if (__pwrite (fd, "", 1, offset) != 1)
+        return errno;
+    }
+
+next:
   /* Write a null byte to every block.  This is racy; we currently
      lack a better option.  Compare-and-swap against a file mapping
      might additional local races, but requires interposition of a
diff --git a/sysdeps/posix/posix_fallocate64.c b/sysdeps/posix/posix_fallocate64.c
index f9d4fe5ca3..1c46b186b6 100644
--- a/sysdeps/posix/posix_fallocate64.c
+++ b/sysdeps/posix/posix_fallocate64.c
@@ -93,6 +93,23 @@  __posix_fallocate64_l64 (int fd, __off64_t offset, __off64_t len)
       increment = 4096;
   }
 
+  if (offset % increment + len % increment > increment)
+    {
+      if (offset < st.st_size)
+        {
+          unsigned char b;
+          ssize_t rdsize = __libc_pread64 (fd, &b, 1, offset);
+          if (rdsize < 0)
+            return errno;
+          if (rdsize == 1 && b != 0)
+            goto next;
+        }
+
+      if (__libc_pwrite64 (fd, "", 1, offset) != 1)
+        return errno;
+    }
+
+next:
   /* Write a null byte to every block.  This is racy; we currently
      lack a better option.  Compare-and-swap against a file mapping
      might address local races, but requires interposition of a signal