Re: device-mapper locking rework

["Matt Fleming" <mjf%NetBSD.org@localhost>]

2008/9/30 Adam Hamsik <haaaad%gmail.com@localhost>:
>
> On Sep,Tuesday 30 2008, at 12:57 AM, Adam Hamsik wrote:
>
>> Hi folks,
>>
>> After some discussion with rmind@ and blymn@ I tried to implement better
>> locking
>> protocol for my dm driver. I'm attaching result of my work as diff against
>> current
>> this diff probably can't be cleanly applied to current haad-dm sources
>> therefore
>> I have uploaded my sources here [1].


[Sorry, this is guaranteed to be whitespace damaged]

@@ -483,7 +484,20 @@ dmsize(dev_t dev)
 static int
 dmsize(dev_t dev)
 {
-       return dm_table_size(minor(dev));
+       struct dm_dev *dmv;
+       uint64_t size;
+
+       size = 0;
+
+       if ((dmv = dm_dev_lookup(NULL, NULL, minor(dev))) == NULL)
+                       return -ENOENT;
+
+
+       size = dm_table_size(dmv);
+
+       mutex_exit(&dmv->dev_mtx);
+
+       return size;
 }

 static void

Is the caller supposed to check for an error code and unlock the mutex?

@@ -61,19 +61,30 @@ dm_dev_lookup(const char *dm_dev_name, c
        struct dm_dev *dmv;

        dmv = NULL;
+       mutex_enter(&dm_dev_mutex);

        if (dm_dev_minor > 0)
-               if ((dmv = dm_dev_lookup_minor(dm_dev_minor)) != NULL)
+               if ((dmv = dm_dev_lookup_minor(dm_dev_minor)) != NULL){
+                       mutex_enter(&dmv->dev_mtx);
+                       mutex_exit(&dm_dev_mutex);
                        return dmv;
+               }

This function smells of deadlock to me. Is it possible that whilst we
hold dm_dev_mutex some other thread can can hold dmv->dev_mtx before
we try to mutex_enter(&dmv->dev_mtx) ?