Re: [PATCH] apr_file_writev() on UNIX
Lets agree there is a bug in the way the function is used in mod_disk_cache. That will be looked at. As far as not having a bug in the !HAS_WRITEV implementation, I disagree. The current implementation does not havea singlechance of being successful if there is more than 1 vector. This does not make sense to me. Let assume the write part is successful, the function will return apr_success but has completely failed to his task. In the header file there is a remark saying that apr_file_writev is available even if the underlying os doesn't provide writev(). Why providing something that it is going to fail every single time? Is the current implementation meets the expectation of others and I am the only one that see something wrong here? Thanks, Jean-Jacques Joe Orton [EMAIL PROTECTED] 10/05/04 2:40 PM On Tue, Oct 05, 2004 at 11:23:35AM -0600, Jean-Jacques Clar wrote: [EMAIL PROTECTED] 10/05/04 12:16 AM On Mon, Oct 04, 2004 at 06:11:57PM -0600, Jean-Jacques Clar wrote: If HAS_WRITEV is not defined the current code will just push the first vector to the target. The function should write all the vectors or none. There is no guarantee that writev(2) writes all the vectors, it's allowed to return short just like write(2) is, so why should apr_file_writev() give such a guarantee only for the non-writev-based implementation? If HAS_WRITEV is not defined, there is a bug inside apr_file_writev(). Only the first vector will be written to the file. How do you think that bug should be fixed?I don't think that's a bug. apr_file_writev() should be allowed toreturn short, as the writev()-based implementation may already do thistoo. So the !HAS_WRITEV implementation is valid. apr_file_writev() is used multiple times without alternatives in mod_disk_cache.c and the current implementation is failing without informing the caller or any failure if writev() is not available.Right, the bug is in how the function is used. It's the same as callingapr_file_write() and presuming it won't return short. The fix is tocall apr_file_write_full(), in that case. In this case, there is noapr_file_writev_full(), so that should be fixed.joe
Re: [PATCH] apr_file_writev() on UNIX
On Wed, 6 Oct 2004, Jean-Jacques Clar wrote: Why providing something that it is going to fail every single time? That was my original thought. I wouldn't mind seeing both the caller as well as the !HAS_WRITEV cases fixed (or fixed if you prefer) for this reason. --Cliff
Re: [PATCH] apr_file_writev() on UNIX
At 11:12 AM 10/6/2004, you wrote: As far as not having a bug in the !HAS_WRITEV implementation, I disagree. The current implementation does not have a single chance of being successful if there is more than 1 vector. This does not make sense to me. Let assume the write part is successful, the function will return apr_success but has completely failed to his task. That's only partially true - we should never return for a blocking socket writev, only for a nonblocking one when the socket would block on the remaining vector(s). In the header file there is a remark saying that apr_file_writev is available even if the underlying os doesn't provide writev(). Why providing something that it is going to fail every single time? Does writev() return *success* on a partial-write? I very much doubt it (EINTR, EWOULDBLOCK or EAGAIN is what I would expect.) The performance of the existing code is obviously sub-par, and we should loop through all the vectors until we get a hiccup. At that point the user can retry the remaining vectors. I should make a point that it seems 'wrong' we implemented this lie - there is another property of writev() that we've ignored... writev() will gather up the vectors into a single datagram, even when nagle is off. So this implementation leaves us potentially sending dozens of datagrams for a few dozen very small buffers. It seems a true implementation would use it's own buffer to concat all of the smaller fragments into a single packet - the pain in cpu is nominal compared to the pain of multiple datagram processing. Bill
Re: [PATCH] apr_file_writev() on UNIX
On Wed, Oct 06, 2004 at 10:12:33AM -0600, Jean-Jacques Clar wrote: Lets agree there is a bug in the way the function is used in mod_disk_cache. That will be looked at. As far as not having a bug in the !HAS_WRITEV implementation, I disagree. The current implementation does not have a single chance of being successful if there is more than 1 vector. This does not make sense to me. Let assume the write part is successful, the function will return apr_success but has completely failed to his task. You're missing the point. It is valid for apr_file_writev() to return APR_SUCCESS and update *nbytes to less than the total number of bytes in the vector. The writev()-based implementation *may* do that. The non-writev-based implementation *will* do it, if passed a vector of more than one buffer. But the caller must cope with it either way. joe
Re: [PATCH] apr_file_writev() on UNIX
Joe Orton [EMAIL PROTECTED] 10/06/04 12:46 PM On Wed, Oct 06, 2004 at 10:12:33AM -0600, Jean-Jacques Clar wrote: As far as not having a bug in the !HAS_WRITEV implementation, I disagree. The current implementation does not have a single chance of being successful if there is more than 1 vector. This does not make sense to me. Let assume the write part is successful, the function will return apr_success but has completely failed to his task.You're missing the point. It is valid for apr_file_writev() to returnAPR_SUCCESS and update *nbytes to less than the total number of bytes inthe vector. The writev()-based implementation *may* do that. Thenon-writev-based implementation *will* do it, if passed a vector of morethan one buffer. But the caller must cope with it either way.Joe, I clearly understand "your" point, but could I disagree with it? I have a problem with the *will* fail. Both option should have a *may* be successful. I don't think our views on that point are reconcilable, but maybe others could share whatever they think the current way is right or wrong. Regards, Jean-Jacques
[PATCH] apr_file_writev() on UNIX
IfHAS_WRITEV is not defined the current code
will just push the first vector to the target.
The function should write all the vectors or none.
I propose the following patch, comments will be
appreciated:
Thanks for looking.
JJ
Index: srclib/apr/file_io/unix/readwrite.c===RCS file: /home/cvs/apr/file_io/unix/readwrite.c,vretrieving revision 1.90diff -u -r1.90 readwrite.c--- srclib/apr/file_io/unix/readwrite.c2 Aug 2004 09:47:48 -1.90+++ srclib/apr/file_io/unix/readwrite.c4 Oct 2004 22:41:45 -@@ -241,8 +241,22 @@ return APR_SUCCESS; }#else- *nbytes = vec[0].iov_len;- return apr_file_write(thefile, vec[0].iov_base, nbytes);+ apr_status_t rv;+ int i, bytes = 0;++ for (i = 0; i nvec; i++) {+ *nbytes = vec[i].iov_len;+ if ((rv = apr_file_write(thefile, vec[i].iov_base, nbytes)) != APR_SUCCESS) {+ *nbytes = bytes;+ return rv;+ }+ else {+ bytes += *nbytes;+ }+ }+ *nbytes = bytes;+ + return APR_SUCCESS;#endif}
Re: [PATCH] apr_file_writev() on UNIX
On Mon, 4 Oct 2004, Jean-Jacques Clar wrote:
If HAS_WRITEV is not defined the current code
will just push the first vector to the target.
Oooh, that sucks. Good catch.
The function should write all the vectors or none.
I propose the following patch, comments will be
+1, with the following nit:
+apr_status_t rv;
+int i, bytes = 0;
+
+for (i = 0; i nvec; i++) {
+*nbytes = vec[i].iov_len;
Is there really a point to the above line? *nbytes will always be
overwritten before returning anyway, so why not just skip this line and
then:
+if ((rv = apr_file_write(thefile, vec[i].iov_base, nbytes)) !=
APR_SUCCESS) {
+*nbytes = bytes;
+return rv;
+}
+else {
+bytes += *nbytes;
bytes += vec[i].iov_len;
+}
+}
+*nbytes = bytes;
+
+return APR_SUCCESS;
Thoughts?
--Cliff
Re: [PATCH] apr_file_writev() on UNIX
Cliff Woolley [EMAIL PROTECTED] 10/04/04 7:08 PM
+ *nbytes = vec[i].iov_len;Is there really a point to the above line? *nbytes will always beoverwritten before returning anyway, so why not just skip this line andthen: + if ((rv = apr_file_write(thefile, vec[i].iov_base, nbytes)) != APR_SUCCESS) { + *nbytes = bytes; + return rv; + } + else { + bytes += *nbytes;bytes += vec[i].iov_len;Thoughts?
I thought that the nbytes on the call for apr_file_write() is the number of bytes to write.
Does that info needs to be passed in?
Should then be set to iov_len before the call. What do I miss there?
Thanks,
JJ
Re: [PATCH] apr_file_writev() on UNIX
On Mon, 4 Oct 2004, Jean-Jacques Clar wrote: +if ((rv = apr_file_write(thefile, vec[i].iov_base, nbytes)) I thought that the nbytes on the call for apr_file_write() is the number of bytes to write. Oh crap, I'm sorry, I totally missed that. *blush* +1 to the patch as-is. --Cliff
Re: [PATCH] apr_file_writev() on UNIX
On Mon, Oct 04, 2004 at 06:11:57PM -0600, Jean-Jacques Clar wrote: If HAS_WRITEV is not defined the current code will just push the first vector to the target. The function should write all the vectors or none. There is no guarantee that writev(2) writes all the vectors, it's allowed to return short just like write(2) is, so why should apr_file_writev() give such a guarantee only for the non-writev-based implementation? What you're looking for, perhaps, is an apr_file_writev_full(), which guarantees all bytes are written, or failure? joe
Re: [PATCH] apr_file_writev() on UNIX
[EMAIL PROTECTED] 10/05/04 12:16 AM On Mon, Oct 04, 2004 at 06:11:57PM -0600, Jean-Jacques Clar wrote: If HAS_WRITEV is not defined the current code will just push the first vector to the target. The function should write all the vectors or none.There is no guarantee that writev(2) writes all the vectors, it'sallowed to return short just like write(2) is, so why shouldapr_file_writev() give such a guarantee only for the non-writev-basedimplementation?If HAS_WRITEV is not defined, there is a bug inside apr_file_writev(). Only the first vector will be written to the file. How do you think that bug should be fixed? apr_file_writev() is used multiple times without alternatives in mod_disk_cache.c and the current implementation is failing without informing the caller or any failure if writev() is not available. Do you see a better solution than fixing apr_file_writev() for that case? Maybe the question is why do we need to have the HAS_WRITEV define? Are there any Unix based system that do not have writev() implemented? What you're looking for, perhaps, is an apr_file_writev_full(), whichguarantees "all bytes are written, or failure"? I am not looking for replacement, there arevectors that needs to be written to a filein the current case. Thanks, JJ
Re: [PATCH] apr_file_writev() on UNIX
On Tue, Oct 05, 2004 at 11:23:35AM -0600, Jean-Jacques Clar wrote: [EMAIL PROTECTED] 10/05/04 12:16 AM On Mon, Oct 04, 2004 at 06:11:57PM -0600, Jean-Jacques Clar wrote: If HAS_WRITEV is not defined the current code will just push the first vector to the target. The function should write all the vectors or none. There is no guarantee that writev(2) writes all the vectors, it's allowed to return short just like write(2) is, so why should apr_file_writev() give such a guarantee only for the non-writev-based implementation? If HAS_WRITEV is not defined, there is a bug inside apr_file_writev(). Only the first vector will be written to the file. How do you think that bug should be fixed? I don't think that's a bug. apr_file_writev() should be allowed to return short, as the writev()-based implementation may already do this too. So the !HAS_WRITEV implementation is valid. apr_file_writev() is used multiple times without alternatives in mod_disk_cache.c and the current implementation is failing without informing the caller or any failure if writev() is not available. Right, the bug is in how the function is used. It's the same as calling apr_file_write() and presuming it won't return short. The fix is to call apr_file_write_full(), in that case. In this case, there is no apr_file_writev_full(), so that should be fixed. joe
Re: [PATCH] apr_file_writev() on UNIX
On Tue, 5 Oct 2004, Joe Orton wrote: Right, the bug is in how the function is used. It's the same as calling apr_file_write() and presuming it won't return short. The fix is to call apr_file_write_full(), in that case. In this case, there is no apr_file_writev_full(), so that should be fixed. Sounds like a reasonable explanation. Thanks, Joe. --Cliff
