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: <20230626201713.1204982-1-surenb@google.com> <ZJn1tQDgfmcE7mNG@slm.duckdns.org>
 <20230627-kanon-hievt-bfdb583ddaa6@brauner> <CAJuCfpECKqYiekDK6Zw58w10n1T4Q3R+2nymfHX2ZGfQVDC3VQ@mail.gmail.com>
 <20230627-ausgaben-brauhaus-a33e292558d8@brauner> <ZJstlHU4Y3ZtiWJe@slm.duckdns.org>
 <CAJuCfpFUrPGVSnZ9+CmMz31GjRNN+tNf6nUmiCgx0Cs5ygD64A@mail.gmail.com>
In-Reply-To: <CAJuCfpFUrPGVSnZ9+CmMz31GjRNN+tNf6nUmiCgx0Cs5ygD64A@mail.gmail.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Tue, 27 Jun 2023 14:43:06 -0700
Message-ID: <CAJuCfpFe2OdBjZkwHW5UCFUbnQh7hbNeqs7B99PXMXdFNjKb5Q@mail.gmail.com>
Subject: Re: [PATCH 1/2] kernfs: add kernfs_ops.free operation to free
 resources tied to the file
To:     Tejun Heo <tj@kernel.org>
Cc:     Christian Brauner <brauner@kernel.org>, gregkh@linuxfoundation.org,
        peterz@infradead.org, lujialin4@huawei.com,
        lizefan.x@bytedance.com, hannes@cmpxchg.org, mingo@redhat.com,
        ebiggers@kernel.org, oleg@redhat.com, akpm@linux-foundation.org,
        viro@zeniv.linux.org.uk, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com, vschneid@redhat.com,
        linux-kernel@vger.kernel.org, cgroups@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jun 27, 2023 at 1:09=E2=80=AFPM Suren Baghdasaryan <surenb@google.c=
om> wrote:
>
> On Tue, Jun 27, 2023 at 11:42=E2=80=AFAM Tejun Heo <tj@kernel.org> wrote:
> >
> > Hello, Christian.
> >
> > On Tue, Jun 27, 2023 at 07:30:26PM +0200, Christian Brauner wrote:
> > ...
> > > ->release() was added in
> > >
> > >     commit 0e67db2f9fe91937e798e3d7d22c50a8438187e1
> > >     kernfs: add kernfs_ops->open/release() callbacks
> > >
> > >     Add ->open/release() methods to kernfs_ops.  ->open() is called w=
hen
> > >     the file is opened and ->release() when the file is either releas=
ed or
> > >     severed.  These callbacks can be used, for example, to manage
> > >     persistent caching objects over multiple seq_file iterations.
> > >
> > >     Signed-off-by: Tejun Heo <tj@kernel.org>
> > >     Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >     Acked-by: Acked-by: Zefan Li <lizefan@huawei.com>
> > >
> > > which mentions "either releases or severed" which imho already points=
 to
> > > separate methods.
> >
> > This is because kernfs has revoking operation which doesn't exist for o=
ther
> > filesystems. Other filesystem implemenations can't just say "I'm done. =
Bye!"
> > and go away. Even if the underlying filesystem has completely failed, t=
he
> > code still has to remain attached and keep aborting operations.
> >
> > However, kernfs serves as the midlayer to a lot of device drivers and o=
ther
> > internal subsystems and it'd be really inconvenient for each of them to=
 have
> > to implement "I want to go away but I gotta wait out this user who's ho=
lding
> > onto my tuning knob file". So, kernfs exposes a revoke or severing sema=
ntics
> > something that's exposing interface through kernfs wants to stop doing =
so.
> >
> > If you look at it from file operation implementation POV, this seems ex=
actly
> > like ->release. All open files are shutdown and there won't be any futu=
re
> > operations. After all, revoke is forced closing of all fd's. So, for mo=
st
> > users, treating severing just like ->release is the right thing to do.
> >
> > The PSI file which caused this is a special case because it attaches
> > something to its kernfs file which outlives the severing operation bypa=
ssing
> > kernfs infra. A more complete way to fix this would be supporting the
> > required behavior from kernfs side, so that the PSI file operates on ke=
rnfs
> > interface which knows the severing event and detaches properly. That sa=
id,
> > currently, this is very much an one-off.
> >
> > Suren, if you're interested, it might make sense to pipe poll through k=
ernfs
> > properly so that it has its kernfs operation and kernfs can sever it. T=
hat
> > said, as this is a fix for something which is currently causing crashes=
,
> > it'd be better to merge this simpler fix first no matter what.
>
> I'm happy to implement the right fix if you go into more details.
> AFAIKT kernfs_ops already has poll() operation, we are hooking
> cgroup_file_poll() to it and using kernfs_generic_poll(). I thought
> this is the right way to pipe poll through kernfs but if that's
> incorrect, please let me know. I'm happy to fix that.

Ah, sorry, for PSI we are not using kernfs_generic_poll(), so my claim
misrepresents the situation. Let me look into how
kernfs_generic_poll() is implemented and maybe I can find a better
solution for PSI.
Thanks,
Suren.

> Thanks,
> Suren.
>
> >
> > Thanks.
>
> >
> > --
> > tejun