Messages in this thread | | | Date | Thu, 28 Jan 2021 14:35:52 +0000 | From | Christoph Hellwig <> | Subject | Re: [PATCH 1/2] quota: Add mountpath based quota support |
| |
> + 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.
| |