Bug#78011: storeio (in)activation (was: Re: CDROM lock)

[Marcus Brinkmann]

Hi,

On Sun, Nov 26, 2000 at 05:16:51PM -0500, Roland McGrath wrote:
> > This is not good enough, IMHO. Alexey Dejneka sent in a patch for gnumach
> > that does the same, and it works just fine. But after doing:
> > 
> > settrans -ag /cdrom
> > 
> > you have to do
> > 
> > settrans -ag /dev/hd3
> > 
> > (or whatever your CD ROM device is) as well, or the CD ROM remains busy.
> 
> Ok.  I think there are two lines of inquiry here.  Firstly, storeio
> definitely needs to be fixed; this is a bug.  What storeio should do is
> keep track of the number of peropens and when there are no live peropens
> for the device, it should deactivate the store.  (Incidentally, I think
> that the changes I made to close and reopen the store should instead just
> clear/set the STORE_INACTIVE flag after opening with STORE_INACTIVE.)
> That will make things work right when everything is well-behaved.

Well, the patch below does just that, but it doesn't work. It seems that
opening with STORE_INACTIVE is not supported. I can imagine several
approaches to this:

1. In storeio, the first time open the store active (and skip the clear
inactive in open_hook). After that, proceed as normal (activating/inactivating).

2. In libstore, implement support for STORE_INACTIVE in store opens.

3. In storeio, use open/close instead activation flag.

4. In storeio, open the store active, but inactivate it immediately. Doh!

I think 2. is the only proper solution to this, and not hard at all (it
might be the simplest amont all. But we need to do it for every store
type seperately, I think (at least for those were the active flag has
any meaning at all). 

> For another level of robustness, we might want to think about having a
> revoke-like call at the kernel device level.

That's for later.

Marcus

diff -ru storeio.pri/ChangeLog storeio/ChangeLog
--- storeio.pri/ChangeLog       Tue Jan 30 00:43:26 2001
+++ storeio/ChangeLog   Sun Feb 18 20:57:20 2001
@@ -1,3 +1,15 @@
+2001-02-18  Marcus Brinkmann  <marcus@gnu.org>
+
+       * storeio.c (global_lock): New global variable.
+       (nperopens): Likewise.
+       (main): Initialize global_lock.
+       (open_hook): Hold global_lock and check if this is the first open.
+       If yes, activate the store.
+       (close_hook): Hold global_lock and check if this was the last open.
+       If yes, inactivate the store.
+       * dev.c (dev_open): Open the store with STORE_INACTIVE
+       (in store_parsed_open as well as in store_create).
+
 2001-01-17  Roland McGrath  <roland@frob.com>
 
        * dev.c (dev_buf_discard): Don't check AMOUNT if store_write failed.
diff -ru storeio.pri/dev.c storeio/dev.c
--- storeio.pri/dev.c   Tue Jan 30 00:43:26 2001
+++ storeio/dev.c       Sun Feb 18 20:53:59 2001
@@ -145,14 +145,15 @@
       /* This means we had no store arguments.
         We are to operate on our underlying node. */
       err = store_create (storeio_fsys->underlying,
-                         dev->readonly ? STORE_READONLY : 0,
+                         STORE_INACTIVE | (dev->readonly ? STORE_READONLY : 0),
                          0, &dev->store);
 
     }
   else
     /* Open based on the previously parsed store arguments.  */
     err = store_parsed_open (dev->store_name,
-                            dev->readonly ? STORE_READONLY : 0,
+                            STORE_INACTIVE
+                            | (dev->readonly ? STORE_READONLY : 0),
                             &dev->store);
   if (err)
     return err;
diff -ru storeio.pri/storeio.c storeio/storeio.c
--- storeio.pri/storeio.c       Tue Jan 16 17:34:12 2001
+++ storeio/storeio.c   Sun Feb 18 20:59:03 2001
@@ -32,6 +32,12 @@
 #include "open.h"
 #include "dev.h"
 
+/* Global lock.  */
+struct mutex global_lock;
+
+/* Count of active opens.  */
+int nperopens;
+
 static struct argp_option options[] =
 {
   {"readonly", 'r', 0,   0,"Disallow writing"},
@@ -124,6 +130,8 @@
   struct dev device;
   struct storeio_argp_params params;
 
+  mutex_init (&global_lock);
+
   bzero (&device, sizeof device);
   mutex_init (&device.lock);
 
@@ -233,18 +241,34 @@
 static error_t
 open_hook (struct trivfs_peropen *peropen)
 {
+  error_t err = 0;
   struct dev *const dev = peropen->cntl->hook;
+
   if (dev->store)
-    return open_create (dev, (struct open **)&peropen->hook);
-  else
-    return 0;
+    {
+      mutex_lock (&global_lock);
+      if (nperopens++ == 0)
+       err = store_clear_flags (dev->store, STORE_INACTIVE);
+      mutex_unlock (&global_lock);
+      if (!err)
+       err = open_create (dev, (struct open **)&peropen->hook);
+    }
+  return err;
 }
 
 static void
 close_hook (struct trivfs_peropen *peropen)
 {
+  struct dev *const dev = peropen->cntl->hook;
+
   if (peropen->hook)
-    open_free (peropen->hook);
+    {
+      mutex_lock (&global_lock);
+      if (--nperopens == 0)
+       store_set_flags (dev->store, STORE_INACTIVE);
+      mutex_unlock (&global_lock);
+      open_free (peropen->hook);
+    }
 }
 
 /* ---------------------------------------------------------------- */

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de