| Message ID | 20210128141713.25223-2-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] quota: Add mountpath based quota support | expand |
> + 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.
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
On Tue, Feb 02, 2021 at 07:02:41PM +0100, Jan Kara wrote: > 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? I find the doubled path argument a really horrible API, so I'd pretty strongly prefer to avoid that. > > 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. The way I read quotactl_cmd_write, it only special cases a few commands and returns 0 there, so we should not need the extra quotactl_cmd_onoff call, as all those commands are not explicitly listed.
On Thu 04-02-21 07:34:14, Christoph Hellwig wrote: > On Tue, Feb 02, 2021 at 07:02:41PM +0100, Jan Kara wrote: > > 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? > > I find the doubled path argument a really horrible API, so I'd pretty > strongly prefer to avoid that. Honestly, I don't understand why is it so horrible. The paths point to different things... The first path identifies the filesystem to operate on, the second path identifies the file which contains quota accounting data. In the ancient times, the file with quota accounting data could even be stored on a different filesystem (these were still times when filesystem metadata journalling was a new thing - like late 90's). But later I just disallowed that because it was not very useful (and luckily even used) and just complicated matters. Anyway, back to 2021 :). What I find somewhat confusing about a single path for Q_QUOTAON is that for any other quotactl, any path on the filesystem is fine. Similarly if quota data is stored in the hidden file, any path on the filesystem is fine. It is only for Q_QUOTAON on a filesystem where quota data is stored in a normal file, where we suddently require that the path has to point to it. Now quota data stored in a normal file is a setup we try to deprecate anyway so another option is to just leave quotactl_path() only for those setups where quota metadata is managed by the filesystem so we don't need to pass quota files to Q_QUOTAON? > > > 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. > > The way I read quotactl_cmd_write, it only special cases a few commands > and returns 0 there, so we should not need the extra quotactl_cmd_onoff > call, as all those commands are not explicitly listed. Right, sorry, I was mistaken. Honza
On Thu, Feb 04, 2021 at 01:53:50PM +0100, Jan Kara wrote: > Now quota data stored in a normal file is a setup we try to deprecate > anyway so another option is to just leave quotactl_path() only for those > setups where quota metadata is managed by the filesystem so we don't need > to pass quota files to Q_QUOTAON? I'd be perfectly fine with that.
On Tue 09-02-21 08:51:01, Christoph Hellwig wrote: > On Thu, Feb 04, 2021 at 01:53:50PM +0100, Jan Kara wrote: > > Now quota data stored in a normal file is a setup we try to deprecate > > anyway so another option is to just leave quotactl_path() only for those > > setups where quota metadata is managed by the filesystem so we don't need > > to pass quota files to Q_QUOTAON? > > I'd be perfectly fine with that. OK, then this looks like the best way forward to me. We can always extend the quotactl_path() syscall later if we find this is problematic for some real usecases. Honza
diff --git a/fs/quota/quota.c b/fs/quota/quota.c index 6d16b2be5ac4..9ac09e128686 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -17,6 +17,7 @@ #include <linux/capability.h> #include <linux/quotaops.h> #include <linux/types.h> +#include <linux/mount.h> #include <linux/writeback.h> #include <linux/nospec.h> #include "compat.h" @@ -968,3 +969,79 @@ SYSCALL_DEFINE4(quotactl, unsigned int, cmd, const char __user *, special, path_put(pathp); return ret; } + +SYSCALL_DEFINE4(quotactl_path, unsigned int, cmd, const char __user *, + mountpoint, qid_t, id, void __user *, addr) +{ + uint cmds, type; + struct super_block *sb = NULL; + struct path path, *pathp = NULL; + struct path mountpath; + bool excl = false, thawed = false; + int ret; + + cmds = cmd >> SUBCMDSHIFT; + type = cmd & SUBCMDMASK; + + if (type >= MAXQUOTAS) + return -EINVAL; + + if (!mountpoint) + return -ENODEV; + + /* + * 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; + + 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); + + ret = do_quotactl(sb, type, cmds, id, addr, pathp); + + if (excl) + up_write(&sb->s_umount); + else + up_read(&sb->s_umount); + + if (thawed) + mnt_drop_write(mountpath.mnt); +out1: + path_put(&mountpath); + +out: + if (pathp && !IS_ERR(pathp)) + path_put(pathp); + return ret; +}
Add syscall quotactl_path, a variant of quotactl which allows to specify the mountpath instead of a path of to a block device. The quotactl syscall expects a path to the mounted block device to specify the filesystem to work on. This limits usage to filesystems which actually have a block device. quotactl_path replaces the path to the block device with a path where the filesystem is mounted at. The global Q_SYNC command to sync all filesystems is not supported for this new syscall, otherwise quotactl_path behaves like quotactl. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- fs/quota/quota.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+)