2008/5/20 Matthew Wilcox <matthew@xxxxxx>:
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
