On Tue, Jun 27, 2023 at 10:03:15AM -0700, Suren Baghdasaryan wrote: > On Mon, Jun 26, 2023 at 11:25 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > > > > On Mon, Jun 26, 2023 at 01:17:12PM -0700, Suren Baghdasaryan wrote: > > > kernfs_ops.release operation can be called from kernfs_drain_open_files > > > which is not tied to the file's real lifecycle. Introduce a new kernfs_ops > > > free operation which is called only when the last fput() of the file is > > > performed and therefore is strictly tied to the file's lifecycle. This > > > operation will be used for freeing resources tied to the file, like > > > waitqueues used for polling the file. > > > > This is confusing, shouldn't release be the "last" time the file is > > handled and then all resources attached to it freed? Why do we need > > another callback, shouldn't release handle this? > > That is what I thought too but apparently kernfs_drain_open_files() > can also cause ops->release to be called while the file keeps on > living (see details here: > https://lore.kernel.org/all/CAJuCfpFZ3B4530TgsSHqp5F_gwfrDujwRYewKReJru==MdEHQg@xxxxxxxxxxxxxx/#t). > > > > > > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx> > > > --- > > > fs/kernfs/file.c | 8 +++++--- > > > include/linux/kernfs.h | 5 +++++ > > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c > > > index 40c4661f15b7..acc52d23d8f6 100644 > > > --- a/fs/kernfs/file.c > > > +++ b/fs/kernfs/file.c > > > @@ -766,7 +766,7 @@ static int kernfs_fop_open(struct inode *inode, struct file *file) > > > > > > /* used from release/drain to ensure that ->release() is called exactly once */ > > > static void kernfs_release_file(struct kernfs_node *kn, > > > - struct kernfs_open_file *of) > > > + struct kernfs_open_file *of, bool final) > > > > Adding flags to functions like this are a pain, now we need to look it > > up every time to see what that bool means. > > > > And when we do, we see that it is not documented here so we have no idea > > of what it is :( > > > > This is not going to be maintainable as-is, sorry. > > It's a static function with only two places it's used in the same > file. I can add documentation too if that helps. > > > > > > { > > > /* > > > * @of is guaranteed to have no other file operations in flight and > > > @@ -787,6 +787,8 @@ static void kernfs_release_file(struct kernfs_node *kn, > > > of->released = true; > > > of_on(of)->nr_to_release--; > > > } > > > + if (final && kn->attr.ops->free) > > > + kn->attr.ops->free(of); > > > } > > > > > > static int kernfs_fop_release(struct inode *inode, struct file *filp) > > > @@ -798,7 +800,7 @@ static int kernfs_fop_release(struct inode *inode, struct file *filp) > > > struct mutex *mutex; > > > > > > mutex = kernfs_open_file_mutex_lock(kn); > > > - kernfs_release_file(kn, of); > > > + kernfs_release_file(kn, of, true); > > > mutex_unlock(mutex); > > > } > > > > > > @@ -852,7 +854,7 @@ void kernfs_drain_open_files(struct kernfs_node *kn) > > > } > > > > > > if (kn->flags & KERNFS_HAS_RELEASE) > > > - kernfs_release_file(kn, of); > > > + kernfs_release_file(kn, of, false); > > > > Why isn't this also the "last" time things are touched here? why is it > > false? > > Because it's called from the context of the process doing rmdir() and > if another process has the file in the directory opened it will have > that file alive until it calls the last fput(). These are the call > paths: > > do_rmdir > cgroup_rmdir > kernfs_drain_open_files > kernfs_release_file(..., false) > kn->attr.ops->release(), of->released=true This seems weird to me. Why would that trigger a ->release() call. In general, calling ->release() kinda betrays the name. So imho, this really wants to be a separate ->drain() or ->shutdown() call (and seems conceptually related to f_op->flush()). > > fput() > kernfs_fop_release() > kernfs_release_file(..., true), of->released==true, > kn->attr.ops->release() is not called.
