Re: [PATCH 1/8] quota: Allow to pass mount path to quotactl

[Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>]

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.

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:

static bool __grab_super_thawed(struct super_block *sb, bool excl)
{
	while (1) {
		bool again = false;

		spin_lock(&sb_lock);

		if (hlist_unhashed(&sb->s_instances)) {
			spin_unlock(&sb_lock);
			return false;
		}

		sb->s_count++;
		spin_unlock(&sb_lock);

		if (excl)
			down_write(&sb->s_umount);
		else
			down_read(&sb->s_umount);

		if (sb->s_root && (sb->s_flags & SB_BORN)) {
			if (sb->s_writers.frozen == SB_UNFROZEN)
				return true;
			else
				again = true;
		}

		if (excl)
			up_write(&sb->s_umount);
		else
			up_read(&sb->s_umount);

		if (again)
			wait_event(sb->s_writers.wait_unfrozen,
				   sb->s_writers.frozen == SB_UNFROZEN);

		put_super(sb);

		if (!again)
			return false;
	}

	return ret;
}

int grab_super_thawed(struct super_block *sb)
{
	return __grab_super_thawed(sb, false);
}

int grab_super_exclusive_thawed(struct super_block *sb)
{
	return __grab_super_thawed(sb, true);
}

Does this look ok now?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |