You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Ivan Zhakov via dev <de...@apr.apache.org> on 2022/07/01 09:13:55 UTC
Re: svn commit: r1902378 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
On Fri, 1 Jul 2022 at 01:10, Yann Ylavic <yl...@gmail.com> wrote:
>
> On Fri, Jul 1, 2022 at 12:00 AM Yann Ylavic <yl...@gmail.com> wrote:
> >
> > On Thu, Jun 30, 2022 at 7:28 PM <iv...@apache.org> wrote:
> > >
> > > Author: ivan
> > > Date: Thu Jun 30 17:28:50 2022
> > > New Revision: 1902378
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1902378&view=rev
> > > Log:
> > > On 1.8.x branch: Merge r1806299, r1806301, r1806308, r1806610:
> > > *) apr_file_write: Optimize large writes to buffered files on
> > > Windows.
> > []
> > >
> > > --- apr/apr/branches/1.8.x/file_io/win32/readwrite.c (original)
> > > +++ apr/apr/branches/1.8.x/file_io/win32/readwrite.c Thu Jun 30
17:28:50 2022
> > > @@ -247,6 +247,91 @@ APR_DECLARE(apr_status_t) apr_file_read(
> > > return rv;
> > > }
> > >
> > > +/* Helper function that adapts WriteFile() to apr_size_t instead
> > > + * of DWORD. */
> > > +static apr_status_t write_helper(HANDLE filehand, const char *buf,
> > > + apr_size_t len, apr_size_t
*pwritten)
> > > +{
> > > + apr_size_t remaining = len;
> > > +
> > > + *pwritten = 0;
> > > + do {
> > > + DWORD to_write;
> > > + DWORD written;
> > > +
> > > + if (remaining > APR_DWORD_MAX) {
> > > + to_write = APR_DWORD_MAX;
> > > + }
> > > + else {
> > > + to_write = (DWORD)remaining;
> > > + }
> > > +
> > > + if (!WriteFile(filehand, buf, to_write, &written, NULL)) {
> > > + *pwritten += written;
> > > + return apr_get_os_error();
> > > + }
> > > +
> > > + *pwritten += written;
> > > + remaining -= written;
> > > + buf += written;
> > > + } while (remaining);
> >
> > So there's no writev() like syscall on Windows (something that
> > provides atomicity)?
>
> I found WriteFileGather
> (
https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefilegather
).
> Maybe it can help with some logic à la apr_socket_sendv()?
>
[[[
Each buffer must be at least the size of a system memory page and *must be
aligned on a system memory page size boundary*. The system writes one
system memory page of data from each buffer.
]]]
[[[
The total number of bytes to be written. Each element of aSegmentArray
contains a one-page chunk of this total. Because the* file must be opened
with FILE_FLAG_NO_BUFFERING*, the number of bytes must be a multiple of the
sector size of the file system where the file is located.
]]]
--
Ivan Zhakov
Re: svn commit: r1902378 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
Posted by Evgeny Kotkov via dev <de...@apr.apache.org>.
Yann Ylavic <yl...@gmail.com> writes:
> But the short-write argument stands, if Windows never short-writes
> then the implicit full-write is unnecessary, but if it does
> short-write then apr_file_write() has to return it too (for the user
> to know), so in both cases the loop has to stop when written <
> to_write (and probably return success if written > 0).
> Users that don't expect short-writes should use apr_file_write_full().
As far as I recall, I had an intent of not changing the behavior more than
necessary, as the original code was always making a full write.
I agree that we could potentially allow for short writes when performing
the passthrough part of the write that makes its way around the buffer.
But all in all, I think that we might want to keep the current approach,
because:
— This won't change the actual behavior for Win32 files from a practical
standpoint, because synchronous WriteFile() guarantees a full write
for them.
— There's a tricky case where we need to handle a large write with
len > DWORD_MAX. I think that we might want to not handle it with a
short write, because if we do so, we're going to have an actually
reproducible short write for files (i.e., different behavior) in the rare
edge case.
— Flushing the buffer requires a full write, so at least a part of the
buffered apr_file_write() is still going to do a full write.
Regards,
Evgeny Kotkov
Re: svn commit: r1902378 - in /apr/apr/branches/1.8.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jul 1, 2022 at 11:14 AM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> On Fri, 1 Jul 2022 at 01:10, Yann Ylavic <yl...@gmail.com> wrote:
> >
> > On Fri, Jul 1, 2022 at 12:00 AM Yann Ylavic <yl...@gmail.com> wrote:
> > >
> > > On Thu, Jun 30, 2022 at 7:28 PM <iv...@apache.org> wrote:
> > > >
> > > > Author: ivan
> > > > Date: Thu Jun 30 17:28:50 2022
> > > > New Revision: 1902378
> > > >
> > > > URL: http://svn.apache.org/viewvc?rev=1902378&view=rev
> > > > Log:
> > > > On 1.8.x branch: Merge r1806299, r1806301, r1806308, r1806610:
> > > > *) apr_file_write: Optimize large writes to buffered files on
> > > > Windows.
> > > []
> > > >
> > > > --- apr/apr/branches/1.8.x/file_io/win32/readwrite.c (original)
> > > > +++ apr/apr/branches/1.8.x/file_io/win32/readwrite.c Thu Jun 30 17:28:50 2022
> > > > @@ -247,6 +247,91 @@ APR_DECLARE(apr_status_t) apr_file_read(
> > > > return rv;
> > > > }
> > > >
> > > > +/* Helper function that adapts WriteFile() to apr_size_t instead
> > > > + * of DWORD. */
> > > > +static apr_status_t write_helper(HANDLE filehand, const char *buf,
> > > > + apr_size_t len, apr_size_t *pwritten)
> > > > +{
> > > > + apr_size_t remaining = len;
> > > > +
> > > > + *pwritten = 0;
> > > > + do {
> > > > + DWORD to_write;
> > > > + DWORD written;
> > > > +
> > > > + if (remaining > APR_DWORD_MAX) {
> > > > + to_write = APR_DWORD_MAX;
> > > > + }
> > > > + else {
> > > > + to_write = (DWORD)remaining;
> > > > + }
> > > > +
> > > > + if (!WriteFile(filehand, buf, to_write, &written, NULL)) {
> > > > + *pwritten += written;
> > > > + return apr_get_os_error();
> > > > + }
> > > > +
> > > > + *pwritten += written;
> > > > + remaining -= written;
> > > > + buf += written;
> > > > + } while (remaining);
> > >
> > > So there's no writev() like syscall on Windows (something that
> > > provides atomicity)?
> >
> > I found WriteFileGather
> > (https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefilegather).
> > Maybe it can help with some logic à la apr_socket_sendv()?
> >
> [[[
> Each buffer must be at least the size of a system memory page and must be aligned on a system memory page size boundary. The system writes one system memory page of data from each buffer.
> ]]]
> [[[
> The total number of bytes to be written. Each element of aSegmentArray contains a one-page chunk of this total. Because the file must be opened with FILE_FLAG_NO_BUFFERING, the number of bytes must be a multiple of the sector size of the file system where the file is located.
> ]]]
OK, I said "found it" not "looked at it in detail" :)
Too restrictive it seems (reminds me of O_DIRECT on unixes, we don't
want to use it here).
But the short-write argument stands, if Windows never short-writes
then the implicit full-write is unnecessary, but if it does
short-write then apr_file_write() has to return it too (for the user
to know), so in both cases the loop has to stop when written <
to_write (and probably return success if written > 0).
Users that don't expect short-writes should use apr_file_write_full().
Same on the apr_file_read() side.
Regards;
Yann.