On Tue, Aug 06, 2024 at 04:46:28PM +0200, Mateusz Guzik wrote:
> The flag thing is optional and can be dropped, but I think the general
> direction should be to add *more* asserts and whatnot (even if they are
> to land separately). A debug-only variant would not hurt.
Asserts do *not* clarify anything; if you want your optional flag,
come up with clear description of its semantics. In terms of state,
_not_ control flow.
> @@ -3683,6 +3685,7 @@ static const char *open_last_lookups(struct nameidata *nd,
> static int do_open(struct nameidata *nd,
> struct file *file, const struct open_flags *op)
> {
> + struct vfsmount *mnt;
> struct mnt_idmap *idmap;
> int open_flag = op->open_flag;
> bool do_truncate;
> @@ -3720,11 +3723,22 @@ static int do_open(struct nameidata *nd,
> error = mnt_want_write(nd->path.mnt);
> if (error)
> return error;
> + /*
> + * We grab an additional reference here because vfs_open_consume()
> + * may error out and free the mount from under us, while we need
> + * to undo write access below.
> + */
> + mnt = mntget(nd->path.mnt);
It's "after vfs_open_consume() we no longer own the reference in nd->path.mnt",
error or no error...
> do_truncate = true;
> }
> error = may_open(idmap, &nd->path, acc_mode, open_flag);
> - if (!error && !(file->f_mode & FMODE_OPENED))
> - error = vfs_open(&nd->path, file);
> + if (!error && !(file->f_mode & FMODE_OPENED)) {
> + BUG_ON(nd->state & ND_PATH_CONSUMED);
> + error = vfs_open_consume(&nd->path, file);
> + nd->state |= ND_PATH_CONSUMED;
> + nd->path.mnt = NULL;
> + nd->path.dentry = NULL;
Umm... The thing that feels wrong here is that you get an extra
asymmetry with ->atomic_open() ;-/ We obviously can't do that
kind of thing there (if nothing else, we have the parent directory's
inode to unlock, error or no error).
I don't hate that patch, but it really feels like the calling
conventions are not right. Let me try to tweak it a bit and
see if anything falls out...
