Re: [Final call for testers][PATCH] superblock handling changes

[Alexander Viro]

On Fri, 15 Jun 2001, Matthew Wilcox wrote:

> > +	while (sb != sb_entry(&super_blocks))
> > +		if (sb->s_dirt) {
> > +			sb->s_count++;
			^^^^^^^^^^^^^^
> > +			spin_unlock(&sb_lock);
> > +			down_read(&sb->s_umount);
> > +			write_super(sb);
> > +			drop_super(sb);
> > +			goto restart;
> > +		} else
> > +			sb = sb_entry(sb->s_list.next);
> > +	spin_unlock(&sb_lock);
> 
> I think this could be clearer.
> 
> 	struct list_head *tmp;
> restart:
> 	spin_lock(&sb_lock);
> 	list_for_each(tmp, super_blocks) {
> 		struct super_block *sb = sb_entry(tmp);
> 		if (!sb->s_dirt)
> 			continue;
> 		spin_unlock(&sb_lock);
> 		down_read(&sb->s_umount);
> 		write_super(sb);
> 		drop_super(sb);
> 		goto restart;
> 	}
> 	spin_unlock(&sb_lock);

Aside of the missing ->s_count++ - no arguments.

> > @@ -773,16 +810,16 @@
> >  				       void *data, int silent)
> >  {
> >  	struct super_block * s;
> > -	s = get_empty_super();
> > +	s = alloc_super();
> >  	if (!s)
> >  		goto out;
> >  	s->s_dev = dev;
> >  	s->s_bdev = bdev;
> >  	s->s_flags = flags;
> > -	s->s_dirt = 0;
> >  	s->s_type = type;
> > -	s->s_dquot.flags = 0;
> > -	s->s_maxbytes = MAX_NON_LFS;
> > +	spin_lock(&sb_lock);
> > +	list_add (&s->s_list, super_blocks.prev);
> 
> I'd use list_add_tail(&s->s_list, super_blocks);

Umm... Why? I've no problems with either variant, but I really see no
clear win (or loss) in list_add_tail here. If there is some code that
relies on the order in that list it's badly broken - remember, we used
to reuse unmounted superblocks, so order might be almost arbitrary.
Not even "root is first", whatever value that might have - FS_SINGLE
filesystems ended up before the root.