Re: [PATCH 1/2] quota: Add mountpath based quota support

[Jan Kara]

On Thu 28-01-21 14:35:52, Christoph Hellwig wrote:
> > +	struct path path, *pathp = NULL;
> > +	struct path mountpath;
> > +	bool excl = false, thawed = false;
> > +	int ret;
> > +
> > +	cmds = cmd >> SUBCMDSHIFT;
> > +	type = cmd & SUBCMDMASK;
> 
> Personal pet peeve: it would be nice to just initialize cmds and
> type on their declaration line, or while we're at it declutter
> this a bit and remove the separate cmds variable:
> 
> 	unsigned int type = cmd & SUBCMDMASK;
> 
> 	cmd >>= SUBCMDSHIFT;

Yeah, whatever :)

> > +	/*
> > +	 * Path for quotaon has to be resolved before grabbing superblock
> > +	 * because that gets s_umount sem which is also possibly needed by path
> > +	 * resolution (think about autofs) and thus deadlocks could arise.
> > +	 */
> > +	if (cmds == Q_QUOTAON) {
> > +		ret = user_path_at(AT_FDCWD, addr,
> > +				   LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &path);
> > +		if (ret)
> > +			pathp = ERR_PTR(ret);
> > +		else
> > +			pathp = &path;
> > +	}
> > +
> > +	ret = user_path_at(AT_FDCWD, mountpoint,
> > +			     LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT, &mountpath);
> > +	if (ret)
> > +		goto out;
> 
> I don't think we need two path lookups here, we can path the same path
> to the command for quotaon.

Hum, let me think out loud. The path we pass to Q_QUOTAON is a path to
quota file - unless the filesystem stores quota in hidden files in which
case this argument is just ignored. You're right we could require that
specifically for Q_QUOTAON, the mountpoint path would actually need to
point to the quota file if it is relevant, otherwise anywhere in the
appropriate filesystem. We don't allow quota file to reside on a different
filesystem (for a past decade or so) so it should work fine.

So the only problem I have is whether requiring the mountpoint argument to
point quota file for Q_QUOTAON isn't going to be somewhat confusing to
users. At the very least it would require some careful explanation in the
manpage to explain the difference between quotactl_path() and quotactl()
in this regard. But is saving the second path for Q_QUOTAON really worth the
bother?

> > +	if (quotactl_cmd_onoff(cmds)) {
> > +		excl = true;
> > +		thawed = true;
> > +	} else if (quotactl_cmd_write(cmds)) {
> > +		thawed = true;
> > +	}
> > +
> > +	if (thawed) {
> > +		ret = mnt_want_write(mountpath.mnt);
> > +		if (ret)
> > +			goto out1;
> > +	}
> > +
> > +	sb = mountpath.dentry->d_inode->i_sb;
> > +
> > +	if (excl)
> > +		down_write(&sb->s_umount);
> > +	else
> > +		down_read(&sb->s_umount);
> 
> Given how cheap quotactl_cmd_onoff and quotactl_cmd_write are we
> could probably simplify this down do:
> 
> 	if (quotactl_cmd_write(cmd)) {

This needs to be (quotactl_cmd_write(cmd) || quotactl_cmd_onoff(cmd)).
Otherwise I agree what you suggest is somewhat more readable given how
small the function is.

> 		ret = mnt_want_write(path.mnt);
> 		if (ret)
> 			goto out1;
> 	}
> 	if (quotactl_cmd_onoff(cmd))
> 		down_write(&sb->s_umount);
> 	else
> 		down_read(&sb->s_umount);
> 
> and duplicate the checks after the do_quotactl call.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR