You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by je...@apache.org on 2007/02/28 02:47:32 UTC
svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Author: jerenkrantz
Date: Tue Feb 27 17:47:32 2007
New Revision: 512557
URL: http://svn.apache.org/viewvc?view=rev&rev=512557
Log:
Use buffered I/O with SDBM.
Submitted by: Joe Schaefer
Reviewed by: Justin Erenkrantz
Modified:
apr/apr-util/trunk/CHANGES
apr/apr-util/trunk/dbm/sdbm/sdbm.c
Modified: apr/apr-util/trunk/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/CHANGES?view=diff&rev=512557&r1=512556&r2=512557
==============================================================================
--- apr/apr-util/trunk/CHANGES (original)
+++ apr/apr-util/trunk/CHANGES Tue Feb 27 17:47:32 2007
@@ -1,5 +1,7 @@
Changes with APR-util 1.3.0
+ *) Use buffered I/O with SDBM. [Joe Schaefer]
+
*) Unify parsing of prepared statements and add binary argument functions
to DBD [Bojan Smojver with help from many on the APR list]
Modified: apr/apr-util/trunk/dbm/sdbm/sdbm.c
URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/dbm/sdbm/sdbm.c?view=diff&rev=512557&r1=512556&r2=512557
==============================================================================
--- apr/apr-util/trunk/dbm/sdbm/sdbm.c (original)
+++ apr/apr-util/trunk/dbm/sdbm/sdbm.c Tue Feb 27 17:47:32 2007
@@ -120,7 +120,7 @@
flags &= ~APR_SHARELOCK;
}
- flags |= APR_BINARY | APR_READ;
+ flags |= APR_BINARY | APR_READ | APR_BUFFERED;
/*
* open the files in sequence, and stat the dirfile.
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/28/07, Joe Orton <jo...@redhat.com> wrote:
> No, that sounds fine, or just moving your change up inside the !(flags &
> APR_WRITE) condition so that all apr_sdbm_* users benefit equally.
Latter done in r512842. Thanks. -- justin
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/28/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Another alternative, we can either write lock the file by all readers,
> or perhaps easier but probably more cpu intensive, lstat to pick up the
> modification stamp?
SDBM already takes out a shared file lock when opening the file - so
it does exactly this.
If you want to remove BUFFERED when SHARELOCK is set, please be my
guest as the DBD layers don't use that flag. -- justin
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Another alternative, we can either write lock the file by all readers,
or perhaps easier but probably more cpu intensive, lstat to pick up the
modification stamp?
I'm still apprehensive about the assumptions here, if our buffering was
block oriented this wouldn't be an issue, but the SDBM is block oriented
and our buffering is stream oriented.
William A. Rowe, Jr. wrote:
> Joe Orton wrote:
>> On Wed, Feb 28, 2007 at 08:29:10AM -0800, Justin Erenkrantz wrote:
>>> The performance implications of not doing buffered reads just *kills*
>>> our server - so we need to do something and adding buffering lessens
>>> the load quite dramatically. In our situation, we couldn't care less
>>> about modifying the file - we only care to optimize the read-only case
>>> - and I believe that code is just fine and stable. Though I guess I'd
>>> prefer we fix the problems with buffering if they do exist.
>>>
>>> Would you be concerned if we added APR_BUFFERED to APR_DBM_DBMODE_RO
>>> for sdbm? -- justin
>> No, that sounds fine, or just moving your change up inside the !(flags &
>> APR_WRITE) condition so that all apr_sdbm_* users benefit equally.
>
> -1 on this change unless you compare the datum you do hit that it's still
> the record you expected. If we hit the wrong record (our copy of the
> buffered index page mismatched the found page) we must flush and retry
> once. If the retry fails, then error out.
>
> Bill
>
>
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Wed, Feb 28, 2007 at 08:29:10AM -0800, Justin Erenkrantz wrote:
>> The performance implications of not doing buffered reads just *kills*
>> our server - so we need to do something and adding buffering lessens
>> the load quite dramatically. In our situation, we couldn't care less
>> about modifying the file - we only care to optimize the read-only case
>> - and I believe that code is just fine and stable. Though I guess I'd
>> prefer we fix the problems with buffering if they do exist.
>>
>> Would you be concerned if we added APR_BUFFERED to APR_DBM_DBMODE_RO
>> for sdbm? -- justin
>
> No, that sounds fine, or just moving your change up inside the !(flags &
> APR_WRITE) condition so that all apr_sdbm_* users benefit equally.
-1 on this change unless you compare the datum you do hit that it's still
the record you expected. If we hit the wrong record (our copy of the
buffered index page mismatched the found page) we must flush and retry
once. If the retry fails, then error out.
Bill
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by Jim Jagielski <ji...@devsys.jaguNET.com>.
Joe Orton wrote:
>
> On Wed, Feb 28, 2007 at 08:29:10AM -0800, Justin Erenkrantz wrote:
> > The performance implications of not doing buffered reads just *kills*
> > our server - so we need to do something and adding buffering lessens
> > the load quite dramatically. In our situation, we couldn't care less
> > about modifying the file - we only care to optimize the read-only case
> > - and I believe that code is just fine and stable. Though I guess I'd
> > prefer we fix the problems with buffering if they do exist.
> >
> > Would you be concerned if we added APR_BUFFERED to APR_DBM_DBMODE_RO
> > for sdbm? -- justin
>
> No, that sounds fine, or just moving your change up inside the !(flags &
> APR_WRITE) condition so that all apr_sdbm_* users benefit equally.
>
+1
--
===========================================================================
Jim Jagielski [|] jim@jaguNET.com [|] http://www.jaguNET.com/
"If you can dodge a wrench, you can dodge a ball."
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Feb 28, 2007 at 08:29:10AM -0800, Justin Erenkrantz wrote:
> The performance implications of not doing buffered reads just *kills*
> our server - so we need to do something and adding buffering lessens
> the load quite dramatically. In our situation, we couldn't care less
> about modifying the file - we only care to optimize the read-only case
> - and I believe that code is just fine and stable. Though I guess I'd
> prefer we fix the problems with buffering if they do exist.
>
> Would you be concerned if we added APR_BUFFERED to APR_DBM_DBMODE_RO
> for sdbm? -- justin
No, that sounds fine, or just moving your change up inside the !(flags &
APR_WRITE) condition so that all apr_sdbm_* users benefit equally.
joe
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/28/07, Joe Orton <jo...@redhat.com> wrote:
> The caller could already pass in APR_BUFFERED in the mode parameter to
> apr_sdbm_open(), AFAICS.
Well, that doesn't help apr_dbm_* - which doesn't permit such flags to
be passed. I only realized after I committed that we even had a
bypass mechanism for apr_dbm_* which allows SDBM to be utilized -
ugly!
> I have very little trust in the buffered I/O code, people have been very
> fixing basic bugs in it all through 1.2.x, and there are a couple more
> reported in bugzilla already. So I wouldn't call this low-risk.
>
> http://issues.apache.org/bugzilla/show_bug.cgi?id=40963 could easily
> affect sdbm.
What would you suggest?
The performance implications of not doing buffered reads just *kills*
our server - so we need to do something and adding buffering lessens
the load quite dramatically. In our situation, we couldn't care less
about modifying the file - we only care to optimize the read-only case
- and I believe that code is just fine and stable. Though I guess I'd
prefer we fix the problems with buffering if they do exist.
Would you be concerned if we added APR_BUFFERED to APR_DBM_DBMODE_RO
for sdbm? -- justin
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Sorry if this is feeling like fetch me a rock, I was offering suggestions.
-1 to the code committed this week if there is no APR_NOTBUFFERED flag
to apr_sdbm_open(), period.
So, how to infer APR_BUFFERED? Since apr_dbm_open doesn't pass APR_SHARELOCK
it can safely pass APR_BUFFERED from its sdbm open delegate function.
Otherwise, for callers of apr_sdbm_open, honor APR_BUFFERED and let's
just update apr_sdbm_open.
Justin Erenkrantz wrote:
> On 2/28/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>> Sounds great. Let's move this hack to apr_dbm_open's delegate for
>> apr_sdbm_open, and simply ensure apr_sdbm_open honors the APR_BUFFERED
>> flag.
>>
>> If this is moved to apr_dbm_, and this flag true for any SDBM that isn't
>> APR_SHARELOCK'ed, then you'll turn my -.99 to a +1.
>
> Is below acceptable to you? I'm getting tired of playing fetch me a
> rock here.... -- justin
>
> Index: dbm/sdbm/sdbm.c
> ===================================================================
> --- dbm/sdbm/sdbm.c (revision 513019)
> +++ dbm/sdbm/sdbm.c (working copy)
> @@ -103,7 +103,6 @@
> */
> if (!(flags & APR_WRITE)) {
> db->flags |= SDBM_RDONLY;
> - flags |= APR_BUFFERED;
> }
>
> /*
> Index: dbm/apr_dbm_sdbm.c
> ===================================================================
> --- dbm/apr_dbm_sdbm.c (revision 513019)
> +++ dbm/apr_dbm_sdbm.c (working copy)
> @@ -54,7 +54,7 @@
> #define APR_DBM_NEXTKEY(f, k, nk) apr_sdbm_nextkey(f, &(nk))
> #define APR_DBM_FREEDPTR(dptr) NOOP_FUNCTION
> -#define APR_DBM_DBMODE_RO APR_READ
> +#define APR_DBM_DBMODE_RO (APR_READ | APR_BUFFERED)
> #define APR_DBM_DBMODE_RW (APR_READ | APR_WRITE)
> #define APR_DBM_DBMODE_RWCREATE (APR_READ | APR_WRITE | APR_CREATE)
> #define APR_DBM_DBMODE_RWTRUNC (APR_READ | APR_WRITE | APR_CREATE | \
>
>
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
This patch looks right, +1 The last comment vetoed the code in trunk,
not your patch below, and explained my rational.
Sorry for confusion, and thanks!
Bill
Justin Erenkrantz wrote:
> On 2/28/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>> Sounds great. Let's move this hack to apr_dbm_open's delegate for
>> apr_sdbm_open, and simply ensure apr_sdbm_open honors the APR_BUFFERED
>> flag.
>>
>> If this is moved to apr_dbm_, and this flag true for any SDBM that isn't
>> APR_SHARELOCK'ed, then you'll turn my -.99 to a +1.
>
> Is below acceptable to you? I'm getting tired of playing fetch me a
> rock here.... -- justin
>
> Index: dbm/sdbm/sdbm.c
> ===================================================================
> --- dbm/sdbm/sdbm.c (revision 513019)
> +++ dbm/sdbm/sdbm.c (working copy)
> @@ -103,7 +103,6 @@
> */
> if (!(flags & APR_WRITE)) {
> db->flags |= SDBM_RDONLY;
> - flags |= APR_BUFFERED;
> }
>
> /*
> Index: dbm/apr_dbm_sdbm.c
> ===================================================================
> --- dbm/apr_dbm_sdbm.c (revision 513019)
> +++ dbm/apr_dbm_sdbm.c (working copy)
> @@ -54,7 +54,7 @@
> #define APR_DBM_NEXTKEY(f, k, nk) apr_sdbm_nextkey(f, &(nk))
> #define APR_DBM_FREEDPTR(dptr) NOOP_FUNCTION
> -#define APR_DBM_DBMODE_RO APR_READ
> +#define APR_DBM_DBMODE_RO (APR_READ | APR_BUFFERED)
> #define APR_DBM_DBMODE_RW (APR_READ | APR_WRITE)
> #define APR_DBM_DBMODE_RWCREATE (APR_READ | APR_WRITE | APR_CREATE)
> #define APR_DBM_DBMODE_RWTRUNC (APR_READ | APR_WRITE | APR_CREATE | \
>
>
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/28/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Sounds great. Let's move this hack to apr_dbm_open's delegate for
> apr_sdbm_open, and simply ensure apr_sdbm_open honors the APR_BUFFERED
> flag.
>
> If this is moved to apr_dbm_, and this flag true for any SDBM that isn't
> APR_SHARELOCK'ed, then you'll turn my -.99 to a +1.
Is below acceptable to you? I'm getting tired of playing fetch me a
rock here.... -- justin
Index: dbm/sdbm/sdbm.c
===================================================================
--- dbm/sdbm/sdbm.c (revision 513019)
+++ dbm/sdbm/sdbm.c (working copy)
@@ -103,7 +103,6 @@
*/
if (!(flags & APR_WRITE)) {
db->flags |= SDBM_RDONLY;
- flags |= APR_BUFFERED;
}
/*
Index: dbm/apr_dbm_sdbm.c
===================================================================
--- dbm/apr_dbm_sdbm.c (revision 513019)
+++ dbm/apr_dbm_sdbm.c (working copy)
@@ -54,7 +54,7 @@
#define APR_DBM_NEXTKEY(f, k, nk) apr_sdbm_nextkey(f, &(nk))
#define APR_DBM_FREEDPTR(dptr) NOOP_FUNCTION
-#define APR_DBM_DBMODE_RO APR_READ
+#define APR_DBM_DBMODE_RO (APR_READ | APR_BUFFERED)
#define APR_DBM_DBMODE_RW (APR_READ | APR_WRITE)
#define APR_DBM_DBMODE_RWCREATE (APR_READ | APR_WRITE | APR_CREATE)
#define APR_DBM_DBMODE_RWTRUNC (APR_READ | APR_WRITE | APR_CREATE | \
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Wed, Feb 28, 2007 at 01:12:05AM -0800, Justin Erenkrantz wrote:
>> On 2/28/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>>> If this was a flag to apr_sdbm_open, or was modified to interact with
>>> the existing locking logic, I'd have much more faith that this is
>>> a reasonable approach.
>
> The caller could already pass in APR_BUFFERED in the mode parameter to
> apr_sdbm_open(), AFAICS.
Sounds great. Let's move this hack to apr_dbm_open's delegate for
apr_sdbm_open, and simply ensure apr_sdbm_open honors the APR_BUFFERED
flag.
If this is moved to apr_dbm_, and this flag true for any SDBM that isn't
APR_SHARELOCK'ed, then you'll turn my -.99 to a +1.
If they want to combine any bogus combination of APR_BUFFERED and also
APR_SHARELOCK in an app's call to apr_sdbm_open() they can be our guest,
maybe they will hit the wall and study the correct hacks to make these
behave politely together.
Bill
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Feb 28, 2007 at 01:12:05AM -0800, Justin Erenkrantz wrote:
> On 2/28/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> >If this was a flag to apr_sdbm_open, or was modified to interact with
> >the existing locking logic, I'd have much more faith that this is
> >a reasonable approach.
The caller could already pass in APR_BUFFERED in the mode parameter to
apr_sdbm_open(), AFAICS.
> I would appreciate it if you present a specific case where this breaks
> something rather than vague generalities about how it might break
> something. As I said earlier, I view this as a very low-risk change
> as SDBM uses APR's file locking semantics and doesn't support
> concurrent reader/writer combinations. If you want that facility via
> BDB, you need newer versions of BDB (and even that tends to barf all
> over itself on load).
I have very little trust in the buffered I/O code, people have been very
fixing basic bugs in it all through 1.2.x, and there are a couple more
reported in bugzilla already. So I wouldn't call this low-risk.
http://issues.apache.org/bugzilla/show_bug.cgi?id=40963 could easily
affect sdbm.
joe
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/28/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> There's a signficant difference between not calling flush 'anytime soon'
> and lazy writes, however :)
No - not really.
> If this was a flag to apr_sdbm_open, or was modified to interact with
> the existing locking logic, I'd have much more faith that this is
> a reasonable approach.
I would appreciate it if you present a specific case where this breaks
something rather than vague generalities about how it might break
something. As I said earlier, I view this as a very low-risk change
as SDBM uses APR's file locking semantics and doesn't support
concurrent reader/writer combinations. If you want that facility via
BDB, you need newer versions of BDB (and even that tends to barf all
over itself on load).
If we do determine there is a case where it would break when SHARELOCK
is present (which I'm doubtful works given it doesn't even try to
flush), then we can either not set the buffering when APR_SHARELOCK is
set, or move the flag into apr_dbm's usage of SDBM - which, I hope we
agree, certainly doesn't permit multiple reader/writers with SDBM at
all. -- justin
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Justin Erenkrantz wrote:
> On 2/27/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>> Any thread safety implications here? If the data isn't refreshed
>> between writes, it seems this could be a major hassle.
>
> I don't think this opens us up to any more issues than we already have
> with the current code. The APR file buffers are thread-safe in and of
> themselves - essentially this doesn't alter anything except that we're
> allowing APR to cache - the same as the FS or OS might do on its own.
> You could not do buffering if the DBM was open for writing, but
> really, I don't think that'd change anything of substance here as the
> code isn't calling flush() or similar after each modification - so
> there's never been a guarantee that changes would be committed on disk
> at the instant the DBM was changed in memory. -- justin
There's a signficant difference between not calling flush 'anytime soon'
and lazy writes, however :)
If this was a flag to apr_sdbm_open, or was modified to interact with
the existing locking logic, I'd have much more faith that this is
a reasonable approach.
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 2/27/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> Any thread safety implications here? If the data isn't refreshed
> between writes, it seems this could be a major hassle.
I don't think this opens us up to any more issues than we already have
with the current code. The APR file buffers are thread-safe in and of
themselves - essentially this doesn't alter anything except that we're
allowing APR to cache - the same as the FS or OS might do on its own.
You could not do buffering if the DBM was open for writing, but
really, I don't think that'd change anything of substance here as the
code isn't calling flush() or similar after each modification - so
there's never been a guarantee that changes would be committed on disk
at the instant the DBM was changed in memory. -- justin
Re: svn commit: r512557 - in /apr/apr-util/trunk: CHANGES dbm/sdbm/sdbm.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Any thread safety implications here? If the data isn't refreshed
between writes, it seems this could be a major hassle.
jerenkrantz@apache.org wrote:
> Author: jerenkrantz
> Date: Tue Feb 27 17:47:32 2007
> New Revision: 512557
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=512557
> Log:
> Use buffered I/O with SDBM.
>
> Submitted by: Joe Schaefer
> Reviewed by: Justin Erenkrantz
>
> Modified:
> apr/apr-util/trunk/CHANGES
> apr/apr-util/trunk/dbm/sdbm/sdbm.c
>
> Modified: apr/apr-util/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/CHANGES?view=diff&rev=512557&r1=512556&r2=512557
> ==============================================================================
> --- apr/apr-util/trunk/CHANGES (original)
> +++ apr/apr-util/trunk/CHANGES Tue Feb 27 17:47:32 2007
> @@ -1,5 +1,7 @@
> Changes with APR-util 1.3.0
>
> + *) Use buffered I/O with SDBM. [Joe Schaefer]
> +
> *) Unify parsing of prepared statements and add binary argument functions
> to DBD [Bojan Smojver with help from many on the APR list]
>
>
> Modified: apr/apr-util/trunk/dbm/sdbm/sdbm.c
> URL: http://svn.apache.org/viewvc/apr/apr-util/trunk/dbm/sdbm/sdbm.c?view=diff&rev=512557&r1=512556&r2=512557
> ==============================================================================
> --- apr/apr-util/trunk/dbm/sdbm/sdbm.c (original)
> +++ apr/apr-util/trunk/dbm/sdbm/sdbm.c Tue Feb 27 17:47:32 2007
> @@ -120,7 +120,7 @@
> flags &= ~APR_SHARELOCK;
> }
>
> - flags |= APR_BINARY | APR_READ;
> + flags |= APR_BINARY | APR_READ | APR_BUFFERED;
>
> /*
> * open the files in sequence, and stat the dirfile.
>
>
>
>