Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221018041233.376977-1-stephen.s.brennan@oracle.com>
 <20221021010310.29521-1-stephen.s.brennan@oracle.com> <20221021010310.29521-3-stephen.s.brennan@oracle.com>
 <CAOQ4uxj+ctptwuJ__gn=2URvzkXUc2NZkJaY=woGFEQQZdZn9Q@mail.gmail.com>
In-Reply-To: <CAOQ4uxj+ctptwuJ__gn=2URvzkXUc2NZkJaY=woGFEQQZdZn9Q@mail.gmail.com>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Fri, 21 Oct 2022 12:18:28 +0300
Message-ID: <CAOQ4uxh7OvmH6o1fUmMoQ_D347jVBx53TLe4R=BjtXTuvCzKCA@mail.gmail.com>
Subject: Re: [PATCH v2 2/3] fsnotify: Protect i_fsnotify_mask and child flags
 with inode rwsem
To:     Stephen Brennan <stephen.s.brennan@oracle.com>
Cc:     Jan Kara <jack@suse.cz>, Alexander Viro <viro@zeniv.linux.org.uk>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 21, 2022 at 11:22 AM Amir Goldstein <amir73il@gmail.com> wrote:
...
> > +/*
> > + * Objects may need some additional actions to be taken when the last reference
> > + * is dropped. Define flags to indicate which actions are necessary.
> > + */
> > +#define FSNOTIFY_OBJ_FLAG_NEED_IPUT            0x01
> > +#define FSNOTIFY_OBJ_FLAG_UPDATE_CHILDREN      0x02
>
> with changed_flags argument, you do not need these, you can use
> the existing CONN_FLAGS.
>
> It is a bit ugly that the direction of the change is not expressed
> in changed_flags, but for the current code, it is not needed, because
> update_children does care about the direction of the change and
> the direction of change to HAS_IREF is expressed by the inode
> object return value.
>

Oh that is a lie...

return value can be non NULL because of an added mark
that wants iref and also wants to watch children, but the
only practical consequence of this is that you can only
do the WARN_ON for the else case of update_children
in fsnotify_recalc_mask().

I still think it is a win for code simplicity as I detailed
in my comments.

> Maybe try it out in v3 to see how it works.
>
> Unless Jan has an idea that will be easier to read and maintain...
>

Maybe fsnotify_update_inode_conn_flags() should return "update_flags"
and not "changed_flags", because actually the WATCHING_CHILDREN
flag is not changed by the helper itself.

Then, HAS_IREF is not returned when helper did get_iref() and changed
HAS_IREF itself and then the comment that says:
     /* Unpin inode after detach of last mark that wanted iref */
will be even clearer:

        if (want_iref) {
                /* Pin inode if any mark wants inode refcount held */
                fsnotify_get_inode_ref(fsnotify_conn_inode(conn));
                conn->flags |= FSNOTIFY_CONN_FLAG_HAS_IREF;
        } else {
                /* Unpin inode after detach of last mark that wanted iref */
                ret = inode;
                update_flags |= FSNOTIFY_CONN_FLAG_HAS_IREF;
        }

Thanks,
Amir.