Re: [RFC PATCH] Introduce filesystem type tracking

["Tom Spink" <tspink@gmail.com>]

2008/5/20 Matthew Wilcox <matthew@wil.cx>:
> On Tue, May 20, 2008 at 10:08:04PM +0100, Tom Spink wrote:
>> I've taken some more time to go over the locking semantics.  I wrote a
>> quick toy filesystem to simulate delays, blocking, memory allocation,
>> etc in the init and exit routines - and with an appropriately large
>> amount of printk's everywhere, I saw a quite a few interleavings.
>>
>> I *think* I may have got it right, but please, let me know what you
>> think!  The only thing that I think may be wrong with this patch is
>> the
>> spin_lock/unlock at the end of sget, where the superblock is
>> list_add_tailed into the super_blocks list.  I believe this opens the
>> possibility for the same superblock being list_add_tailed twice... can
>> anyone else see this code-path, and is it a problem?
>
> Hi Tom,

Hi Matthew,

> I spotted one definite bug; on failure, you leave the superblock on
> the super_blocks list.

I spotted this while I was coding, and I was careful not to let it get
added to the list...  If the ->init routine fails, the superblock
hasn't even been added to the list yet.  The patch moves this line:

list_add_tail(&s->s_list, &super_blocks);

Down to after the ->init call.

> Your locking may well be correct, but it has the hallmarks of being "a bit
> tricky" and a bit tricky means potentially buggy.  How about doing the
> nesting the other way round, ie take the mutex first, then the spinlock?

Thanks for the suggestion!

> The code needs a bit of tweaking because you don't want to put the
> superblock on any list where it can be found until it's fully
> initialised.  This may not be quite right:
>
>> +     mutex_lock(&type->fs_supers_lock);
>>       spin_lock(&sb_lock);
>>       /* should be initialized for __put_super_and_need_restart() */
>>       list_del_init(&sb->s_list);
>>       list_del(&sb->s_instances);
>>       spin_unlock(&sb_lock);
>> +
>> +     if (list_empty(&type->fs_supers) && type->exit)
>> +             type->exit();
>> +     mutex_unlock(&type->fs_supers_lock);
>> +
>>       up_write(&sb->s_umount);
>>  }
>>

I'll definitely give it a go.

> sget is a little more complex ... the fs_supers_lock would need to be
> dropped in a lot more places than I've shown here:
>
> @@ -365,11 +372,31 @@ retry:
>  retry:
> +       mutex_lock(&type->fs_supers_lock);
>        spin_lock(&sb_lock);
>
>                destroy_super(s);
>                return ERR_PTR(err);
>        }
>        s->s_type = type;
>        strlcpy(s->s_id, type->name, sizeof(s->s_id));
> +       if (list_empty(&type->fs_supers) && type->init) {
> +               spin_unlock(&sb_lock);
> +               err = type->init();
> +               if (err) {
> +                       mutex_unlock(&type->fs_supers_lock);
> +                       destroy_super(s);
> +                       return ERR_PTR(err);
> +               }
> +               spin_lock(&sb_lock);
> +       }
>        list_add_tail(&s->s_list, &super_blocks);
>        list_add(&s->s_instances, &type->fs_supers);
>        spin_unlock(&sb_lock);
> +       mutex_unlock(&type->fs_supers_lock);
>        get_filesystem(type);
>        return s;
> }

I had something similar earlier, but I thought it started to look
slightly messy when I discovered that dropping the spinlock would lead
to a racey ->init... but I hadn't thought of putting the mutex outside
the spinlock; the mutex protecting ->init and ->exit (I was getting
caught up in trying not to go to sleep inside a spinlock)

Thanks!
-- 
Tom Spink