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

[Christoph Hellwig <hch@infradead.org>]

> +	uint cmds, type;
> +	struct super_block *sb = NULL;

I don't think sb needs the NULL initialization.

> +	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;

> +	/*
> +	 * 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.


> +	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)) {
		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.