On Tue, Dec 11, 2018 at 06:58:31PM +0000, Al Viro wrote:
> > +static bool mounts_trylock_super(struct proc_mounts *p, struct super_block *sb)
> > +{
> > + if (p->locked_sb == sb)
> > + return true;
> > + if (p->locked_sb) {
> > + drop_super(p->locked_sb);
> > + p->locked_sb = NULL;
> > + }
> > + if (down_read_trylock(&sb->s_umount)) {
> > + hold_sb(sb);
> > + p->locked_sb = sb;
> > + return true;
> > + }
> > + return false;
> > +}
>
> Bad calling conventions, and you are paying for those with making
> hold_sb() non-static (and having it, in the first place).
>
> > + if (mounts_trylock_super(p, sb))
> > + return p->cached_mount;
> > + /*
> > + * Trylock failed. Since namepace_sem ranks below s_umount (through
> > + * sb->s_umount > dir->i_rwsem > namespace_sem in the mount path), we
> > + * have to drop it, wait for s_umount and then try again to guarantee
> > + * forward progress.
> > + */
> > + hold_sb(sb);
>
> That. Just hoist that hold_sb() into your trylock (and put it before the
> down_read_trylock() there, while we are at it). And turn the other caller
> into
> if (unlikely(!.....))
> ret = -EAGAIN;
> else
> ret = p->show(m, &r->mnt);
> followed by unconditional drop_super(). And I would probably go for
> mount_trylock_super(&p->locked_super, sb)
> while we are at it, so that it's isolated from proc_mounts and can
> be moved to fs/super.c
Looking at it some more... I still hate it ;-/ Take a look at traverse()
in fs/seq_file.c and think what kind of clusterfuck will it cause...
