On Tue 11-12-18 19:14:52, Al Viro wrote:
> 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...
I guess you mean that in case we fail to lock s_umount semaphore, we'll
return -EAGAIN and traverse() will abort? That is true but since we return
-EAGAIN, callers will call into traverse() again - both do:
while ((err = traverse(m, *ppos)) == -EAGAIN) ;
and then in m_start() we will do the blocking lock on s_umount. I agree it
is ugly and twisted but it should be rare...
Now looking at the code, maybe we could avoid this weird retry dance with
traverse(). Something like following in m_show():
sb = mnt->mnt_sb;
if (mount_trylock_super())
show and done
get passive sb reference
namespace_unlock();
down_read(&sb->s_umount);
namespace_lock();
new_mnt = seq_list_start();
if (new_mnt != mnt)
retry
show and done
This could be handled completely inside m_show() so no strange retry dance
with traverse(). Do you find this better?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
