Re: [RFC][PATCH v4 38/69] do_last(): simplify the liveness analysis past finish_open_created

Al Viro
2020-03-15T14:34:32Z

On Sun, Mar 15, 2020 at 08:40:07PM +0800, Hillf Danton wrote:
>
> On Fri, 13 Mar 2020 23:53:26 +0000
> > From: Al Viro <viro@zeniv.linux.org.uk>
> >
> > Don't mess with got_write there - it is guaranteed to be false on
> > entry and it will be set true if and only if we decide to go for
> > truncation and manage to get write access for that.
> >
> > Don't carry acc_mode through the entire thing - it's only used
> > in that part. And don't bother with gotos in there - compiler is
> > quite capable of optimizing that.
> >
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > fs/namei.c | 28 +++++++++++-----------------
> > 1 file changed, 11 insertions(+), 17 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index c85fdfd6b33d..8cdf8ef41194 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3122,9 +3122,9 @@ static const char *do_last(struct nameidata *nd,
> > kuid_t dir_uid = nd->inode->i_uid;
> > umode_t dir_mode = nd->inode->i_mode;
> > int open_flag = op->open_flag;
> > - bool will_truncate = (open_flag & O_TRUNC) != 0;
> > + bool do_truncate;
> > bool got_write = false;
>
> Without got_write cut,
>
> > - int acc_mode = op->acc_mode;
> > + int acc_mode;
> > unsigned seq;
> > struct inode *inode;
> > struct dentry *dentry;
> > @@ -3243,36 +3243,30 @@ static const char *do_last(struct nameidata *nd,
> > return ERR_PTR(-ENOTDIR);
> >
> > finish_open_created:
> > + do_truncate = false;
> > + acc_mode = op->acc_mode;
> > if (file->f_mode & FMODE_CREATED) {
> > /* Don't check for write permission, don't truncate */
> > open_flag &= ~O_TRUNC;
> > - will_truncate = false;
> > acc_mode = 0;
> > - } else if (!d_is_reg(nd->path.dentry)) {
> > - will_truncate = false;
> > - }
> > - if (will_truncate) {
> > + } else if (d_is_reg(nd->path.dentry) && open_flag & O_TRUNC) {
> > error = mnt_want_write(nd->path.mnt);
> > if (error)
> > return ERR_PTR(error);
> > - got_write = true;
> > + do_truncate = true;
>
> replacing its role with do_truncate probably makes it easier to maintain
> do_last() leaving readers baffling and digging in logs after going up
> and down the code but failing to grip what is inteded in this patch.
>
> And that kink can be fixed perhaps simply by not axing this got_write.

I'm sorry, I'd been unable to parse that ;-/ What do you mean?