From patchwork Thu Jan 30 16:34:19 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 37622 Received: (qmail 117969 invoked by alias); 30 Jan 2020 16:34:27 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 117951 invoked by uid 89); 30 Jan 2020 16:34:26 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=yang, Yang, emulation, H*p:D*org X-HELO: mail-qv1-f68.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:references:from:autocrypt:subject:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=HbC1BGF4YMvYUmxnOcnyPYUqlEzqn5mKu0Rxqs898mU=; b=G/9Z4lObD9urbkh/WtvvsPGHcW56n9v57BrGiknY3UIg0fMYqU3szXyeU+bf51uhvS 13ZrGugYhD4aNcVUjw+NsIXtvMkrzcBunn0oDLbdP3g42orVWImo1uTRB/kP+CZH0KZt W02oSN5qr7cpnvzOZWKobQyWhiqK6hjSc2ZD4I/8TgDzsjioZCfr/3L9tGI62OYgu8zy pnZajCvpM9G1U4M60Ueyuah+J5bqrTolDsceVFlzqbUfMYF7zuw/K5uPaUR9nkkMgsQo NntURBZ2BDA27mhTk5oE47pkrOJ0dmtEeh8Dzvf+2DXHsBJ0KzyXwAi3mnhMdJVcA22/ bz/A== Return-Path: To: libc-alpha@sourceware.org References: <20200115013151.905-1-yangx.jy@cn.fujitsu.com> From: Adhemerval Zanella Subject: Re: [PATCH v2] sysdeps/posix/posix_fallocate*: Make emulated posix_fallocate() work properly Message-ID: <73c25e8d-8114-6571-7a71-175dacffffe2@linaro.org> Date: Thu, 30 Jan 2020 13:34:19 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200115013151.905-1-yangx.jy@cn.fujitsu.com> On 14/01/2020 22:31, 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. Thanks for catching it, comments below. > > Signed-off-by: Xiao Yang We do not use signed-off, but Copyright assignments. I am not sure this particular fix can be considered trivial. > --- > sysdeps/posix/posix_fallocate.c | 17 +++++++++++++++++ > sysdeps/posix/posix_fallocate64.c | 17 +++++++++++++++++ > 2 files changed, 34 insertions(+) > > 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: The patch looks ok, although I would prefer if we factor the logic where or not to write the byte on its own function. Something like: > /* 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 > diff --git a/sysdeps/posix/posix_fallocate.c b/sysdeps/posix/posix_fallocate.c index e7fccfc1c8..855439bfcb 100644 --- a/sysdeps/posix/posix_fallocate.c +++ b/sysdeps/posix/posix_fallocate.c @@ -23,6 +23,26 @@ #include #include +static inline int +write_byte (int fd, off_t offset, off_t st_size) +{ + if (offset < st_size) + { + unsigned char b; + ssize_t rdsize = __libc_pread64 (fd, &b, 1, offset); + if (rdsize < 0) + return errno; + /* If there is a non-zero byte, the block must have been + allocated already. */ + if (rdsize == 1 && b != 0) + return 0; + } + + if (__libc_pwrite64 (fd, "", 1, offset) != 1) + return errno; + return 0; +} + /* Reserve storage for the data of the file associated with FD. This emulation is far from perfect, but the kernel cannot do not much better for network file systems, either. */ @@ -31,6 +51,7 @@ int posix_fallocate (int fd, __off_t offset, __off_t len) { struct stat64 st; + int r; if (offset < 0 || len < 0) return EINVAL; @@ -97,25 +118,21 @@ posix_fallocate (int fd, __off_t offset, __off_t len) lack a better option. Compare-and-swap against a file mapping might additional local races, but requires interposition of a signal handler to catch SIGBUS. */ + + if (offset % increment + len % increment > increment) + { + r = write_byte (fd, offset, st.st_size); + if (r != 0) + return r; + } + for (offset += (len - 1) % increment; len > 0; offset += increment) { len -= increment; - if (offset < st.st_size) - { - unsigned char c; - ssize_t rsize = __pread (fd, &c, 1, offset); - - if (rsize < 0) - return errno; - /* If there is a non-zero byte, the block must have been - allocated already. */ - else if (rsize == 1 && c != 0) - continue; - } - - if (__pwrite (fd, "", 1, offset) != 1) - return errno; + r = write_byte (fd, offset, st.st_size); + if (r != 0) + return r; } return 0;