Re: [PATCH] libpager/data-request.c: missing call to _pager_allow_termination

[Neal H. Walfield]

At Tue, 28 Sep 2004 14:07:52 +0300,
Ognyan Kulev wrote:
> 
> 2004-09-28  Ognyan Kulev  <ogi@fmi.uni-sofia.bg>
> 
>       * data-request.c (_pager_seqnos_memory_object_data_request):
>       When pager state is not NORMAL, shorten exit path.
>       When _pager_pagemap_resize fails, add call to
>       _pager_allow_termination.
> 
> @@ -67,14 +67,16 @@ _pager_seqnos_memory_object_data_request
>     if (p->pager_state != NORMAL)
>       {
>         printf ("pager in wrong state for read\n");
> -      _pager_release_seqno (p, seqno);
> -      mutex_unlock (&p->interlock);
> -      goto allow_term_out;
> +      _pager_allow_termination (p);
> +      goto release_out;
>       }

This looks correct to me.  Normally, I would say that this sort of
micro-optimization does not matter as we don't care how slow the error
path is, however, the other jump to allow_term_out (on the fast path)
also has a gratuitous unlock relock sequence:

  /* Let someone else in.  */
  _pager_release_seqno (p, seqno);
  mutex_unlock (&p->interlock);

  if (!doread)
    goto allow_term_out;

So, if wold be useful to also move the if above the unlock and moved
the mutex_lock in allow_term_out to error_read and adjusted callers.

>     err = _pager_pagemap_resize (p, offset + length);
>     if (err)
> -    goto release_out;                /* Can't do much about the actual 
> error.  */
> +    {
> +      _pager_allow_termination (p);
> +      goto release_out;        /* Can't do much about the actual error.  */
> +    }

This only solves half the problem.  You also have to call
_pager_release_seqno as well.  (By the way, this is not the only bug
involving _pager_pagemap_resize.  data-return.c just assumes that
_pager_pagemap_resize succeeds.  I think the other callers are,
however, okay.)

Here is a new untested patch:

2004-09-28  Neal H. Walfield  <neal@cs.uml.edu>
        Ognyan Kulev  <ogi@fmi.uni-sofia.bg>

        * data-request.c (_pager_seqnos_memory_object_data_request): 
        Elide gratuitous mutex_unlock, mutex_lock sequence.
        When _pager_pagemap_resize fails, clean up correctly.

Index: data-request.c
===================================================================
RCS file: /cvsroot/hurd/hurd/libpager/data-request.c,v
retrieving revision 1.22
diff -u -p -r1.22 data-request.c
--- data-request.c      8 May 2002 09:22:14 -0000       1.22
+++ data-request.c      28 Sep 2004 15:03:43 -0000
@@ -1,5 +1,5 @@
  /* Implementation of memory_object_data_request for pager library
-   Copyright (C) 1994,95,96,97,2000,02 Free Software Foundation
+   Copyright (C) 1994,95,96,97,2000,02,04 Free Software Foundation, Inc.

     This program is free software; you can redistribute it and/or
     modify it under the terms of the GNU General Public License as
@@ -40,11 +40,11 @@ _pager_seqnos_memory_object_data_request
    if (!p)
      return EOPNOTSUPP;

-  /* Acquire the right to meddle with the pagemap */
+  /* Acquire the right to meddle with the pagemap.  */
    mutex_lock (&p->interlock);
    _pager_wait_for_seqno (p, seqno);

-  /* sanity checks -- we don't do multi-page requests yet.  */
+  /* Sanity checks -- we don't do multi-page requests yet.  */
    if (control != p->memobjcntl)
      {
        printf ("incg data request: wrong control port\n");
@@ -68,13 +68,16 @@ _pager_seqnos_memory_object_data_request
     {
       printf ("pager in wrong state for read\n");
       _pager_release_seqno (p, seqno);
-      mutex_unlock (&p->interlock);
       goto allow_term_out;
     }
 
   err = _pager_pagemap_resize (p, offset + length);
   if (err)
-    goto release_out;          /* Can't do much about the actual error.  */
+    {
+      /* Can't do much about the actual error.  */
+      _pager_release_seqno (p, seqno);
+      goto allow_term_out;
+    }
 
   /* If someone is paging this out right now, the disk contents are
      unreliable, so we have to wait.  It is too expensive (right now) to
@@ -109,10 +112,12 @@ _pager_seqnos_memory_object_data_request
 
   /* Let someone else in.  */
   _pager_release_seqno (p, seqno);
-  mutex_unlock (&p->interlock);
 
   if (!doread)
     goto allow_term_out;
+
+  mutex_unlock (&p->interlock);
+
   if (doerror)
     goto error_read;
 
@@ -133,8 +138,8 @@ _pager_seqnos_memory_object_data_request
  error_read:
   memory_object_data_error (p->memobjcntl, offset, length, EIO);
   _pager_mark_object_error (p, offset, length, EIO);
- allow_term_out:
   mutex_lock (&p->interlock);
+ allow_term_out:
   _pager_allow_termination (p);
   mutex_unlock (&p->interlock);
   ports_port_deref (p);