The decision to free kernfs_open_node object in kernfs_put_open_node can
be taken based on whether kernfs_open_node->files list is empty or not. As
far as kernfs_drain_open_files is concerned it can't overlap with
kernfs_fops_open and hence can check for ->attr.open optimistically
(if ->attr.open is NULL) or under kernfs_open_file_mutex (if it needs to
traverse the ->files list.) Thus kernfs_drain_open_files can work w/o ref
counting involved kernfs_open_node as well.
So remove ->refcnt and modify the above mentioned users accordingly.
Suggested by: Al Viro <[email protected]>
Signed-off-by: Imran Khan <[email protected]>
---
fs/kernfs/file.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 88423069407c..aea6968c979e 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -33,7 +33,6 @@ static DEFINE_SPINLOCK(kernfs_open_node_lock);
static DEFINE_MUTEX(kernfs_open_file_mutex);
struct kernfs_open_node {
- atomic_t refcnt;
atomic_t event;
wait_queue_head_t poll;
struct list_head files; /* goes through kernfs_open_file.list */
@@ -530,10 +529,8 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
}
on = kn->attr.open;
- if (on) {
- atomic_inc(&on->refcnt);
+ if (on)
list_add_tail(&of->list, &on->files);
- }
spin_unlock_irq(&kernfs_open_node_lock);
mutex_unlock(&kernfs_open_file_mutex);
@@ -548,7 +545,6 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
if (!new_on)
return -ENOMEM;
- atomic_set(&new_on->refcnt, 0);
atomic_set(&new_on->event, 1);
init_waitqueue_head(&new_on->poll);
INIT_LIST_HEAD(&new_on->files);
@@ -557,11 +553,12 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
/**
* kernfs_put_open_node - put kernfs_open_node
- * @kn: target kernfs_nodet
+ * @kn: target kernfs_node
* @of: associated kernfs_open_file
*
* Put @kn->attr.open and unlink @of from the files list. If
- * reference count reaches zero, disassociate and free it.
+ * list of associated open files becomes empty, disassociate and
+ * free kernfs_open_node.
*
* LOCKING:
* None.
@@ -578,7 +575,7 @@ static void kernfs_put_open_node(struct kernfs_node *kn,
if (of)
list_del(&of->list);
- if (atomic_dec_and_test(&on->refcnt))
+ if (list_empty(&on->files))
kn->attr.open = NULL;
else
on = NULL;
@@ -768,15 +765,15 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE)))
return;
- spin_lock_irq(&kernfs_open_node_lock);
on = kn->attr.open;
- if (on)
- atomic_inc(&on->refcnt);
- spin_unlock_irq(&kernfs_open_node_lock);
if (!on)
return;
mutex_lock(&kernfs_open_file_mutex);
+ if (!kn->attr.open) {
+ mutex_unlock(&kernfs_open_file_mutex);
+ return;
+ }
list_for_each_entry(of, &on->files, list) {
struct inode *inode = file_inode(of->file);
@@ -789,8 +786,6 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
}
mutex_unlock(&kernfs_open_file_mutex);
-
- kernfs_put_open_node(kn, NULL);
}
/*
--
2.30.2
On Sun, Apr 10, 2022 at 12:37:10PM +1000, Imran Khan wrote:
> @@ -768,15 +765,15 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
> if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE)))
> return;
>
> - spin_lock_irq(&kernfs_open_node_lock);
> on = kn->attr.open;
> - if (on)
> - atomic_inc(&on->refcnt);
> - spin_unlock_irq(&kernfs_open_node_lock);
> if (!on)
> return;
>
> mutex_lock(&kernfs_open_file_mutex);
> + if (!kn->attr.open) {
> + mutex_unlock(&kernfs_open_file_mutex);
> + return;
> + }
What if @on got freed and new one got allocated between the lockless check
and the locked check? Is there a reason to keep the lockless check at all?
> list_for_each_entry(of, &on->files, list) {
> struct inode *inode = file_inode(of->file);
Thanks.
--
tejun
Hello Tejun,
On 23/4/22 2:03 am, Tejun Heo wrote:
> On Sun, Apr 10, 2022 at 12:37:10PM +1000, Imran Khan wrote:
>> @@ -768,15 +765,15 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
>> if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE)))
>> return;
>>
>> - spin_lock_irq(&kernfs_open_node_lock);
>> on = kn->attr.open;
>> - if (on)
>> - atomic_inc(&on->refcnt);
>> - spin_unlock_irq(&kernfs_open_node_lock);
>> if (!on)
>> return;
>>
>> mutex_lock(&kernfs_open_file_mutex);
>> + if (!kn->attr.open) {
>> + mutex_unlock(&kernfs_open_file_mutex);
>> + return;
>> + }
>
> What if @on got freed and new one got allocated between the lockless check
> and the locked check? Is there a reason to keep the lockless check at all?
>
The only reason for lockless check is to opportunistically check and
return if ->attr.open is already NULL, without waiting to acquire the
mutex. This is because no one will be adding to ->attr.open at this
point of time.
But we can live with just the locked check as well.
Please let me know if you think of lockless check as an overkill in this
case.
Thanks
-- Imran
On Tue, Apr 26, 2022 at 08:29:21AM -1000, Tejun Heo wrote:
> The code is just wrong. You can end up:
>
> on = kn->attr.open;
> if (!on)
> return;
>
> // we get preempted here and someone else puts @on and then
> // recreates it
Can't happen - the caller has guaranteed that no new openers will be
coming. Look at the call chain of kernfs_drain_open_files()...
Al, back to life and staring at the pile of mail to sift through...
On Tue, Apr 26, 2022 at 08:13:22PM +0000, Al Viro wrote:
> On Tue, Apr 26, 2022 at 08:29:21AM -1000, Tejun Heo wrote:
>
> > The code is just wrong. You can end up:
> >
> > on = kn->attr.open;
> > if (!on)
> > return;
> >
> > // we get preempted here and someone else puts @on and then
> > // recreates it
>
> Can't happen - the caller has guaranteed that no new openers will be
> coming. Look at the call chain of kernfs_drain_open_files()...
Ah, okay. It can only transition to NULL from there. It's still a weird way
to go about it tho.
Thanks.
--
tejun
On Tue, Apr 26, 2022 at 11:43:38AM +1000, Imran Khan wrote:
> Hello Tejun,
>
> On 23/4/22 2:03 am, Tejun Heo wrote:
> > On Sun, Apr 10, 2022 at 12:37:10PM +1000, Imran Khan wrote:
> >> @@ -768,15 +765,15 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
> >> if (!(kn->flags & (KERNFS_HAS_MMAP | KERNFS_HAS_RELEASE)))
> >> return;
> >>
> >> - spin_lock_irq(&kernfs_open_node_lock);
> >> on = kn->attr.open;
> >> - if (on)
> >> - atomic_inc(&on->refcnt);
> >> - spin_unlock_irq(&kernfs_open_node_lock);
> >> if (!on)
> >> return;
> >>
> >> mutex_lock(&kernfs_open_file_mutex);
> >> + if (!kn->attr.open) {
> >> + mutex_unlock(&kernfs_open_file_mutex);
> >> + return;
> >> + }
> >
> > What if @on got freed and new one got allocated between the lockless check
> > and the locked check? Is there a reason to keep the lockless check at all?
>
> The only reason for lockless check is to opportunistically check and
> return if ->attr.open is already NULL, without waiting to acquire the
> mutex. This is because no one will be adding to ->attr.open at this
> point of time.
> But we can live with just the locked check as well.
> Please let me know if you think of lockless check as an overkill in this
> case.
The code is just wrong. You can end up:
on = kn->attr.open;
if (!on)
return;
// we get preempted here and someone else puts @on and then
// recreates it
mutex_lock();
if (!kn->attr.open) {
mutex_unlock();
return;
}
// we're here but @on is a pointer to an already freed memory
Thanks.
--
tejun