Re: [PATCH] files: rcu free files_struct

[Eric W. Biederman]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> On Wed, Dec 9, 2020 at 10:05 AM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>>
>> -                               struct file * file = xchg(&fdt->fd[i], NULL);
>> +                               struct file * file = fdt->fd[i];
>>                                 if (file) {
>> +                                       rcu_assign_pointer(fdt->fd[i], NULL);
>
> This makes me nervous. Why did we use to do that xchg() there? That
> has atomicity guarantees that now are gone.
>
> Now, this whole thing should be called for just the last ref of the fd
> table, so presumably that atomicity was never needed in the first
> place. But the fact that we did that very expensive xchg() then makes
> me go "there's some reason for it".
>
> Is this xchg() just bogus historical leftover? It kind of looks that
> way. But maybe that change should be done separately?

Removing the xchg in a separate patch seems reasonable.  Just to make
the review easier.

I traced the xchg back to 7cf4dc3c8dbf ("move files_struct-related bits
from kernel/exit.c to fs/file.c") when put_files_struct was introduced.
The xchg did not exist before that change.

There were many other xchgs in the code back then so I suspect was left
over from some way an earlier version of the change worked and simply
was not removed when the patch was updated.

Eric