Re: [PATCH 1/2] kernfs: add kernfs_ops.free operation to free resources tied to the file

[Greg KH]

On Wed, Jun 28, 2023 at 11:18:20AM -0700, Suren Baghdasaryan wrote:
> On Wed, Jun 28, 2023 at 11:02 AM Tejun Heo <tj@xxxxxxxxxx> wrote:
> >
> > On Wed, Jun 28, 2023 at 07:35:20PM +0200, Christian Brauner wrote:
> > > > To summarize my understanding of your proposal, you suggest adding new
> > > > kernfs_ops for the case you marked (1) and change ->release() to do
> > > > only (2). Please correct me if I misunderstood. Greg, Tejun, WDYT?
> > >
> > > Yes. I can't claim to know all the intricate implementation details of
> > > kernfs ofc but this seems sane to me.
> >
> > This is going to be massively confusing for vast majority of kernfs users.
> > The contract kernfs provides is that you can tell kernfs that you want out
> > and then you can do so synchronously in a finite amount of time (you still
> > have to wait for in-flight operations to finish but that's under your
> > control). Adding an operation which outlives that contract as something
> > usual to use is guaranteed to lead to obscure future crnashes. For a
> > temporary fix, it's fine as long as it's marked clearly but please don't
> > make it something seemingly widely useable.
> >
> > We have a long history of modules causing crashes because of this. The
> > severing semantics is not there just for fun.
> 
> I'm sure there are reasons things are working as they do today. Sounds
> like we can't change the ->release() logic from what it is today...
> Then the question is how do we fix this case needing to release a
> resource which can be released only when there are no users of the
> file? My original suggestion was to add a kernfs_ops operation which
> would indicate there are no more users but that seems to be confusing.
> Are there better ways to fix this issue?

Just make sure that you really only remove the file when all users are
done with it?  Do you have control of that from the driver side?

But, why is this kernfs file so "special" that it must have this special
construct?  Why not do what all other files that handle polling do and
just remove and get out of there when done?

thanks,

greg k-h