I'm sorry for the late reply, I was busy with other things and I wasn't
quite sure how I'd like this to be handled :)

On Tue 04-02-20 11:35:23, Sascha Hauer wrote:
> On Wed, Jan 29, 2020 at 01:29:29AM +0000, Al Viro wrote:
> > On Tue, Jan 28, 2020 at 11:06:31AM +0100, Sascha Hauer wrote:
> > > Hi Jan,
> > 
> > > @@ -810,6 +811,36 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
> > >  #endif
> > >  }
> > >  
> > > +static struct super_block *quotactl_path(const char __user *special, int cmd,
> > > +					 struct path *path)
> > > +{
> > > +	struct super_block *sb;
> > > +	int ret;
> > > +
> > > +	ret = user_path_at(AT_FDCWD, special, LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT,
> > > +			   path);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	sb = path->mnt->mnt_sb;
> > > +restart:
> > > +	if (quotactl_cmd_onoff(cmd))
> > > +		down_write(&sb->s_umount);
> > > +	else
> > > +		down_read(&sb->s_umount);
> > > +
> > > +	if (quotactl_cmd_write(cmd) && sb->s_writers.frozen != SB_UNFROZEN) {
> > > +		if (quotactl_cmd_onoff(cmd))
> > > +			up_write(&sb->s_umount);
> > > +		else
> > > +			up_read(&sb->s_umount);
> > > +		wait_event(sb->s_writers.wait_unfrozen,
> > > +			   sb->s_writers.frozen == SB_UNFROZEN);
> > > +		goto restart;
> > > +	}
> > > +
> > > +	return sb;
> > > +}
> > 
> > This partial duplicate of __get_super_thawed() guts does *not* belong here,
> > especially not interleaved with quota-specific checks.
> > 
> > > +	if (q_path) {
> > > +		if (quotactl_cmd_onoff(cmd))
> > > +			up_write(&sb->s_umount);
> > > +		else
> > > +			up_read(&sb->s_umount);
> > > +
> > > +		path_put(&sb_path);
> > > +	} else {
> > > +		if (!quotactl_cmd_onoff(cmds))
> > > +			drop_super(sb);
> > > +		else
> > > +			drop_super_exclusive(sb);
> > > +	}
> > 
> > Er...  Why not have the same code that you've used to lock the damn thing
> > (needs to be moved to fs/super.c) simply get a passive ref to it?  Then
> > you could do the same thing, q_path or no q_path...
> 
> I am getting confused here. To an earlier version of this series you
> responded:
> 
> > And for path-based you don't need to mess with superblock
> > references - just keep the struct path until the end.  That
> > will keep the superblock alive and active just fine.
> 
> I did that and got the objection from Jan:
> 
> > So I've realized that just looking up superblock with user_path_at() is not
> > enough. Quota code also expects that the superblock will be locked
> > (sb->s_umount) and filesystem will not be frozen (in case the quota
> > operation is going to modify the filesystem). This is needed to serialize
> > e.g. remount and quota operations or quota operations among themselves.

Yes, using passive reference is not necessary. On the other hand the
symmetry with how get_super() and friends work has some appeal too so if Al
wants that, well, he's the maintainer ;)

> So after drawing circles we now seem to be back at passive references.
> What I have now in my tree is this in fs/super.c, untested currently:

I was thinking how to make the API most sensible. In the end I've decided
for a variant that is attached - we pass in struct path which enforces
active reference to a superblock and thus we don't have to be afraid of the
superblock going away or similar problems. Also the operation "get me
superblock for a path" kind of makes sense...

Guys, what do you think?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR