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
